tests: add a test to validate pause and resume feature#351
tests: add a test to validate pause and resume feature#351darthsuburbus wants to merge 3 commits intokubernetes-sigs:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: darthsuburbus 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 |
|
Welcome @darthsuburbus! |
|
Hi @darthsuburbus. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for agent-sandbox canceled.
|
vicentefb
left a comment
There was a problem hiding this comment.
Thanks! This is a very valuable test that was missing. Overall, the logic is very solid. You're correctly simulating the exact behavior the controller executes when Replicas hits 0 (where it deletes the pod but retains the Sandbox CR and PVCs ), and then successfully validating the recovery. Left some comments.
8f8720b to
7103823
Compare
Thanks for the thoughtful feedback, I've addressed all comments. I can squash the commits before merging but keeping them for ease of review for now. |
| @@ -0,0 +1,295 @@ | |||
| // Copyright 2026 The Kubernetes Authors. | |||
There was a problem hiding this comment.
we've stopped putting the dates on newly created files
| // Copyright 2026 The Kubernetes Authors. | |
| // Copyright The Kubernetes Authors. |
(at least this is true in k/k... if this change results in a failure here you can feel free to disregard this comment)
| } | ||
| opts := []cmp.Option{ | ||
| cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"), | ||
| cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime", "Message", "ObservedGeneration"), |
There was a problem hiding this comment.
why ignore observedGeneration? Seems that it may be a useful thing to check?
|
|
||
| // Namespace | ||
| ns := &corev1.Namespace{} | ||
| ns.Name = fmt.Sprintf("pvc-persistence-test-%d", time.Now().UnixNano()) |
There was a problem hiding this comment.
to make this easier to read, can we combine this with L64; just set the value when creating the object struct?
| pvcName := fmt.Sprintf("%s-%s", workspacesPVCTemplateName, pvcSandboxName) | ||
| pvc := &corev1.PersistentVolumeClaim{} | ||
| pvc.Name = pvcName | ||
| pvc.Namespace = ns.Name |
There was a problem hiding this comment.
same here; set these values in the object struct directly?
| pod := &corev1.Pod{} | ||
| pod.Name = pvcSandboxName | ||
| pod.Namespace = ns.Name |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Description
Fixes #248
Add a e2e test that tests out the pause and resume functionality implemented in #82:
This test addresses the request made in #248.
Testing Done