-
Notifications
You must be signed in to change notification settings - Fork 413
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
[DO_NOT_MERGE][DO_NOT_REVIEW] PR capi node labels fix test #3378
[DO_NOT_MERGE][DO_NOT_REVIEW] PR capi node labels fix test #3378
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
[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 |
/test ? |
@ykakarap: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
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-provider-azure-e2e-optional |
Found a bug in the patch. Fixing it now and re-testing. |
/test pull-cluster-api-provider-azure-e2e |
/test pull-cluster-api-provider-azure-e2e-optional |
The |
487f5dc
to
a0e2c86
Compare
/test pull-cluster-api-provider-azure-e2e-optional |
/test pull-cluster-api-provider-azure-e2e |
1 similar comment
/test pull-cluster-api-provider-azure-e2e |
@ykakarap @fabriziopandini the MachinePool job seems to be more flaky than usual in this PR. I looked at one of the failures (https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/3378/pull-cluster-api-provider-azure-e2e/1641831314409656320) and the flake seems like it's related to the uninitialized node taint:
In the pod describe in artifacts I see:
and
So it seems it took a full 16 minutes for the pod to start, and the test timed out right before that after 15 minutes. This part |
The machine should drop the taint from the node almost immediately after infra provider reports
Sounds like a good idea. In such cases the pods should be scheduled almost immediately as the taint should already be dropped at this point. |
@ykakarap I think the issue might be that there's a delay in MachinePools because the MachinePool controller is not watching cluster nodes, unlike the Machine controller. |
@CecileRobertMichon I think you are right. There are not enough events that trigger the machinepool reconciler. It is only watching for changes on I was not able to force any delay in dropping the taints (tested locally) when using Machines, even when I delay the Can we move the "slow MachinePool reconciliation" into a separate issue and get the current fix for syncing node labels merged? |
yes, I'm working on a fix edit: opened kubernetes-sigs/cluster-api#8442 to track |
/test pull-cluster-api-provider-azure-e2e |
pass 1/1 /test pull-cluster-api-provider-azure-e2e |
/test pull-cluster-api-provider-azure-e2e |
1 similar comment
/test pull-cluster-api-provider-azure-e2e |
@ykakarap: The following tests 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. |
/test pull-cluster-api-provider-azure-e2e |
pass 1/1 /test pull-cluster-api-provider-azure-e2e |
pass 2/2 /test pull-cluster-api-provider-azure-e2e |
pass 3/3 /test pull-cluster-api-provider-azure-e2e |
pass 4/4 /test pull-cluster-api-provider-azure-e2e |
5/5 passes 🎉 |
Test with both node watcher + node patching fix /test pull-cluster-api-provider-azure-e2e |
pass 1/1 /test pull-cluster-api-provider-azure-e2e |
pass 2/2 /test pull-cluster-api-provider-azure-e2e |
pass 3/3 /test pull-cluster-api-provider-azure-e2e |
/close |
@CecileRobertMichon: Closed this PR. 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. |
What type of PR is this?
This PR is to test a CAPI patch: kubernetes-sigs/cluster-api#8427
What this PR does / why we need it:
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:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: