Update and improve ReplicationController resource lifecycle test #90880
Conversation
k8s-ci-robot
added
kind/cleanup
|
Hi @Riaankl. Thanks for your PR. I'm waiting for a kubernetes 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. Instructions 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/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
/area conformance |
k8s-ci-robot
added
the
area/conformance
|
This PR replaces #89746 |
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
cncf-cla: yes
|
/retest pull-kubernetes-files-remake |
|
@hh: The
Use In response to this:
Instructions 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/test-infra repository. |
|
/test pull-kubernetes-files-remake |
There was a problem hiding this comment.
/ok-to-test
/release-note-none
It's been a while since I've looked at this, and I've got to admit the test is now so long I'm having a tough time keeping track of what is being done when and why. I'm not sure why all of the watchEvent checks have been added. Do we know they help?
test/e2e/apps/rc.go
Outdated
| eventFound := false | ||
| for watchEvent := range rcWatchChan { | ||
| if watchEvent.Type == watch.Added { | ||
| eventFound = true | ||
| break | ||
| } | ||
| } | ||
| framework.ExpectEqual(eventFound, true, "failed to find RC %v event", watch.Added) |
There was a problem hiding this comment.
nit: for the number of times this is copy-pasted around, I would pull this into a function
expectWatchEventType(rcWatch, watch.Added)
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/apimachinery/watch.go#L397-L431 has something to crib from
There was a problem hiding this comment.
Using framework.WatchUntilWithoutRetry (introduced in #91416) for these checks as of 313f0341ca3cc3de91c664e6f5d001dab1a6852e
test/e2e/apps/rc.go
Outdated
| eventFound = false | ||
| for watchEvent := range rcWatchChan { | ||
| rc, ok := watchEvent.Object.(*v1.ReplicationController) | ||
| framework.ExpectEqual(ok, true, "Unable to convert type of ReplicationController watch watchEvent") | ||
|
|
||
| if rc.Status.Replicas == testRcInitialReplicaCount { | ||
| eventFound = true | ||
| break | ||
| } | ||
| } | ||
| framework.ExpectEqual(eventFound, true, "RC does not have initial replica count") |
There was a problem hiding this comment.
Why do we need this check / what is this trying to test?
If this is trying to wait until the RC status has been updated to undo what we just patched in, it should be looking at rc.Status.ReadyReplicas
test/e2e/apps/rc.go
Outdated
| ginkgo.By("waiting for RC to be modified") | ||
|
|
||
| eventFound = false | ||
| for watchEvent := range rcWatchChan { | ||
| if watchEvent.Type == watch.Modified { | ||
| eventFound = true | ||
| break | ||
| } | ||
| } | ||
| framework.ExpectEqual(eventFound, true, "failed to find RC %v event", watch.Modified) |
There was a problem hiding this comment.
Do we need this? It seems like the below watch event loop is going to wait until the RC status converges to the new number of replicas, I'm not sure why we need to wait before we wait
| @@ -245,9 +301,7 @@ var _ = SIGDescribe("ReplicationController", func() { | |||
| if rcItem.ObjectMeta.Name == testRcName && | |||
| rcItem.ObjectMeta.Namespace == testRcNamespace && | |||
| rcItem.ObjectMeta.Labels["test-rc-static"] == "true" && | |||
| rcItem.ObjectMeta.Labels["test-rc"] == "patched" && | |||
| rcItem.Status.Replicas == testRcMaxReplicaCount && | |||
There was a problem hiding this comment.
should we be checking Spec.Replicas?
k8s-ci-robot
added
ok-to-test
Riaankl
moved this from PRs Needing Review
to PRs Needing Approval
in conformance-definition
Riaankl
moved this from PRs Needing Approval
to PRs Needing Review
in conformance-definition
BobyMCbobs
force-pushed
the
ii-update-and-improve-replicationcontroller-resource-lifecycle-test
branch
from
9caf289
to
e3b9d78
Compare
|
/retest |
|
/test pull-kubernetes-verify |
|
/release-note-none /approve |
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Riaankl, spiffxp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/test pull-kubernetes-e2e-kind |
|
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Updates the test to rely on responses from
PatchorUpdateinstead of laterGetorListWhich issue(s) this PR fixes:
Fixes: #90881
Resolves #90957
#88588 (comment)
Special notes for your reviewer:
Attempts to fix flakiness.
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig testing