✨ Extra validations for v1alpha3 -> v1alpha4 upgrade #4230

shysank · 2021-02-24T19:16:09Z

What this PR does / why we need it:
Validations for v1alpha3 -> v1alpha4 upgrade

Blocks if there are multiple instances of a provider
Deletes capi-webhook-system namespace

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 #4194

shysank · 2021-02-24T19:17:02Z

cmd/clusterctl/client/cluster/upgrader.go

@@ -143,6 +149,13 @@ func (u *providerUpgrader) ApplyPlan(coreProvider clusterctlv1.Provider, contrac
 	log := logf.Log
 	log.Info("Performing upgrade...")

+	// Check for multiple instances of the same provider for v1alpha4 contract.
+	if contract == v1alpha4Contract {


Should this be contract > v1alpha3? Is there an api version parsing library that can be used for this?

If we are moving this check into the doUpgrade func, then we can rely on the upgrade plan and do check only if the current contract is v1alpha3

I wasn't able to find the current contract in upgradePlan, I could only find upgradePlan.contract which I believe is the contract that we want to upgrade to.

shysank · 2021-02-24T19:23:02Z

/test pull-cluster-api-test-main

fabriziopandini

It will be great to have a test for upgrade apply, might be in https://github.com/kubernetes-sigs/cluster-api/blob/master/cmd/clusterctl/client/cluster/upgrader_test.go

cmd/clusterctl/client/cluster/components.go

fabriziopandini · 2021-02-25T10:20:36Z

cmd/clusterctl/client/cluster/components_test.go

+func Test_providerComponents_DeleteCoreProviderWebhookNamespace(t *testing.T) {
+	t.Run("deletes capi-webhook-system namespace", func(t *testing.T) {
+		g := NewWithT(t)
+		labels := map[string]string{


Are these labels required for the test scenario?

cmd/clusterctl/client/cluster/upgrader.go

fabriziopandini · 2021-02-25T10:35:36Z

cmd/clusterctl/client/cluster/upgrader.go

+	providerTypeGrouping := make(map[string][]string)
+	for _, p := range providers.Items {
+		namespacedName := types.NamespacedName{Namespace: p.Namespace, Name: p.Name}.String()
+		if providers, ok := providerTypeGrouping[p.Type]; ok {
+			providerTypeGrouping[p.Type] = append(providers, namespacedName)
+		} else {
+			providerTypeGrouping[p.Type] = []string{namespacedName}
+		}
+	}


You can use provider.ManifestLabel() to uniquely identify a provider (it is a combination of type and name).

I wanted to return the namespaces of the provider in the error message so that it's useful for the user.

I got your point, but my concern is that we are redefining the concept of provider uniqueness (name + type) and provider instance (a combination of type, name + namespace), while there was an effort to centralizing those definitions on the Provider type and make them consistent across the codebase.

Please also note that the proposed implementation uses as criteria for detecting duplicates only type which is not correct because if you have infrastructure docker + infrastructure AWS it will block (instead this should pass).

The only use where we should black is when I have two instances of the same provider, where the same is defined by the uniqueness concept defined above (name + type). e,g if you have infrastructure AWS on ns1, infrastructure AWS on ns2 it should block

Thanks for the explanation, Fabrizio. I think I misread your original comment thinking it was for the value in the map. Thanks for catching it. I've fixed it now and updated the tests to reflect that. Let me know what you think.

cmd/clusterctl/client/cluster/upgrader.go

cmd/clusterctl/client/cluster/upgrader_test.go

cmd/clusterctl/client/cluster/upgrader.go

shysank · 2021-02-26T02:05:00Z

@fabriziopandini I've addressed the review comments, PTAL.

shysank · 2021-02-26T03:30:11Z

/test pull-cluster-api-test-main

fabriziopandini · 2021-02-26T10:53:27Z

cmd/clusterctl/client/cluster/inventory.go

+		return errors.Wrap(kerrors.NewAggregate(errs), "detected multiple instances of the same provider, "+
+			"multi-tenancy does not require to run multiple provider instances from v1alpha4: https://master.cluster-api.sigs.k8s.io/developer/architecture/controllers/support-multiple-instances.html")


Suggested change

return errors.Wrap(kerrors.NewAggregate(errs), "detected multiple instances of the same provider, "+

"multi-tenancy does not require to run multiple provider instances from v1alpha4: https://master.cluster-api.sigs.k8s.io/developer/architecture/controllers/support-multiple-instances.html")

return errors.Wrap(kerrors.NewAggregate(errs), "detected multiple instances of the same provider, "+

"but clusterctl v1alpha4 does not support this use case. See https://cluster-api.sigs.k8s.io/developer/architecture/controllers/support-multiple-instances.html for more details")

fabriziopandini · 2021-02-26T11:06:44Z

cmd/clusterctl/client/cluster/upgrader.go

@@ -49,6 +49,10 @@ type UpgradePlan struct {
 	Providers    []UpgradeItem
 }

+const (
+	v1alpha4Contract = "v1alpha4"


you can use clusterv1.GroupVersion.Version instead of this const, so the code will be more future proof

fabriziopandini · 2021-02-26T11:09:24Z

cmd/clusterctl/client/cluster/upgrader_test.go

+				return
+			}
+
+			g.Expect(err).NotTo(HaveOccurred())


Could we add a check ensuring the webhook namespace is removed?

CecileRobertMichon · 2021-02-26T17:20:43Z

/retitle ✨ Extra validations for v1alpha3 -> v1alpha4 upgrade

vincepri

/lgtm

vincepri · 2021-03-01T16:06:35Z

/retest

vincepri · 2021-03-01T18:15:35Z

/retest

fabriziopandini · 2021-03-05T19:27:49Z

/milestone v0.4.0

shysank · 2021-03-08T14:10:53Z

/test pull-cluster-api-test-main

cmd/clusterctl/client/cluster/upgrader.go

k8s-ci-robot · 2021-04-06T15:04:32Z

@shysank: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	`5afd3ac`	link	`/test pull-cluster-api-apidiff-main`

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.

vincepri

/approve
/lgtm

k8s-ci-robot · 2021-04-06T17:34:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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:

~~cmd/clusterctl/OWNERS~~ [vincepri]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2021

k8s-ci-robot requested review from JoelSpeed and vincepri February 24, 2021 19:16

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 24, 2021

shysank commented Feb 24, 2021

View reviewed changes

fabriziopandini reviewed Feb 25, 2021

View reviewed changes

shysank force-pushed the feature/4194 branch from 13408fe to 0c54037 Compare February 26, 2021 02:00

fabriziopandini reviewed Feb 26, 2021

View reviewed changes

shysank force-pushed the feature/4194 branch from 0c54037 to 8bd9512 Compare February 26, 2021 17:04

k8s-ci-robot changed the title ~~✨ Extra validations for v1alph3 -> valpha4 upgrade~~ ✨ Extra validations for v1alpha3 -> v1alpha4 upgrade Feb 26, 2021

shysank mentioned this pull request Feb 26, 2021

More unit test coverage for clusterctl upgrader #4233

Closed

vincepri approved these changes Mar 1, 2021

View reviewed changes

k8s-ci-robot assigned vincepri Mar 1, 2021

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

k8s-ci-robot added this to the v0.4.0 milestone Mar 5, 2021

shysank force-pushed the feature/4194 branch from 8bd9512 to 7803133 Compare April 1, 2021 16:29

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2021

shysank force-pushed the feature/4194 branch 2 times, most recently from 1810198 to ac44257 Compare April 1, 2021 18:56

fabriziopandini reviewed Apr 6, 2021

View reviewed changes

cmd/clusterctl/client/cluster/upgrader.go Outdated Show resolved Hide resolved

cmd/clusterctl/client/cluster/upgrader.go Outdated Show resolved Hide resolved

extra validations for v1alpha3 -> valpha4 upgrade

5afd3ac

shysank force-pushed the feature/4194 branch from ac44257 to 5afd3ac Compare April 6, 2021 15:02

vincepri approved these changes Apr 6, 2021

View reviewed changes

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

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

k8s-ci-robot merged commit c5b434a into kubernetes-sigs:master Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Extra validations for v1alpha3 -> v1alpha4 upgrade #4230

✨ Extra validations for v1alpha3 -> v1alpha4 upgrade #4230

shysank commented Feb 24, 2021

shysank Feb 24, 2021 •

edited

Loading

fabriziopandini Feb 25, 2021

shysank Feb 26, 2021

shysank commented Feb 24, 2021

fabriziopandini left a comment

fabriziopandini Feb 25, 2021

fabriziopandini Feb 25, 2021

shysank Feb 26, 2021

fabriziopandini Feb 26, 2021

shysank Feb 26, 2021

shysank commented Feb 26, 2021

shysank commented Feb 26, 2021

fabriziopandini Feb 26, 2021

fabriziopandini Feb 26, 2021

fabriziopandini Feb 26, 2021

CecileRobertMichon commented Feb 26, 2021

vincepri left a comment

vincepri commented Mar 1, 2021

vincepri commented Mar 1, 2021

fabriziopandini commented Mar 5, 2021

shysank commented Mar 8, 2021

k8s-ci-robot commented Apr 6, 2021 •

edited

Loading

vincepri left a comment

k8s-ci-robot commented Apr 6, 2021

		return errors.Wrap(kerrors.NewAggregate(errs), "detected multiple instances of the same provider, "+
		"multi-tenancy does not require to run multiple provider instances from v1alpha4: https://master.cluster-api.sigs.k8s.io/developer/architecture/controllers/support-multiple-instances.html")

✨ Extra validations for v1alpha3 -> v1alpha4 upgrade #4230

✨ Extra validations for v1alpha3 -> v1alpha4 upgrade #4230

Conversation

shysank commented Feb 24, 2021

shysank Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shysank commented Feb 24, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shysank commented Feb 26, 2021

shysank commented Feb 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Feb 26, 2021

vincepri left a comment

Choose a reason for hiding this comment

vincepri commented Mar 1, 2021

vincepri commented Mar 1, 2021

fabriziopandini commented Mar 5, 2021

shysank commented Mar 8, 2021

k8s-ci-robot commented Apr 6, 2021 • edited Loading

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 6, 2021

shysank Feb 24, 2021 •

edited

Loading

k8s-ci-robot commented Apr 6, 2021 •

edited

Loading