-
Notifications
You must be signed in to change notification settings - Fork 421
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 include customData in AzureMachinePool hash calculation #1197
Conversation
8f9ad57
to
7634b05
Compare
Pass 1, retry |
Pass1 was successful Pass 2, retry |
Pass 2 was successful Pass 3, retry |
Pass 3 was successful Pass 4, retry |
Pass 4 was successful Pass 5, retry |
Pass 5 was partially successful with Windows failing with an unrelated error:
Pass 6, retry |
7634b05
to
db46bad
Compare
Pass 6 was partially successful with Windows with VMSS error. Rebased on master to get the latest from v1alpha4 and hopefully cluster data from test clusters that failed to come up. |
Pass 7 was successful Pass 8, retry |
Pass 8 was successful Pass 9, retry |
Pass 9 failed with e[1]: *** [Makefile:249: /home/prow/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/kubectl-v1.19.2] Error 56 again. Looking that looks like it is related to curl having a network issue. created #1202 Pass 10, retry |
/retest |
Pass 11, retry |
Pass 11 success Pass 12, retry |
Pass 12 success Pass 13, retry |
Pass 18 success. Pass 19, retry |
Pass 19 Fail; IPv6 Pass 20, retry |
Pass 20 success Pass 21, retry |
Pass 21 success Pass 22, retry |
11b61d4
to
9210284
Compare
Pass 22 success @nader-ziada, @CecileRobertMichon I think this is looking pretty solid. wdyt? |
looks good to go! passed most of the times /lgtm |
9210284
to
3aeef19
Compare
config/manager/manager.yaml
Outdated
@@ -24,6 +24,7 @@ spec: | |||
- --leader-elect | |||
- "--metrics-bind-addr=127.0.0.1:8080" | |||
- "--feature-gates=MachinePool=${EXP_MACHINE_POOL:=false},AKS=${EXP_AKS:=false}" | |||
- "--v=2" |
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.
was this checked in by mistake?
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 increased verbosity of the test logging. To make it easier, I add an explicit verbosity to the controller.
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.
isn't 1 the default though? if so this increases verbosity for all default clusterctl deployments
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.
Looks like the default is 0. I did not intend on increasing the default. I'll change it.
3aeef19
to
410a36d
Compare
test/e2e/config/azure-dev.yaml
Outdated
@@ -75,6 +75,9 @@ providers: | |||
targetName: "cluster-template-windows.yaml" | |||
- sourcePath: "${PWD}/templates/test/cluster-template-prow-machine-pool-windows.yaml" | |||
targetName: "cluster-template-machine-pool-windows.yaml" | |||
replacements: | |||
- old: "--v=2" |
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.
- old: "--v=2" | |
- old: "--v=0" |
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.
Great catch!! Fixed
410a36d
to
9d2dd13
Compare
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
/approve
nothing was changed in the last few commits that should affect the stability of the fix (added a code comment and changed default verbosity) so we're still good to merge this
[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 type of PR is this?
/kind bug
What this PR does / why we need it:
AzureMachinePool will occasionally enter into a cycle where the hash tag calculated from the last model update will be different than the hash calculated for a newly reconciled AzureMachinePool since the bootstrap data secret has been updated since the previous reconcile causing the customData of the VMSS to change, thus changing the hash for the entire model.
This PR removes the customData from the hash calculation and increases test controller log verbosity.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Related: #1190, #1186, #1182
TODOs:
Release note: