-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Make node drain delete timeout configurable in e2e framework #4830
🌱 Make node drain delete timeout configurable in e2e framework #4830
Conversation
/test ls |
@CecileRobertMichon: The specified target(s) for
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-cluster-api-e2e-full-main |
@@ -128,3 +128,4 @@ intervals: | |||
default/wait-machine-remediation: ["5m", "10s"] | |||
node-drain/wait-deployment-available: ["3m", "10s"] | |||
node-drain/wait-control-plane: ["15m", "10s"] | |||
node-drain/wait-machine-deleted: ["2m", "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.
Being new to this code this name makes me think this is the timeout to wait for a machine to be fully deleted, though by reading the code I understand this is the timeout for the new machine to come up, not sure if there's a better name may be node-drain/wait-machine-recreated
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.
this is the timeout waiting for a machine to be fully deleted (if you look at where it's used in the code, it's for scaling to 0 replica count for MD - and 3 to 1 for control plane)
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.
Tried to explain this in PR description, let me know if it's unclear:
Now, we take a node-drain/wait-machine-deleted interval in the e2e config and calculate the appropriate machine deployment and control plane scale down timeout by adding the provided machine deletion timeout with the nodeDrainTimeout and multiply it by the number of machines.
/lgtm |
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.
/lgtm
/hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon 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 |
What this PR does / why we need it: Previously, we were adding 2 minutes to the nodeDrainTimeout for each node that needed to get deleted in order to come up with the interval timeout in the nodeDrainTimeout test. 2 minutes is not flexible enough for all infrastructure and providers, it should be a configurable timeout.
Now, we take a
node-drain/wait-machine-deleted
interval in the e2e config and calculate the appropriate machine deployment and control plane scale down timeout by adding the provided machine deletion timeout with the nodeDrainTimeout and multiply it by the number of machines.There is no behavior change for the existing docker infra node drain test (timeouts stay the same for MD and KCP)
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 #