Allow machine controller to function without the need of a cluster object #644

maisem · 2018-12-18T01:26:23Z

Release note:

The machine controller can now call the machine actuators with a nil cluster object.
Provider specific actuators should be updated to handle that case.
The primary motivation is to have allow machine reconcilation in the absence of cluster objects.

davidewatson · 2018-12-18T02:21:06Z

The alternative is to integrate with the Cluster Registry. We would need a spec and status...

maisem · 2018-12-18T04:28:54Z

Not sure I understand how integrating with the cluster registry would work in this context. Can you please explain?

The idea here is to push the responsibility of checking whether the cluster object is nil to the provider specific machine actuator. This allows the machine actuator to take any actions necessary without being completely blocked.

davidewatson · 2018-12-18T18:47:40Z

Fair enough. Please ignore my comment. Cluster status is an orthogonal issue.

Speaking with folks at KubeCon there multiple implementations which do not rely on the Cluster CRD/Controller. And without this change, if a user deletes the Cluster before the Machines they cannot delete the Machines, which results in a poor user experience.

davidewatson · 2018-12-18T19:12:58Z

/lgtm

roberthbailey · 2018-12-18T21:07:00Z

Does this change mean that actuators need to properly handle a nil cluster (which was not previously the case)? I think that should be noted as a release note (since the pre-1.0 releases are typically consumed by providers, not by end users).

maisem · 2018-12-18T23:00:52Z

Does this change mean that actuators need to properly handle a nil cluster (which was not previously the case)? I think that should be noted as a release note (since the pre-1.0 releases are typically consumed by providers, not by end users).

Added a release note.

justinsb · 2018-12-18T23:25:32Z

pkg/controller/machine/controller.go

-	}
-
-	// TODO: Assume single master for now.
-	// TODO: Assume we never change the role for the machines. (Master->Node, Node->Master, etc)


We've lost these comments, but I agree I don't really see why they belong here

justinsb · 2018-12-18T23:25:57Z

pkg/controller/machine/controller.go

-
+	cluster, err := r.getCluster(ctx, m)
+	if err != nil {
+		klog.Warningf("Cluster not found, machine creation might fail: %v", err)


This message is going to get annoying for providers that don't need a cluster!

Maybe the provider should be the one to determine what a nil Cluster reference means for their implementation. Also, at present, it is not just Create() that will fail. Update() and Delete() will too...

Increased the verbosity of the log message.

justinsb · 2018-12-18T23:26:23Z

pkg/controller/machine/controller.go

@@ -185,35 +183,6 @@ func (r *ReconcileMachine) Reconcile(request reconcile.Request) (reconcile.Resul
 	return reconcile.Result{}, nil
 }

-func (c *ReconcileMachine) create(ctx context.Context, machine *clusterv1.Machine) error {
-	cluster, err := c.getCluster(ctx, machine)


Nice cleanup!

dlipovetsky · 2018-12-19T21:47:37Z

This PR was discussed in the meeting.

dlipovetsky · 2018-12-20T00:03:42Z

An alternative to this change is for Providers to create/maintain the Cluster CRD even if they do not use the Cluster object. This would be an ongoing burden to some Providers, as opposed a one-time change to code that all Providers have to make to handle nil Cluster objects.

Even if this change lands, we still have the option of evaluating the Machine actuator interface in the future.

If there are no objections from Providers, I think this is ok.

roberthbailey · 2018-12-20T04:19:52Z

I'd prefer not to force providers to create / maintain a dummy cluster object just to keep the code the same. There was also the point above about deletion order, and I think most providers should be able to handle the delete flow for a machine if the cluster reference is nil.

vincepri

+1 from me.

I think the change here is pretty small and allows more providers to operate without a Cluster object. For long term solutions we should consolidate ideas and use cases in #639.

roberthbailey · 2018-12-20T20:32:26Z

/assign @sidharthsurana

I think you had the most objections during the meeting yesterday.

sidharthsurana · 2019-01-16T22:00:12Z

@roberthbailey Thanks a lot for providing me a chance to respond here :)
Looking at this change per say, I am okay with it. However, going over the past conversation involving this PR, I believe my main objection was around the fact that if we make cluster optional, what happens to the information that is indeed needed from the cluster object. For e.g. in the vsphere provider implementation the vCenter server URL and credentials are part of the cluster definition. Thus if the cluster object is missing, then the current vsphere implementation won't work. During the previous meeting the idea of defining a new object that would concretely hold all the common cluster level information (connection info, cloud provider configuration, etc.) was also brought up which I supported. Once something like this exists then the cluster object can truly be optional.

roberthbailey · 2019-01-16T22:03:35Z

What we discussed today was that making it optional makes it possible for implementations that don't need a cluster object (such as existing systems like kops) to adopt machines without needing to create a fake cluster object just to pass into the actuator to make it work. I agree that for many of the existing implementations, create won't work if nil is passed in.

vikaschoudhary16 · 2019-01-17T04:23:20Z

+1 for this change, Here i have started making cluster object optional in aws actuator, though WIP at the moment. I am testing these changes. Meanwhile early comments are welcome.

…ject.

roberthbailey · 2019-01-17T17:21:57Z

/test pull-cluster-api-test

roberthbailey · 2019-01-17T17:22:02Z

/lgtm
/approve

k8s-ci-robot · 2019-01-17T17:22:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maisem, roberthbailey

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~~ [roberthbailey]

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

roberthbailey · 2019-01-17T17:22:29Z

I think we have general consensus that we should merge this.

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 Dec 18, 2018

k8s-ci-robot requested review from krisnova and roberthbailey December 18, 2018 01:26

enxebre mentioned this pull request Dec 18, 2018

Machine api #645

Closed

k8s-ci-robot assigned davidewatson Dec 18, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2018

davidewatson mentioned this pull request Dec 18, 2018

Revisit actuators #639

Closed

justinsb reviewed Dec 18, 2018

View reviewed changes

vincepri approved these changes Dec 20, 2018

View reviewed changes

maisem force-pushed the c2 branch from 90cd3d0 to 82bc30b Compare January 17, 2019 16:13

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

Allow machine controller to function without the need of a cluster ob…

53b04a9

…ject.

maisem force-pushed the c2 branch from 82bc30b to 53b04a9 Compare January 17, 2019 16:15

k8s-ci-robot assigned roberthbailey Jan 17, 2019

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

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

k8s-ci-robot merged commit 302479e into kubernetes-sigs:master Jan 17, 2019

vikaschoudhary16 mentioned this pull request Jan 21, 2019

API changes for making cluster object optional kubernetes-sigs/cluster-api-provider-aws#508

Closed

enxebre mentioned this pull request Feb 6, 2019

Stronger link between Machine* <-> Cluster #728

Merged

Allow machine controller to function without the need of a cluster object #644

Allow machine controller to function without the need of a cluster object #644

Uh oh!

Conversation

maisem commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidewatson commented Dec 18, 2018

Uh oh!

maisem commented Dec 18, 2018

Uh oh!

davidewatson commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidewatson commented Dec 18, 2018

Uh oh!

roberthbailey commented Dec 18, 2018

Uh oh!

maisem commented Dec 18, 2018

Uh oh!

justinsb Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

justinsb Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

davidewatson Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maisem Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

justinsb Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

dlipovetsky commented Dec 19, 2018

Uh oh!

dlipovetsky commented Dec 20, 2018

Uh oh!

roberthbailey commented Dec 20, 2018

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

roberthbailey commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sidharthsurana commented Jan 16, 2019

Uh oh!

roberthbailey commented Jan 16, 2019

Uh oh!

vikaschoudhary16 commented Jan 17, 2019

Uh oh!

roberthbailey commented Jan 17, 2019

Uh oh!

roberthbailey commented Jan 17, 2019

Uh oh!

k8s-ci-robot commented Jan 17, 2019

Uh oh!

roberthbailey commented Jan 17, 2019

Uh oh!

Uh oh!

maisem commented Dec 18, 2018 •

edited

Loading

davidewatson commented Dec 18, 2018 •

edited

Loading

davidewatson Dec 20, 2018 •

edited

Loading

roberthbailey commented Dec 20, 2018 •

edited

Loading