⚠️ Add support to auto-update external references #2267

vincepri · 2020-02-05T20:37:05Z

Signed-off-by: Vince Prignano vincepri@vmware.com

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

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

Going to add tests once code looks good / this is what we expect

k8s-ci-robot · 2020-02-05T20:37:29Z

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

~~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:56:35Z

controllers/machinedeployment_controller.go

@@ -182,6 +196,18 @@ func (r *MachineDeploymentReconciler) reconcile(_ context.Context, cluster *clus
 	return ctrl.Result{}, errors.Errorf("unexpected deployment strategy type: %s", d.Spec.Strategy.Type)
 }

+func (r *MachineDeploymentReconciler) reconcileExternalReference(ctx context.Context, cluster *clusterv1.Cluster, ref *corev1.ObjectReference) error {


Should we go ahead and set ownerref (non-controller) here for the referenced templates as well? I'm worried that we could potentially delete a template that is still referenced by an existing MachineDeployment if for some reason all of the associated MachineSets are deleted.

docs/book/src/developer/providers/v1alpha2-to-v1alpha3.md

detiber · 2020-02-05T22:08:41Z

util/conversion/conversion.go

 	"testing"

 	fuzz "github.com/google/gofuzz"
 	"github.com/onsi/gomega"
+	"github.com/pkg/errors"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"


This import caused me to double take... not sure why they aren't exposing this out via k8s.io/api/apiextensions/...

Related-ish kubernetes/client-go#247

util/conversion/conversion.go

detiber · 2020-02-06T01:03:49Z

util/conversion/conversion.go

+	// If there is no label, return early without changing the reference.
+	supportedVersions, ok := crd.Labels[contract]
+	if !ok {
+		return nil


Should we return an error here since we are saying that the labels will be required for v1alpha3+?

I was wondering the same if we should treat it as an error, or just let the reference be what it is. I'd prefer erroring out though

I think my preference here is to go ahead and error, since it is breaking the contract we are defining, but I'll defer to @ncdc for final ack/nack :)

detiber · 2020-02-06T01:14:58Z

util/conversion/conversion.go

+
+	// Modify the GroupVersionKind with the new version.
+	gvk := ref.GroupVersionKind()
+	gvk.Version = versions[len(versions)-1]


I'm wondering if it would make sense to sort the available versions using https://godoc.org/k8s.io/apimachinery/pkg/version#CompareKubeAwareVersionStrings?

Ah nice! I can definitely add this in

vincepri · 2020-02-06T03:29:41Z

Tests are gonna fail until kubernetes-sigs/controller-tools#400 is implemented, because that will change the actual base files vs kustomize won't until they're rebuilt. This is of course a possible solution, if we want to go down another path, that's also fine.

The method to check for the CRD versions is also going to be very expensive until we can use kubernetes-sigs/controller-runtime#792

rudoi · 2020-02-14T19:02:26Z

@vincepri aside from the controller-tools changes, is this generally good to go, or is there more to do?

It's the last piece of the puzzle!!!

vincepri · 2020-02-14T19:16:42Z

The controller-tools change is needed for tests, I'll try to see if I can do a patch locally though

vincepri · 2020-02-14T19:35:20Z

I might get away with it in tests for now, pushed an update

vincepri · 2020-02-14T19:39:16Z

This is good to go @detiber @ncdc @rudoi

controllers/machine_controller_phases.go

fabriziopandini

LGTM for me, two nits not blocking

util/util.go

fabriziopandini · 2020-02-14T20:46:17Z

util/util.go

+// optional "alpha" or "beta" strings followed by a minor version (e.g. v1, v2beta1).
+// Versions will be sorted based on GA/alpha/beta first and then major and minor
+// versions. e.g. v2, v1, v1beta2, v1beta1, v1alpha1.
+type KubeVersions []string


KubeAwareVersion?

+1, KubeVersions instantly makes me think of KubernetesVersion.

Opted for KubeAwareAPIVersions to be extra clear :)

rudoi · 2020-02-14T21:55:05Z

/retest

This looks good to me, I think the capd e2e has just been having a bad time today.

wfernandes · 2020-02-14T22:50:04Z

util/conversion/conversion.go

 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 )

 const (
 	DataAnnotation = "cluster.x-k8s.io/conversion-data"
 )

+var (


Suggested change

var (

const (

It can't actually be assigned to a constant because the value comes from a function and can only be resolved at runtime

Signed-off-by: Vince Prignano <vincepri@vmware.com>

wfernandes · 2020-02-14T23:06:43Z

/lgtm

k8s-ci-robot · 2020-02-14T23:18:59Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-capd-e2e	`9ad583b`	link	`/test pull-cluster-api-capd-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.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels 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

k8s-ci-robot requested review from chuckha and fabriziopandini February 5, 2020 20:37

detiber reviewed Feb 5, 2020

View reviewed changes

vincepri force-pushed the update-refs-nicely branch from 3092565 to e1319b5 Compare February 5, 2020 23:54

detiber reviewed Feb 6, 2020

View reviewed changes

vincepri force-pushed the update-refs-nicely branch from e1319b5 to fd7d10c Compare February 6, 2020 03:26

fabriziopandini mentioned this pull request Feb 6, 2020

🏃clusterctl: cleanup inventory #2277

Merged

vincepri added this to the v0.3.0-rc.0 milestone Feb 12, 2020

vincepri mentioned this pull request Feb 12, 2020

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

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2020

vincepri force-pushed the update-refs-nicely branch from fd7d10c to 9247216 Compare February 14, 2020 19:21

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2020

vincepri changed the title ~~[WIP] 🏃 Add support to auto-update external references~~ ⚠️ Add support to auto-update external references Feb 14, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2020

vincepri force-pushed the update-refs-nicely branch from 9247216 to 6161888 Compare February 14, 2020 19:26

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 Feb 14, 2020

vincepri force-pushed the update-refs-nicely branch from 6161888 to aa14269 Compare February 14, 2020 19:38

vincepri force-pushed the update-refs-nicely branch 3 times, most recently from 203a7a5 to 7cabe4b Compare February 14, 2020 20:00

cpanato reviewed Feb 14, 2020

View reviewed changes

controllers/machine_controller_phases.go Outdated Show resolved Hide resolved

vincepri force-pushed the update-refs-nicely branch from 7cabe4b to 739755c Compare February 14, 2020 20:41

fabriziopandini approved these changes Feb 14, 2020

View reviewed changes

vincepri force-pushed the update-refs-nicely branch 2 times, most recently from 22b8f14 to b3ea5b0 Compare February 14, 2020 21:34

vincepri force-pushed the update-refs-nicely branch from b3ea5b0 to 90e3795 Compare February 14, 2020 22:39

wfernandes reviewed Feb 14, 2020

View reviewed changes

⚠️ Add support to auto-update external references

9ad583b

Signed-off-by: Vince Prignano <vincepri@vmware.com>

vincepri force-pushed the update-refs-nicely branch from 90e3795 to 9ad583b Compare February 14, 2020 23:06

k8s-ci-robot assigned wfernandes Feb 14, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2020

k8s-ci-robot merged commit 4fd4c7f into kubernetes-sigs:master Feb 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Add support to auto-update external references #2267

⚠️ Add support to auto-update external references #2267

vincepri commented Feb 5, 2020

vincepri commented Feb 5, 2020

k8s-ci-robot commented Feb 5, 2020

detiber Feb 5, 2020

detiber Feb 5, 2020

ncdc Feb 5, 2020

detiber Feb 6, 2020

vincepri Feb 6, 2020

detiber Feb 6, 2020

detiber Feb 6, 2020

vincepri Feb 6, 2020

vincepri commented Feb 6, 2020

rudoi commented Feb 14, 2020 •

edited

Loading

vincepri commented Feb 14, 2020

vincepri commented Feb 14, 2020

vincepri commented Feb 14, 2020

fabriziopandini left a comment

fabriziopandini Feb 14, 2020

rudoi Feb 14, 2020

vincepri Feb 14, 2020

rudoi commented Feb 14, 2020

wfernandes Feb 14, 2020

vincepri Feb 14, 2020

wfernandes commented Feb 14, 2020

k8s-ci-robot commented Feb 14, 2020

⚠️ Add support to auto-update external references #2267

⚠️ Add support to auto-update external references #2267

Conversation

vincepri commented Feb 5, 2020

vincepri commented Feb 5, 2020

k8s-ci-robot commented Feb 5, 2020

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

vincepri commented Feb 6, 2020

rudoi commented Feb 14, 2020 • edited Loading

vincepri commented Feb 14, 2020

vincepri commented Feb 14, 2020

vincepri commented Feb 14, 2020

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

rudoi commented Feb 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfernandes commented Feb 14, 2020

k8s-ci-robot commented Feb 14, 2020

rudoi commented Feb 14, 2020 •

edited

Loading