NodeRef controller #1011

vincepri · 2019-06-11T16:50:37Z

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:
This PR adds a new controller to set Machine.Status.NodeRef. The primary goal of this work is to support controllers running in a management cluster. The PR is based on parts of #997 proposal while being compatible with v1alpha1.

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

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-11T16:50:51Z

[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

pkg/controller/noderef/noderef_controller.go

vincepri · 2019-06-11T21:09:35Z

@ncdc ptal

timothysc · 2019-06-13T14:52:36Z

Definitely add a release note to this PR please.

vincepri

@detiber @ncdc ptal :), if things look good, I'll start working on some unit tests and then we should be ready to merge it

ncdc · 2019-06-13T17:18:36Z

Is there a doc we can update to indicate CAPI now has read access to all secrets across all namespaces (and why), along with guidance on how to scope it narrower?

pkg/controller/noderef/noderef_controller.go

enxebre · 2019-06-18T15:05:30Z

Could we please update the PR desc with the purpose of this PR?

vincepri · 2019-06-18T15:10:45Z

@enxebre Done, thanks for keeping me honest :)

vincepri · 2019-06-18T20:35:43Z

@detiber @ncdc ptal

pkg/controller/noderefutil/providerid.go

pkg/controller/noderef/noderef_controller.go

pkg/controller/noderef/noderef_controller_test.go

detiber · 2019-06-20T14:20:32Z

pkg/controller/noderef/noderef_controller.go

+	}
+
+	// Update Machine.
+	machine.Status.NodeRef = nodeRef


There are currently workflows related to the existing Node and Machine controllers that will attempt to perform actions when the nodeRef is set.

Node deletion:

cluster-api/pkg/controller/machine/machine_controller.go

Lines 182 to 188 in bd0628f

if m.Status.NodeRef != nil {

klog.Infof("Deleting node %q for machine %q", m.Status.NodeRef.Name, m.Name)

if err := r.deleteNode(ctx, m.Status.NodeRef.Name); err != nil {

klog.Errorf("Error deleting node %q for machine %q", name, err)

return reconcile.Result{}, err

}

}

isDeleteAllowed:

cluster-api/pkg/controller/machine/machine_controller.go

Lines 256 to 275 in bd0628f

func (r *ReconcileMachine) isDeleteAllowed(machine *clusterv1.Machine) bool {

if r.nodeName == "" || machine.Status.NodeRef == nil {

return true

}

if machine.Status.NodeRef.Name != r.nodeName {

return true

}

node := &corev1.Node{}

if err := r.Client.Get(context.Background(), client.ObjectKey{Name: r.nodeName}, node); err != nil {

klog.Infof("Failed to determine if controller's node %q is associated with machine %q: %v", r.nodeName, machine.Name, err)

return true

}

// When the UID of the machine's node reference and this controller's actual node match then then the request is to

// delete the machine this machine-controller is running on. Return false to not allow machine controller to delete its

// own machine.

return node.UID != machine.Status.NodeRef.UID

}

MachineSet status:

cluster-api/pkg/controller/machineset/status.go

Lines 58 to 63 in e325dfa

if noderefutil.IsNodeReady(node) {

readyReplicasCount++

if noderefutil.IsNodeAvailable(node, ms.Spec.MinReadySeconds, metav1.Now()) {

availableReplicasCount++

}

}

Have these been tested with these changes?

Those all fail as you’d expect. I’ll follow up with other PRs to tackle them separately

detiber · 2019-06-20T14:30:42Z

pkg/controller/noderefutil/providerid.go

+
+// Equals returns true if both the CloudProvider and ID match.
+func (p *ProviderID) Equals(o *ProviderID) bool {
+	return p.CloudProvider() == o.CloudProvider() && p.ID() == o.ID()


Wouldn't this result in aws://// being != aws:////?

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/legacy-cloud-providers/aws/instances.go#L48-L52

I think we disallow empty IDs for validation, but that should work if you only test Equals

oops, markdown killed my comparison... I meant aws:////i-xxxxxx being != aws:///us-east-1a/i-xxxxxx

ID() will only return the last part of the ProviderID. That comparison you mentioned should be tested here https://github.com/kubernetes-sigs/cluster-api/pull/1011/files#diff-a5350fd6ed4fabde029f67abad7b17a5R110

pkg/controller/noderefutil/providerid_test.go

Signed-off-by: Vince Prignano <[email protected]>

detiber · 2019-06-20T15:33:56Z

/test pull-cluster-api-test

detiber · 2019-06-20T16:01:45Z

/lgtm

Signed-off-by: Vince Prignano <[email protected]> (cherry picked from commit 9e89546)

* Remove duplicate checks for deletionTimestamp (#987) Signed-off-by: Vince Prignano <[email protected]> (cherry picked from commit aacb0c6) * Rename pkg/controller/controller (#998) Signed-off-by: Vince Prignano <[email protected]> (cherry picked from commit eafafe8) * Add remote/util.go helpers to work with KubeConfig Secrets (#1004) Signed-off-by: Vince Prignano <[email protected]> (cherry picked from commit d48025d) * Add remote.ClusterClient to access remote workload clusters (#1006) Signed-off-by: Vince Prignano <[email protected]> (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 <[email protected]> (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 <[email protected]> (cherry picked from commit 0b215f4) * Add CLUSTER_API_KUBECONFIG_READY_TIMEOUT to clusterctl (#1026) Signed-off-by: Vince Prignano <[email protected]> (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 <[email protected]> (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 <[email protected]> (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 <[email protected]> (cherry picked from commit 9e89546) * Update generated files, add workaround for controller-tools (#1050) Signed-off-by: Vince Prignano <[email protected]> (cherry picked from commit 1c10a3f) * Update controller-tools to 0.1.11 (#1053) Signed-off-by: Vince Prignano <[email protected]> (cherry picked from commit 1cb3deb) * Update dep check (#1056) Signed-off-by: Vince Prignano <[email protected]> (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 <[email protected]> (cherry picked from commit 1eaf864) * Bazel fixes Signed-off-by: Andy Goldstein <[email protected]>

enxebre · 2019-07-02T11:55:38Z

pkg/controller/noderef/noderef_controller.go

+			return nil, err
+		}
+
+		for _, node := range nodeList.Items {


In a follow up we could use an indexer here and filter the request by node.Spec.ProviderID

@enxebre if you'd like to see that happen, please file an issue. Otherwise, I fear this comment will got lost. Thanks!

sure, I'll also put a PR

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. labels Jun 11, 2019

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 11, 2019

k8s-ci-robot requested review from detiber and justinsb June 11, 2019 16:50

vincepri force-pushed the noderef-controller branch 3 times, most recently from 69a9991 to ba2230a Compare June 11, 2019 16:58

ncdc reviewed Jun 11, 2019

View reviewed changes

pkg/controller/noderef/noderef_controller.go Outdated Show resolved Hide resolved

ncdc reviewed Jun 11, 2019

View reviewed changes

pkg/controller/noderef/noderef_controller.go Show resolved Hide resolved

ncdc reviewed Jun 11, 2019

View reviewed changes

pkg/controller/noderef/noderef_controller.go Outdated Show resolved Hide resolved

vincepri force-pushed the noderef-controller branch from ba2230a to b17478c Compare June 11, 2019 21:00

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 Jun 11, 2019

vincepri force-pushed the noderef-controller branch from b17478c to 9153851 Compare June 11, 2019 21:01

vincepri force-pushed the noderef-controller branch 2 times, most recently from 78053dd to 4229b86 Compare June 12, 2019 02:43

timothysc added this to the v1alpha2 milestone Jun 12, 2019

vincepri force-pushed the noderef-controller branch 2 times, most recently from 25ddda8 to 38199c8 Compare June 12, 2019 23:54

timothysc mentioned this pull request Jun 13, 2019

Machine States & Preboot Bootstrapping #997

Merged

vincepri force-pushed the noderef-controller branch from 38199c8 to d2581ff Compare June 13, 2019 17:11

vincepri commented Jun 13, 2019

View reviewed changes

ncdc reviewed Jun 13, 2019

View reviewed changes

pkg/controller/noderef/noderef_controller.go Outdated Show resolved Hide resolved

vincepri force-pushed the noderef-controller branch from d2581ff to fe09ec0 Compare June 13, 2019 17:26

vincepri changed the title ~~WIP: NodeRef controller~~ NodeRef controller Jun 18, 2019

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 Jun 18, 2019

vincepri force-pushed the noderef-controller branch 2 times, most recently from 1cbe941 to 0f5cdf1 Compare June 18, 2019 14:50

vincepri force-pushed the noderef-controller branch 2 times, most recently from 4464292 to 152c1c5 Compare June 18, 2019 20:33

ncdc reviewed Jun 18, 2019

View reviewed changes

pkg/controller/noderefutil/providerid.go Outdated Show resolved Hide resolved

pkg/controller/noderef/noderef_controller.go Outdated Show resolved Hide resolved

pkg/controller/noderef/noderef_controller_test.go Outdated Show resolved Hide resolved

vincepri force-pushed the noderef-controller branch from 152c1c5 to 73e1d4f Compare June 18, 2019 23:18

This was referenced Jun 18, 2019

Deprecate node controller #1041

Closed

Bazel updates #1043

Merged

vincepri force-pushed the noderef-controller branch from 73e1d4f to a5b733a Compare June 19, 2019 17:32

detiber reviewed Jun 20, 2019

View reviewed changes

NodeRef controller

99c24e4

Signed-off-by: Vince Prignano <[email protected]>

vincepri force-pushed the noderef-controller branch from a5b733a to 99c24e4 Compare June 20, 2019 15:13

k8s-ci-robot assigned detiber Jun 20, 2019

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

k8s-ci-robot merged commit 9e89546 into kubernetes-sigs:master Jun 20, 2019

This was referenced Jun 20, 2019

Use remote NodeRef in Machine and MachineSet controllers #1052

Merged

Define health check strategy for MachineSet #47

Closed

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

NodeRef controller (kubernetes-sigs#1011)

7137b60

Signed-off-by: Vince Prignano <[email protected]> (cherry picked from commit 9e89546)

ncdc mentioned this pull request Jun 25, 2019

0.1 backports for noderef #1069

Merged

enxebre reviewed Jul 2, 2019

View reviewed changes

alexeldeib mentioned this pull request Jul 10, 2021

fix: provider ID should use full ID for comparison #4913

Closed

enxebre mentioned this pull request Apr 10, 2023

Allow provider id regex matching to support more provider formats #8485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NodeRef controller #1011

NodeRef controller #1011

vincepri commented Jun 11, 2019 •

edited

Loading

k8s-ci-robot commented Jun 11, 2019

vincepri commented Jun 11, 2019

timothysc commented Jun 13, 2019

vincepri left a comment

ncdc commented Jun 13, 2019

enxebre commented Jun 18, 2019

vincepri commented Jun 18, 2019

vincepri commented Jun 18, 2019

detiber Jun 20, 2019

vincepri Jun 20, 2019

detiber Jun 20, 2019

vincepri Jun 20, 2019

detiber Jun 20, 2019

vincepri Jun 20, 2019

detiber commented Jun 20, 2019

detiber commented Jun 20, 2019

enxebre Jul 2, 2019

ncdc Jul 2, 2019

enxebre Jul 2, 2019

	if m.Status.NodeRef != nil {
	klog.Infof("Deleting node %q for machine %q", m.Status.NodeRef.Name, m.Name)
	if err := r.deleteNode(ctx, m.Status.NodeRef.Name); err != nil {
	klog.Errorf("Error deleting node %q for machine %q", name, err)
	return reconcile.Result{}, err
	}
	}

	func (r ReconcileMachine) isDeleteAllowed(machine clusterv1.Machine) bool {
	if r.nodeName == "" \|\| machine.Status.NodeRef == nil {
	return true
	}

	if machine.Status.NodeRef.Name != r.nodeName {
	return true
	}

	node := &corev1.Node{}
	if err := r.Client.Get(context.Background(), client.ObjectKey{Name: r.nodeName}, node); err != nil {
	klog.Infof("Failed to determine if controller's node %q is associated with machine %q: %v", r.nodeName, machine.Name, err)
	return true
	}

	// When the UID of the machine's node reference and this controller's actual node match then then the request is to
	// delete the machine this machine-controller is running on. Return false to not allow machine controller to delete its
	// own machine.
	return node.UID != machine.Status.NodeRef.UID
	}

	if noderefutil.IsNodeReady(node) {
	readyReplicasCount++
	if noderefutil.IsNodeAvailable(node, ms.Spec.MinReadySeconds, metav1.Now()) {
	availableReplicasCount++
	}
	}

NodeRef controller #1011

NodeRef controller #1011

Conversation

vincepri commented Jun 11, 2019 • edited Loading

k8s-ci-robot commented Jun 11, 2019

vincepri commented Jun 11, 2019

timothysc commented Jun 13, 2019

vincepri left a comment

Choose a reason for hiding this comment

ncdc commented Jun 13, 2019

enxebre commented Jun 18, 2019

vincepri commented Jun 18, 2019

vincepri commented Jun 18, 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

detiber commented Jun 20, 2019

detiber commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Jun 11, 2019 •

edited

Loading