✨Update APIVersion for bootstrap references for Machine* resources #1852 #1938

neeleshkorade · 2019-12-19T18:53:47Z

What this PR does / why we need it:

Updates APIVersion for references to KubeadmConfig and KubeadmConfigTemplate for Machine* resources.

Note- Skimming through the code, it appeared to me that it comprised of only two resources- Machine and MachineDeployment. Please let me know if I have missed out any other resources that need to get this change.

Which issue(s) this PR fixes:
Fixes #1852

k8s-ci-robot · 2019-12-19T18:53:55Z

Hi @neeleshkorade. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

ncdc · 2019-12-19T19:03:15Z

/ok-to-test

Thanks for the PR!

Also need to do this for MachineSets.

ncdc · 2019-12-19T19:06:11Z

api/v1alpha2/conversion.go

@@ -155,6 +155,10 @@ func (dst *MachineDeploymentList) ConvertFrom(srcRaw conversion.Hub) error {
 }

 func Convert_v1alpha2_MachineSpec_To_v1alpha3_MachineSpec(in *MachineSpec, out *v1alpha3.MachineSpec, s apiconversion.Scope) error {
+	if out.Bootstrap.ConfigRef.Kind == "KubeadmConfig" || out.Bootstrap.ConfigRef.Kind == "KubeadmConfigTemplate" {


I would also confirm the .Group matches bootstrap.kubeadm.api.v1alpha2.GroupVersion.Group (you can alias the package import to bootstrapv1).

@ncdc for matching Group, this is what I have done-

out.Template.Spec.Bootstrap.ConfigRef.APIVersion = in.Template.Spec.Bootstrap.ConfigRef.GroupVersionKind().Group + "/" + "v1alpha3"

Please let me know if I am mistaken.

I meant this:

inGV, err := schema.ParseGroupVersion(in.Boostrap.ConfigRef.APIVersion) if err != nil { return err } if inGV.Group == bootstrapv1.GroupVersion.Group && (in.Bootstrap.ConfigRef.Kind == "KubeadmConfig" || in.Bootstrap.ConfigRef.Kind == "KubeadmConfigTemplate") { out.Bootstrap.ConfigRef.APIVersion = bootstrapv1.GroupVersion.String() } // ...

Get it. However, bootstrapv1.GroupVersion.String() would return v1alpha2 for the APIVersion and not v1alpha3, isn't it?

It's fine to import the v1alpha3 package as bootstrapv1 - both v1alpha2 and v1alpha3 have the same API group.

Thanks. Updated the PR with the changes.

ncdc · 2019-12-19T19:06:26Z

api/v1alpha2/conversion.go

@@ -164,6 +168,14 @@ func Convert_v1alpha2_MachineSpec_To_v1alpha3_MachineSpec(in *MachineSpec, out *
 	return nil
 }

+func Convert_v1alpha2_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec(in *MachineDeploymentSpec, out *v1alpha3.MachineDeploymentSpec, s apiconversion.Scope) error {
+	if out.Template.Spec.Bootstrap.ConfigRef.Kind == "KubeadmConfig" || out.Template.Spec.Bootstrap.ConfigRef.Kind == "KubeadmConfigTemplate" {


Same comment re group match

neeleshkorade · 2019-12-19T19:58:11Z

/ok-to-test

Thanks for the PR!

Also need to do this for MachineSets.

Added changes to include MachineSets

neeleshkorade · 2019-12-19T20:09:00Z

Looking into the test failures.

neeleshkorade · 2019-12-19T20:55:54Z

@ncdc also, one of the test cases is failing due to lint error-

INFO [runner] linters took 13.243718748s with stages: goanalysis_metalinter: 11.582670405s, unused: 1.602888152s
api/v1alpha2/conversion.go:158:88: string `KubeadmConfigTemplate` has 3 occurrences, make it a constant (goconst)
	if out.Bootstrap.ConfigRef.Kind == "KubeadmConfig" || out.Bootstrap.ConfigRef.Kind == "KubeadmConfigTemplate" {

Trying to understand what's the best way to fix this. I don't particularly like declaring struct names as constants. But I didn't find a pattern elsewhere in the code either. Is there a recommended practice here?

ncdc · 2019-12-19T21:06:49Z

You can put

const (
  kubeadmConfigKind = "KubeadmConfig"
  kubeadmConfigTemplateKind = "KubeadmConfigTemplate"
)

in this file if you'd like

ncdc · 2019-12-19T22:19:07Z

Thanks! Would you mind adding unit tests that test the changes are correct when converting from alpha2 to alpha3?

@detiber the way this change is implemented, it doesn't guarantee that the resources stored in etcd are modified. It just handles the on the fly conversion if the storage version is alpha2. Although because we do use the deferred PatchHelper for all our resources, they'd all get processed and patched the first time a v1alpha3 pod ran... When the MachineDeployments and MachineSets have their KubeadmConfig refs changed, is that going to trigger a rolling update?

neeleshkorade · 2019-12-21T00:46:07Z

@ncdc added unit tests for the three new conversion functions. Please note-

It does not cover the case where incoming v2 object has APIVersion set without Group (that is, only version is present). My guess is this is not a realistic case (and that is how the new conversion functions have been written at the moment) but let me know if I am mistaken.

Please let me know if anything else is missing.

detiber

A couple of nits around how the API group replacement is being done for MachineSets and MachineDeployments, otherwise lgtm

detiber · 2019-12-23T16:20:57Z

api/v1alpha2/conversion.go

+	}
+
+	if inGV.Group == bootstrapv1a3.GroupVersion.Group && (out.Template.Spec.Bootstrap.ConfigRef.Kind == kubeadmConfigKind || out.Template.Spec.Bootstrap.ConfigRef.Kind == kubeadmConfigTemplateKind) {
+		out.Template.Spec.Bootstrap.ConfigRef.APIVersion = in.Template.Spec.Bootstrap.ConfigRef.GroupVersionKind().Group + "/" + "v1alpha3"


Shouldn't this use bootstrapv1a3.GroupVersion.String() like line 175 above?

detiber · 2019-12-23T16:21:23Z

api/v1alpha2/conversion.go

+	}
+
+	if inGV.Group == bootstrapv1a3.GroupVersion.Group && (out.Template.Spec.Bootstrap.ConfigRef.Kind == kubeadmConfigKind || out.Template.Spec.Bootstrap.ConfigRef.Kind == kubeadmConfigTemplateKind) {
+		out.Template.Spec.Bootstrap.ConfigRef.APIVersion = in.Template.Spec.Bootstrap.ConfigRef.GroupVersionKind().Group + "/" + "v1alpha3"


Shouldn't this use bootstrapv1a3.GroupVersion.String() like line 175 above?

ncdc · 2019-12-23T16:23:17Z

@detiber did you see my question above?

detiber · 2019-12-23T16:29:20Z

@detiber the way this change is implemented, it doesn't guarantee that the resources stored in etcd are modified. It just handles the on the fly conversion if the storage version is alpha2. Although because we do use the deferred PatchHelper for all our resources, they'd all get processed and patched the first time a v1alpha3 pod ran... When the MachineDeployments and MachineSets have their KubeadmConfig refs changed, is that going to trigger a rolling update?

It should, however I do wonder if we should also suggest the use of the Storage Version Migrator utility as mentioned here: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#upgrade-existing-objects-to-a-new-stored-version

api/v1alpha2/conversion_test.go

ncdc · 2020-02-04T18:58:45Z

api/v1alpha2/conversion_test.go

@@ -192,7 +216,7 @@ func TestConvertMachineSet(t *testing.T) {
 	})
 }

-func TestConvertMachineDeployment(t *testing.T) {
+func TestConvertMachineDeploymentSpec(t *testing.T) {


Please undo this change

ncdc · 2020-02-04T18:59:47Z

api/v1alpha2/conversion_test.go

@@ -242,4 +279,4 @@ func TestConvertMachineDeployment(t *testing.T) {
 			g.Expect(restored.Spec.Template.Spec.ClusterName).To(Equal(src.Spec.Template.Spec.ClusterName))
 		})
 	})
-}
+}


Please don't remove the newline at the end of the file. Thanks.

ncdc · 2020-02-04T19:00:10Z

hack/tools/go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/blang/semver v3.5.1+incompatible
 	github.com/golangci/golangci-lint v1.21.0
 	github.com/jteeuwen/go-bindata v3.0.7+incompatible
+	golang.org/x/exp v0.0.0-20190312203227-4b39c73a6495 // indirect


Do you know where this is coming from? cc @vincepri

Looks like you need to run make modules

I'm not totally sure, we shouldn't be using anything from the experimental go package afaik, unless we mistakenly updated some deps

Ran make verify-modules and it removed the redundant module. Pushed the change, waiting for the build to go through.

ncdc · 2020-02-04T19:39:18Z

@neeleshkorade there are still outstanding comments - will you be able to address them?

ncdc · 2020-02-05T12:00:20Z

Please squash and rebase on top of master - thanks!

neeleshkorade · 2020-02-05T18:33:45Z

@ncdc running into this issue- make verify-gen fails if golang.org/x/exp v0.0.0-20190312203227-4b39c73a6495 // indirect require dependency is missing in hack/tools/go.mod, while make verify-modules fails if the dependency is there. Any idea how to fix this?

Here's a snippet from the make verify-gen run-

diff --git a/hack/tools/go.mod b/hack/tools/go.mod
index 84d0eefa5..c22cb5436 100644
--- a/hack/tools/go.mod
+++ b/hack/tools/go.mod
@@ -6,6 +6,7 @@ require (
        github.com/blang/semver v3.5.1+incompatible
        github.com/golangci/golangci-lint v1.23.3
        github.com/jteeuwen/go-bindata v3.0.7+incompatible
+       golang.org/x/exp v0.0.0-20190312203227-4b39c73a6495 // indirect
        golang.org/x/tools v0.0.0-20200102140908-9497f49d5709
        k8s.io/code-generator v0.18.0-alpha.2.0.20200130061103-7dfd5e9157ef
        sigs.k8s.io/controller-tools v0.2.4
generated files are out of date, run make generate
make: *** [verify-gen] Error 1

k8s-ci-robot · 2020-02-05T20:25:10Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-e2e	`8ff0ec7`	link	`/test pull-cluster-api-e2e`

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.

neeleshkorade · 2020-02-05T20:37:27Z

/retest

vincepri

/approve
/assign @detiber

k8s-ci-robot · 2020-02-05T21:20:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neeleshkorade, 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:

~~OWNERS~~ [vincepri]

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

detiber · 2020-02-05T21:34:14Z

api/v1alpha2/conversion.go

+		if gvk.Group == bootstrapv1a3.GroupVersion.Group && gvk.Kind == kubeadmConfigTemplateKind {
+			out.Template.Spec.Bootstrap.ConfigRef.APIVersion = bootstrapv1a3.GroupVersion.String()
+		}
+	}


I'm actually wondering if this is strictly needed here, the autoConvert_v1alpha2_MachineSetSpec_To_v1alpha3_MachineSetSpec method will cause Convert_v1alpha2_MachineSpec_To_v1alpha3_MachineSpec to be called.

It should be easy to check by removing this block, the test cases should still pass

This works for Convert_v1alpha2_MachineSetSpec_To_v1alpha3_MachineSetSpec but not for Convert_v1alpha2_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec (the test TestConvertMachineDeployment fails).

Looks like the reason is from here. We updated the if condition to exclude kubeadmConfigTemplateKind so now it fails for MachineDeploymentSpec objects. Updating the if condition makes the test pass.

I added the or condition back in the if statement. Also, as a result, it appeared to me that Convert_v1alpha2_MachineSetSpec_To_v1alpha3_MachineSetSpec and Convert_v1alpha2_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec functions becamse redundant. Have removed those as well. Let me know your feedback.

detiber · 2020-02-05T21:34:59Z

api/v1alpha2/conversion.go

+		if gvk.Group == bootstrapv1a3.GroupVersion.Group && gvk.Kind == kubeadmConfigTemplateKind {
+			out.Template.Spec.Bootstrap.ConfigRef.APIVersion = bootstrapv1a3.GroupVersion.String()
+		}
+	}


I'm actually wondering if this is strictly needed here, the autoConvert_v1alpha2_MachineSetSpec_To_v1alpha3_MachineSetSpec method will cause Convert_v1alpha2_MachineSpec_To_v1alpha3_MachineSpec to be called.

…n v2 to v3 conversion

…nversion code

vincepri · 2020-02-12T22:02:29Z

@neeleshkorade Thinking more about this problem, it seems that this approach might not be what we want.

Going forward, we'll have support to understand contracts with:

A generic solution that understands API contracts for any external reference ⚠️ Add support to auto-update external references #2267
Moving away from ObjectReference (and possibly drop the version field altogether) with Document that providers MUST validate references are in the same namespace #2318

neeleshkorade · 2020-02-14T19:53:36Z

@neeleshkorade Thinking more about this problem, it seems that this approach might not be what we want.

Going forward, we'll have support to understand contracts with:

A generic solution that understands API contracts for any external reference ⚠️ Add support to auto-update external references #2267

Moving away from ObjectReference (and possibly drop the version field altogether) with Document that providers MUST validate references are in the same namespace #2318

Thanks @vincepri I went through the referenced issues and get what you are saying. Let me know if you would like me to close this PR.

vincepri · 2020-02-14T20:33:33Z

Thanks again for understanding!

/close

k8s-ci-robot · 2020-02-14T20:33:36Z

@vincepri: Closed this PR.

In response to this:

Thanks again for understanding!

/close

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.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 19, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 19, 2019

k8s-ci-robot requested review from chuckha and vincepri December 19, 2019 18:54

ncdc self-assigned this Dec 19, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 19, 2019

ncdc added this to the v0.3.0 milestone Dec 19, 2019

ncdc reviewed Dec 19, 2019

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 19, 2019

neeleshkorade force-pushed the update-apiversion branch from 70cd688 to aa9fd8d Compare December 21, 2019 00:39

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 21, 2019

neeleshkorade force-pushed the update-apiversion branch 3 times, most recently from a9f33fd to 165d00a Compare December 21, 2019 01:30

detiber reviewed Dec 23, 2019

View reviewed changes

ncdc reviewed Feb 4, 2020

View reviewed changes

api/v1alpha2/conversion_test.go Show resolved Hide resolved

ncdc reviewed Feb 4, 2020

View reviewed changes

neeleshkorade force-pushed the update-apiversion branch from 2734064 to 1bdee48 Compare February 4, 2020 19:32

neeleshkorade force-pushed the update-apiversion branch from 1bdee48 to 6f654e1 Compare February 5, 2020 04:58

neeleshkorade force-pushed the update-apiversion branch from 6f654e1 to bd936c5 Compare February 5, 2020 18:29

neeleshkorade force-pushed the update-apiversion branch 2 times, most recently from 2278298 to ddefb05 Compare February 5, 2020 20:17

vincepri approved these changes Feb 5, 2020

View reviewed changes

k8s-ci-robot assigned detiber Feb 5, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2020

detiber reviewed Feb 5, 2020

View reviewed changes

neeleshkorade added 2 commits February 9, 2020 16:05

Update APIVersion for bootstrap references for Machine* resources upo…

13c408e

…n v2 to v3 conversion

Reverted redundant changes to MachineSet and MachineDeploymentSpec co…

fcc777c

…nversion code

neeleshkorade force-pushed the update-apiversion branch from ddefb05 to fcc777c Compare February 9, 2020 10:36

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 9, 2020

vincepri mentioned this pull request Feb 12, 2020

webhook conversions for Machine* resources should automatically update the api group version for their bootstrap references #1852

Closed

k8s-ci-robot closed this Feb 14, 2020

✨Update APIVersion for bootstrap references for Machine* resources #1852 #1938

✨Update APIVersion for bootstrap references for Machine* resources #1852 #1938

Conversation

neeleshkorade commented Dec 19, 2019

k8s-ci-robot commented Dec 19, 2019

ncdc commented Dec 19, 2019

Choose a reason for hiding this comment

neeleshkorade Dec 19, 2019 • edited Loading

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

neeleshkorade commented Dec 19, 2019

neeleshkorade commented Dec 19, 2019

neeleshkorade commented Dec 19, 2019

ncdc commented Dec 19, 2019 • edited Loading

ncdc commented Dec 19, 2019

neeleshkorade commented Dec 21, 2019 • edited Loading

detiber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Dec 23, 2019

detiber commented Dec 23, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Feb 4, 2020

ncdc commented Feb 5, 2020

neeleshkorade commented Feb 5, 2020 • edited Loading

k8s-ci-robot commented Feb 5, 2020 • edited Loading

neeleshkorade commented Feb 5, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 5, 2020

Choose a reason for hiding this comment

vincepri Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

neeleshkorade Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Feb 12, 2020

neeleshkorade commented Feb 14, 2020

vincepri commented Feb 14, 2020

k8s-ci-robot commented Feb 14, 2020

neeleshkorade Dec 19, 2019 •

edited

Loading

ncdc commented Dec 19, 2019 •

edited

Loading

neeleshkorade commented Dec 21, 2019 •

edited

Loading

neeleshkorade commented Feb 5, 2020 •

edited

Loading

k8s-ci-robot commented Feb 5, 2020 •

edited

Loading

vincepri Feb 7, 2020 •

edited

Loading

neeleshkorade Feb 9, 2020 •

edited

Loading