test: validate AKS node pool provisioned stability #4124
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
|
This looks good to me. However, I wonder if we need to add more scenarios on top of this improvement. Improvements like:
|
test/e2e/aks.go
Outdated
| func WaitForAKSMachinePoolsToStabilize(ctx context.Context, input WaitForControlPlaneAndMachinesReadyInput, intervals ...interface{}) { | ||
| var specName = "azuremanagedmachinepools-stabilize" | ||
| // First we wait for all AzureManagedMachinePools to be in a Ready state | ||
| Eventually(func(g Gomega) bool { |
There was a problem hiding this comment.
Does the boolean here have any effect?
|
|
||
| return false | ||
| }, intervals...).Should(Succeed()) | ||
| // Then we wait for a time to verify that no pool re-enters a non-Ready state |
There was a problem hiding this comment.
I think the big risk here is that it will make the tests flakier, so I wonder if it may be worth building in some threshold. But we could adjust later since I think this is worth getting in.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4124 +/- ##
=======================================
Coverage 57.66% 57.66%
=======================================
Files 188 188
Lines 19320 19320
=======================================
Hits 11140 11140
Misses 7548 7548
Partials 632 632 ☔ View full report in Codecov by Sentry. |
jackfrancis
force-pushed
the
test-aks-stable-pools
branch
from
2ceb709 to
61ffb6d
Compare
test/e2e/aks.go
Outdated
| for _, pool := range ammpList.Items { | ||
| g.Expect(pool.Status.Ready).To(BeFalse()) | ||
| } | ||
|
|
||
| }, e2eConfig.GetIntervals(specName, "wait-machines-provisioned-stability")...).Should(Not(Succeed())) |
There was a problem hiding this comment.
I'm having a hard time grokking exactly what this is doing. Could this maybe be expressed a bit more clearly with Consistently instead of Eventually?
Signed-off-by: Jack Francis <[email protected]>
jackfrancis
force-pushed
the
test-aks-stable-pools
branch
from
61ffb6d to
ca57acf
Compare
|
@jackfrancis: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
What type of PR is this?
What this PR does / why we need it:
This PR adds a new test to the AKS E2E scenario to validate stability of AzureManagedMachinePool resources after they have come Ready.
Until #4116 is fixed, this new test scenario should cause the AKS E2E scenario to fail.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
Release note: