-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Azure cluster-autoscaler e2e test #6989
Conversation
@@ -46,8 +46,15 @@ install-e2e: $(HELM) | |||
--set image.repository=$(IMAGE)-$(GOARCH) \ | |||
--set image.tag=$(TAG) \ | |||
--set image.pullPolicy=Always \ | |||
--set extraArgs.scale-down-delay-after-add=10s \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically what I was aiming for with this new config was "react as fast as possible" so any feedback on how to best achieve that is most welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
) | ||
|
||
BeforeEach(func() { | ||
Eventually(allVMSSStable, "10m", "30s").Should(Succeed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these timeouts are kind of arbitrary, so I'm totally open to tweaking these.
@@ -162,11 +162,6 @@ spec: | |||
mode: System | |||
owner: | |||
name: ${CLUSTER_NAME} | |||
tags: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try to isolate things like kube-system Pods from the side effects of these tests? That's what I was trying to achieve here by removing the autoscaler tags from this System pool but I can still see things like coredns sometimes scheduling onto User pools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the connection here, but I think at this phase of the game this is good enough, and we're in good shape to raply make tweaks to cover these types of things that make testing cluster-autoscaler tricky.
/assign @jackfrancis @willie-yao |
this is a great start! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, nojnhuh 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 |
Sorry I was late to this, lgtm!! |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR adds the first real functional test of the Azure provider for cluster-autoscaler. The test creates a bunch of Pods, waits for the cluster to scale up, deletes the pods, and waits for the cluster to scale back down.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: