v1beta1 cluster upgrade tests (using clusterctl upgrade) #1771

shysank · 2021-10-11T18:27:52Z

What type of PR is this?

What this PR does / why we need it:

adds v1alpha3 -> v1beta1 upgrade test
~~TODO: v1alpha4 -> v1beta1 upgrade test~~ This will require changes in capi e2e test framework 🌱 add pre init hook to clusterctl upgrade test cluster-api#5521, hold until then.

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 #1727 #1770

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:

squashed commits
includes documentation
adds unit tests

Release note:

Fix default diff issue when upgrading clusters from v1alpha3 to v1beta1

shysank · 2021-10-11T18:28:28Z

api/v1beta1/azuremachinetemplate_webhook.go

@@ -63,6 +63,7 @@ func (r *AzureMachineTemplate) ValidateUpdate(oldRaw runtime.Object) error {
 	var allErrs field.ErrorList
 	old := oldRaw.(*AzureMachineTemplate)

+	old.Spec.Template.Spec.OSDisk.CachingType = "None"


shysank · 2021-10-11T18:29:52Z

cc @sonasingh46

templates/test/ci/v1alpha3/cluster-template-prow.yaml

test/e2e/azure_test.go

shysank · 2021-10-14T00:12:19Z

/test ls

k8s-ci-robot · 2021-10-14T00:12:21Z

@shysank: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-build
/test pull-cluster-api-provider-azure-e2e
/test pull-cluster-api-provider-azure-e2e-windows
/test pull-cluster-api-provider-azure-test
/test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-azure-apidiff
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance-v1alpha4
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-coverage
/test pull-cluster-api-provider-azure-e2e-exp
/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-e2e-workload-upgrade-1-22-latest-main
/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows
/test pull-cluster-api-provider-azure-windows-upstream-with-ci-artifacts

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-azure-apidiff
pull-cluster-api-provider-azure-build
pull-cluster-api-provider-azure-coverage
pull-cluster-api-provider-azure-e2e
pull-cluster-api-provider-azure-e2e-exp
pull-cluster-api-provider-azure-e2e-windows
pull-cluster-api-provider-azure-test
pull-cluster-api-provider-azure-verify

In response to this:

/test ls

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.

shysank · 2021-10-14T00:28:34Z

/test pull-cluster-api-provider-azure-capi-e2e

shysank · 2021-10-14T05:14:28Z

/test pull-cluster-api-provider-azure-capi-e2e

shysank · 2021-10-14T18:22:07Z

Upgrade tests are working now https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/1771/pull-cluster-api-provider-azure-capi-e2e/1448517546729803776/build-log.txt

�[1mSTEP�[0m: THE MANAGEMENT CLUSTER WAS SUCCESSFULLY UPGRADED!
INFO: Scaling machine deployment clusterctl-upgrade/clusterctl-upgrade-i30nkp-md-0 from 1 to 2 replicas
INFO: Waiting for correct number of replicas to exist
�[1mSTEP�[0m: THE UPGRADED MANAGEMENT CLUSTER WORKS!
�[1mSTEP�[0m: PASSED!

shysank · 2021-10-14T18:23:11Z

@CecileRobertMichon @devigned wdyt think about the changes?

CecileRobertMichon · 2021-10-14T21:05:43Z

api/v1beta1/azurecluster_webhook.go

-				c.Spec.AzureEnvironment, "field is immutable"),
-		)
+		// If old spec is in v1alpha3, we need to set defaults before comparison. This is because defaulting webhook was added
+		// in v1alpha4, so old spec that was created in v1alpha3 would not have run through the defaulting webhook when it was


Looking back on #1244 when this was first introduced, this is breaking for users who were previously using non-default clouds with v1alpha3 but looks like that was added in release notes. I wonder if doing this in conversion wouldn't be better though, it feels wrong run through the defaulting it in the webhook validation every single time, and we also have no way to tell if old really is v1alpha3

I would also clarify that it's not just the defaulting webhook that was added in v1alpha4, it's the AzureEnvironment API field itself

I wonder if doing this in conversion wouldn't be better though, it feels wrong run through the defaulting it in the webhook validation every single time, and we also have no way to tell if old really is v1alpha3

Having this in conversion seems more logical. I didn't do it for two reasons:

We'll have to disable fuzz testing for a whole bunch of fields since we'll be overriding the values.

Duplicate the defaulting code in conversion.

I'm also a bit skeptical about this fix in general because there could be other fields that will be affected which we won't know until we test all the flavors. One potential idea that was floated internally was to not have defaulting webhook for update. This makes sense in principle as the defaults should only be set during creation. We'll probably still have to set some defaults during conversion, but at least we'll have some kind of base to work on. wdyt?

One potential idea that was floated internally was to not have defaulting webhook for update. This makes sense in principle as the defaults should only be set during creation.

the problem I see with this is that if controllers in a certain api version expect a certain field to be set (eg. the AzureCluster v1alpha4 controller won't work if AzureEnvironment isn't set) it will fail reconciliation after upgrade, unless we default it in conversion, but then I don't see how not having a defaulting webhook for update helps?

Yeah, we'll have it to still set some defaults in conversion. Not having default webhook for update gives us more predictability on what the old and new objects will be during update as they are run through the same admission webhooks. Eg. consider a scenario where an object was created in version x.y.0. And a new default is added in a patch release x.y.1(for the same api version). Now when the provider is updated, the object created in x.y.0 may fail during reconciliation if it tries to update because the defaults have changed.

Adding the default in conversion will only exist for a little while. v1alpha3 will be going away in a relatively short time. We may need to specialize fuzzing for that conversion, but it won't be around that long.

Conversion, however untidy, seems to be the place.

So, I tried moving it to conversion, but it doesn't work. My assumption that the old object is run through conversion webhook appears to be wrong.

Digging a little deeper: controller runtime's handle tries to decode the old object using api machinery's UniversalDesiralizer which, as mentioned in the the comments, does not perform conversion/defaulting.

The conversion/defaulting happens in the api server. The understanding till now is that the api server does not run conversion, and defaulting webhooks for "GET" requests. More discussion here

shysank · 2021-10-17T22:18:18Z

/test pull-cluster-api-provider-azure-capi-e2e

shysank · 2021-10-27T22:02:09Z

/test pull-cluster-api-provider-azure-capi-e2e

Co-authored-by: sonasingh46 <sonasingh46@gmail.com>

shysank · 2021-10-27T22:19:25Z

/test pull-cluster-api-provider-azure-capi-e2e

shysank · 2021-10-28T01:30:44Z

Upgrade test is passing, but other tests that are unrelated to this pr are failing. Trying again.
/test pull-cluster-api-provider-azure-capi-e2e

CecileRobertMichon · 2021-10-28T14:52:14Z

quick start spec failed 🤔

/retest

CecileRobertMichon

/lgtm
/approve
/hold for passing capi test

k8s-ci-robot · 2021-10-28T16:27:02Z

[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:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

shysank · 2021-10-28T17:33:44Z

@CecileRobertMichon tests are passing now, finally 😄

CecileRobertMichon · 2021-10-28T18:04:43Z

/hold cancel

k8s-ci-robot · 2021-10-28T20:20:45Z

@shysank: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-e2e-windows	`5d06a8f`	link	true	`/test pull-cluster-api-provider-azure-e2e-windows`

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.

shysank · 2021-10-28T20:30:15Z

/retest

k8s-ci-robot requested review from devigned and nader-ziada October 11, 2021 18:28

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Oct 11, 2021

shysank commented Oct 11, 2021

View reviewed changes

shysank force-pushed the v1beta1_upgrade_tests branch from 4104db9 to b10d17d Compare October 11, 2021 18:29

CecileRobertMichon reviewed Oct 11, 2021

View reviewed changes

templates/test/ci/v1alpha3/cluster-template-prow.yaml Outdated Show resolved Hide resolved

sonasingh46 reviewed Oct 13, 2021

View reviewed changes

test/e2e/azure_test.go Outdated Show resolved Hide resolved

shysank force-pushed the v1beta1_upgrade_tests branch from b10d17d to bce1b2c Compare October 13, 2021 22:10

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 13, 2021

shysank force-pushed the v1beta1_upgrade_tests branch 2 times, most recently from 7c087f8 to 12667db Compare October 14, 2021 00:27

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 14, 2021

shysank force-pushed the v1beta1_upgrade_tests branch from 12667db to 9483fab Compare October 14, 2021 05:14

CecileRobertMichon reviewed Oct 14, 2021

View reviewed changes

shysank force-pushed the v1beta1_upgrade_tests branch from 9483fab to 8ef1b69 Compare October 17, 2021 22:17

shysank force-pushed the v1beta1_upgrade_tests branch from 6062349 to 9abce43 Compare October 27, 2021 22:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2021

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 27, 2021

shysank and others added 3 commits October 27, 2021 15:18

add v1alpha3 -> v1beta1 upgrade test

1a48003

Co-authored-by: sonasingh46 <sonasingh46@gmail.com>

fix default mismatch during v1alpha3 -> v1beta1 upgrade

4de6ecd

add test for default mismatch

aefd581

shysank force-pushed the v1beta1_upgrade_tests branch from 9abce43 to aefd581 Compare October 27, 2021 22:18

CecileRobertMichon added this to the v1.0 milestone Oct 28, 2021

CecileRobertMichon reviewed Oct 28, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2021

k8s-ci-robot assigned CecileRobertMichon Oct 28, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2021

k8s-ci-robot merged commit df6c654 into kubernetes-sigs:main Oct 28, 2021

k8s-ci-robot modified the milestones: v1.0, v0.5 Oct 28, 2021

shysank mentioned this pull request Nov 2, 2021

cluster upgrade default fix for v1alpha3 -> v1alpha4 #1824

Merged

3 tasks

shysank mentioned this pull request Nov 16, 2021

Separate out upgrade specs into its own job #1869

Closed

shysank mentioned this pull request Jan 7, 2022

Machine deployment scale fails after v1alpha3 -> v1alpha4,v1beta1 upgrade #1770

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1beta1 cluster upgrade tests (using clusterctl upgrade) #1771

v1beta1 cluster upgrade tests (using clusterctl upgrade) #1771

shysank commented Oct 11, 2021 •

edited

Loading

shysank Oct 11, 2021

shysank commented Oct 11, 2021

shysank commented Oct 14, 2021

k8s-ci-robot commented Oct 14, 2021

shysank commented Oct 14, 2021

shysank commented Oct 14, 2021

shysank commented Oct 14, 2021

shysank commented Oct 14, 2021

CecileRobertMichon Oct 14, 2021

shysank Oct 14, 2021 •

edited

Loading

CecileRobertMichon Oct 14, 2021

shysank Oct 15, 2021

devigned Oct 15, 2021

shysank Oct 18, 2021 •

edited

Loading

shysank commented Oct 17, 2021

shysank commented Oct 27, 2021

shysank commented Oct 27, 2021

shysank commented Oct 28, 2021

CecileRobertMichon commented Oct 28, 2021

CecileRobertMichon left a comment

k8s-ci-robot commented Oct 28, 2021

shysank commented Oct 28, 2021

CecileRobertMichon commented Oct 28, 2021

k8s-ci-robot commented Oct 28, 2021 •

edited

Loading

shysank commented Oct 28, 2021

v1beta1 cluster upgrade tests (using clusterctl upgrade) #1771

v1beta1 cluster upgrade tests (using clusterctl upgrade) #1771

Conversation

shysank commented Oct 11, 2021 • edited Loading

shysank Oct 11, 2021

Choose a reason for hiding this comment

shysank commented Oct 11, 2021

shysank commented Oct 14, 2021

k8s-ci-robot commented Oct 14, 2021

shysank commented Oct 14, 2021

shysank commented Oct 14, 2021

shysank commented Oct 14, 2021

shysank commented Oct 14, 2021

CecileRobertMichon Oct 14, 2021

Choose a reason for hiding this comment

shysank Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

CecileRobertMichon Oct 14, 2021

Choose a reason for hiding this comment

shysank Oct 15, 2021

Choose a reason for hiding this comment

devigned Oct 15, 2021

Choose a reason for hiding this comment

shysank Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

shysank commented Oct 17, 2021

shysank commented Oct 27, 2021

shysank commented Oct 27, 2021

shysank commented Oct 28, 2021

CecileRobertMichon commented Oct 28, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 28, 2021

shysank commented Oct 28, 2021

CecileRobertMichon commented Oct 28, 2021

k8s-ci-robot commented Oct 28, 2021 • edited Loading

shysank commented Oct 28, 2021

shysank commented Oct 11, 2021 •

edited

Loading

shysank Oct 14, 2021 •

edited

Loading

shysank Oct 18, 2021 •

edited

Loading

k8s-ci-robot commented Oct 28, 2021 •

edited

Loading