Use remote NodeRef in Machine and MachineSet controllers #1052

vincepri · 2019-06-20T17:38:01Z

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

What this PR does / why we need it:
This PR is a follow-up to #1011 and allows the Machine and MachineSet controllers to use the remote client to access and retrieve a Node.

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 #520
Fixes #930
Fixes #1055

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

k8s-ci-robot · 2019-06-20T17:38:09Z

[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

vincepri · 2019-06-20T17:39:18Z

/assign @detiber

vincepri · 2019-06-20T18:00:10Z

/test pull-cluster-api-test

detiber · 2019-06-20T18:35:39Z

lgtm, but would like to get some input from @davidewatson if he has a chance.

vincepri · 2019-06-20T18:55:41Z

/assign @davidewatson

chuckha

some questions

chuckha · 2019-06-21T15:10:34Z

pkg/controller/machine/machine_controller.go

-			klog.V(2).Infof("Node %q not found", name)
-			return nil
+func (r *ReconcileMachine) deleteNode(ctx context.Context, cluster *clusterv1.Cluster, name string) error {
+	if cluster == nil {


ah this was a little bit confusing to read through. could consider a comment here because a nil cluster clearly means something special is happening, but I don't know why a nil cluster implies the management cluster is the workload cluster

Added some comments, in case Cluster is specified, we look for the KubeConfig secret

chuckha · 2019-06-21T15:13:07Z

pkg/controller/machine/machine_controller.go

+				return nil
+			}
+			klog.Errorf("Failed to get node %q: %v", name, err)
+			return err


This seems like it could be return nil / won't retry error. But I'm not sure, what was the logic behind this being an actual error?

Removed this part, IsNotFound is checked from the caller

chuckha · 2019-06-21T15:14:21Z

pkg/controller/machine/machine_controller.go

+		return nil
+	}
+
+	return corev1Remote.Nodes().Delete(name, &metav1.DeleteOptions{})


This has potential to loop forever if the node doesn't exist already. This should also error check that if the object can't be found then it's a success, not a retryable error. Unless .Delete does this automatically...:rocket:

The caller does the check at the moment, !apierrors.IsNotFound(err). Is this what you were referring to?

yep! thanks

chuckha

/lgtm
/hold

lgtm! but I think we're blocking on @davidewatson ? Feel free to remove the hold when you get a chance to review

ncdc · 2019-06-21T14:05:44Z

pkg/controller/machine/machine_controller.go

@@ -253,6 +254,9 @@ func (r *ReconcileMachine) getCluster(ctx context.Context, machine *clusterv1.Ma
 	return cluster, nil
 }

+// isDeletedAllowed returns false if the Machine we're trying to delete is the
+// Machine hosting this controller. It only works when the controllers are


Could you please clarify what "it only works" is referring to?

ncdc · 2019-06-21T14:05:57Z

pkg/controller/machine/machine_controller.go

@@ -253,6 +254,9 @@ func (r *ReconcileMachine) getCluster(ctx context.Context, machine *clusterv1.Ma
 	return cluster, nil
 }

+// isDeletedAllowed returns false if the Machine we're trying to delete is the
+// Machine hosting this controller. It only works when the controllers are
+// running in the workload cluster and does not support remote references.


What does not support remote references?

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

vincepri · 2019-06-21T20:43:51Z

@ncdc Reworded the comment, ptal

vincepri · 2019-06-21T20:53:51Z

/test pull-cluster-api-test

davidewatson · 2019-06-21T21:16:48Z

/lgtm

@vincepri: Are there release notes for this feature in one of the supporting PRs? I couldn’t find any...

vincepri · 2019-06-21T21:23:55Z

@davidewatson I'll follow up next week with some documentation, as well as making sure folks are aware of the changes in the next community meeting

vincepri · 2019-06-25T15:32:34Z

/hold cancel

…sigs#1052) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 1eaf864)

* Remove duplicate checks for deletionTimestamp (#987) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit aacb0c6) * Rename pkg/controller/controller (#998) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit eafafe8) * Add remote/util.go helpers to work with KubeConfig Secrets (#1004) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit d48025d) * Add remote.ClusterClient to access remote workload clusters (#1006) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 0b25546) * Add events to MachineSet operations (#1012) Add events to the following MachineSet operations - Adopt Machine - Create Machine - Delete Machine (cherry picked from commit b0170a0) * Add patch to events rbac (#1021) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 7365154) * Update some bazel dependencies (#1019) (cherry picked from commit 77836d8) * Update boilerplate check and generated files (#1010) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 0b215f4) * Add CLUSTER_API_KUBECONFIG_READY_TIMEOUT to clusterctl (#1026) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 9cd85f7) * Add events to MachineDeployment operations (#1014) (cherry picked from commit a5e5fdb) * Update controller-runtime and controller-tools (#1032) (cherry picked from commit da61280) * Support for wrapped RequeueAfterError/interface (#1020) This patch adds support for wrapped "RequeueAfterError" errors as well as a new interface for supporting custom "RequeueAfterError" errors. (cherry picked from commit bd0628f) * Use klog in manager and setup controller-runtime logger (#1022) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 0ebf076) * Bazel updates (#1043) - Remove use of deprecated bazel attribute label single_file - Add bazel version checking - Include go_rules fix for cross-compiling darwin binaries (cherry picked from commit 2123eed) * clean up shell scripts in cluster-api (#1045) (cherry picked from commit d63d4be) * Update k8s/code-generator to latest release-1.13 (#1048) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit c2faf86) * downgrade log of util.ExecuteCommand (#975) As this error E0531 is not that ueful and no need to be mark this as 'E', use Info level should be fine as this won't affect return value. (cherry picked from commit 97f25c9) * Make ClusterNetwork ClusterNetworkingConfig field optional within (#919) Cluster resources. This is useful for Managed Control Planes and Bare Metal where networking configuration options may be limited. (cherry picked from commit 9e1eb56) * NodeRef controller (#1011) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 9e89546) * Update generated files, add workaround for controller-tools (#1050) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 1c10a3f) * Update controller-tools to 0.1.11 (#1053) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 1cb3deb) * Update dep check (#1056) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit aecec29) * Fix register bootstrap options in alpha phases (#1064) Fixes issue: #1063 (cherry picked from commit 8141304) * Use remote NodeRef in Machine and MachineSet controllers (#1052) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 1eaf864) * Bazel fixes Signed-off-by: Andy Goldstein <goldsteina@vmware.com>

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2019

k8s-ci-robot requested review from detiber and justinsb June 20, 2019 17:38

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 20, 2019

k8s-ci-robot assigned detiber Jun 20, 2019

vincepri force-pushed the use-noderef-remote branch from 602a871 to f41b137 Compare June 20, 2019 17:46

k8s-ci-robot assigned davidewatson Jun 20, 2019

ncdc mentioned this pull request Jun 21, 2019

node deletion can't delete node #1055

Closed

chuckha reviewed Jun 21, 2019

View reviewed changes

vincepri force-pushed the use-noderef-remote branch from f41b137 to 630cec2 Compare June 21, 2019 15:24

chuckha approved these changes Jun 21, 2019

View reviewed changes

k8s-ci-robot assigned chuckha Jun 21, 2019

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 21, 2019

vincepri mentioned this pull request Jun 21, 2019

Define health check strategy for MachineSet #47

Closed

ncdc reviewed Jun 21, 2019

View reviewed changes

Use remote NodeRef in Machine and MachineSet controllers

d65989a

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

vincepri force-pushed the use-noderef-remote branch from 630cec2 to d65989a Compare June 21, 2019 20:43

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2019

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

k8s-ci-robot merged commit 1eaf864 into kubernetes-sigs:master Jun 25, 2019

ncdc pushed a commit to ncdc/cluster-api that referenced this pull request Jun 25, 2019

Use remote NodeRef in Machine and MachineSet controllers (kubernetes-…

22eb26b

…sigs#1052) Signed-off-by: Vince Prignano <vincepri@vmware.com> (cherry picked from commit 1eaf864)

ncdc mentioned this pull request Jun 25, 2019

0.1 backports for noderef #1069

Merged

This was referenced Jun 26, 2019

Add noderef controller documentation to gitbook #1066

Merged

Remove NodeConditions from Machine.Status #1081

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use remote NodeRef in Machine and MachineSet controllers #1052

Use remote NodeRef in Machine and MachineSet controllers #1052

vincepri commented Jun 20, 2019 •

edited by ncdc

Loading

k8s-ci-robot commented Jun 20, 2019

vincepri commented Jun 20, 2019

vincepri commented Jun 20, 2019

detiber commented Jun 20, 2019

vincepri commented Jun 20, 2019

chuckha left a comment

chuckha Jun 21, 2019

vincepri Jun 21, 2019

chuckha Jun 21, 2019

vincepri Jun 21, 2019

chuckha Jun 21, 2019

vincepri Jun 21, 2019

chuckha Jun 21, 2019

chuckha left a comment

ncdc Jun 21, 2019

ncdc Jun 21, 2019

vincepri commented Jun 21, 2019

vincepri commented Jun 21, 2019

davidewatson commented Jun 21, 2019 •

edited

Loading

vincepri commented Jun 21, 2019

vincepri commented Jun 25, 2019

Use remote NodeRef in Machine and MachineSet controllers #1052

Use remote NodeRef in Machine and MachineSet controllers #1052

Conversation

vincepri commented Jun 20, 2019 • edited by ncdc Loading

k8s-ci-robot commented Jun 20, 2019

vincepri commented Jun 20, 2019

vincepri commented Jun 20, 2019

detiber commented Jun 20, 2019

vincepri commented Jun 20, 2019

chuckha 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Jun 21, 2019

vincepri commented Jun 21, 2019

davidewatson commented Jun 21, 2019 • edited Loading

vincepri commented Jun 21, 2019

vincepri commented Jun 25, 2019

vincepri commented Jun 20, 2019 •

edited by ncdc

Loading

davidewatson commented Jun 21, 2019 •

edited

Loading