Add e2e tests for warmpool rollout #562
Add e2e tests for warmpool rollout #562shrutiyam-glitch wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for agent-sandbox canceled.
|
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shrutiyam-glitch The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
codebot-robot
left a comment
codebot-robot
left a comment
There was a problem hiding this comment.
Overall, this PR introduces solid E2E test coverage for the SandboxWarmPool rollout behavior and update strategies. However, there are a few areas where the tests can be improved to ensure stability, prevent false positives, and increase maintainability:
- Update Conflicts: Directly calling
.Get()followed immediately by.Update()on Kubernetes resources is susceptible toOptimisticLockErrors if a controller updates the object's status concurrently. Consider usingretry.RetryOnConflict. - Timing, Flakiness, and False Positives:
- Avoid using
time.Sleepto wait for reconciliations, as it slows down tests and introduces flakiness. Wait for theObservedGenerationto update instead. - Direct usage of
tc.Listmight fail if the local informer cache is slightly delayed. Wrapping it inrequire.Eventuallyis a safer approach used elsewhere in your tests. - The
verifySandboxStaysSamefunction may assert the state before the controller has processed the template update, which would falsely pass even if a bug exists.
- Avoid using
- Performance: When iterating over
sandboxList.Items, iterate by index (&sandboxList.Items[i]) instead of by range value to prevent copying large objects and avoid pointer issues. - Code Cleanup and Maintainability:
- Consider extracting repetitive logic, such as the block used to find the active sandbox, into helper functions.
- Clean up unused function parameters, such as
_ *framework.TestContextin your helper functions.
Please review the detailed inline comments for specific suggestions.
(This review was generated by Overseer)
| }) | ||
| } | ||
|
|
||
| func verifySandboxStaysSame(t *testing.T, tc *framework.TestContext, ns *corev1.Namespace, poolSandboxName string, sandboxWarmpoolID types.NamespacedName) { |
There was a problem hiding this comment.
This check is evaluated immediately after updating the template. Because the controller processes updates asynchronously, this assertion may succeed before the controller has even processed the update, leading to a false positive test pass. You should add a small delay (e.g., time.Sleep as used in TestWarmPoolRolloutMetadataUpdate) or verify that the warmpool's ObservedGeneration (if tracked) has updated to ensure the controller has genuinely decided to keep the sandbox unchanged.
| return true | ||
| } | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
can we log the error t.Logf here so that it's easier to debug please
| verify: verifySandboxStaysSame, | ||
| }, | ||
| { | ||
| name: "onreplenish", |
There was a problem hiding this comment.
are you also testing that it creates a new sandbox and that sandbox has the updated spec ? i think the full lifecycle would be useful to test out here otherwise rollout could be broken without us knowing
| templateA := createSandboxTemplate(tc, ns, "template-a") | ||
| require.NoError(t, tc.CreateWithCleanup(t.Context(), templateA)) | ||
|
|
||
| templateB := createSandboxTemplate(tc, ns, "template-b") |
There was a problem hiding this comment.
i know that templateA and templateB have identical specs, but down in this test are you checking that the annotation SandboxTemplateRefAnnotation is also being updated ?
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| func createSandboxTemplate(_ *framework.TestContext, ns *corev1.Namespace, name string) *extensionsv1alpha1.SandboxTemplate { |
There was a problem hiding this comment.
super nit: do we need _ *framework.TestContext since it's not being used ?
4 participants
The added tests verify the behavior of the
SandboxWarmPoolcontroller when its associatedSandboxTemplateis updated, ensuring that sandboxes are handled correctly according to the configuredUpdateStrategy.Follow up on #347.