Externalize provider specific specs and status in separated CRDs #1137

pablochacin · 2019-07-09T18:49:54Z

Signed-off-by: Pablo Chacin (@pablochacin)

What this PR does / why we need it:

This proposal outlines the replacement of the provider-specific fields embedded in the Cluster object by references to objects managed by a provider controller, effectively eliminating the actuator interface. The new objects are introduced and the cooperation between the Cluster controller and the Cluster Infrastructure Controller is outlined. This process introduces a new field InfrastructureState which reflects the state of the provisioning process.

Fixes #833

Special notes for your reviewer:

07/14/20109: First set of review comments addressed.

InfrastructureRef is a top level element of ClusterSpec and InfrastructureStatusRef was removed
InfrastructureRef is mandatory and is expected to be set when the Cluster object is created
State model simplify with a boolean field indicating if the infrastructure has been provisioned or not.

Note: changes submitted in two commits to allow reverting the change in the transition model if found it is not satisfactory.

07/18/2019: Replace ascii-art sequence diagrams

Release note:

Signed-off-by: Pablo Chacin <[email protected]>

k8s-ci-robot · 2019-07-09T18:50:08Z

Hi @pablochacin. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes 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-07-09T18:53:55Z

/ok-to-test

Thanks @pablochacin. We'll take a look at this soon!

serbrech · 2019-07-09T18:58:38Z

I like this, it breaks up dependencies and exposes a clearer contract between the moving pieces.

ncdc

This is my first pass through the proposal. I will likely have additional comments, but this is what jumped at out me. And I wanted to provide this feedback now because I'm in meetings the rest of the day.

docs/proposals/20190709-cluster-spec-crds.md

ncdc · 2019-07-09T19:20:27Z

docs/proposals/20190709-cluster-spec-crds.md

+
+When the `InfrastructureRef` is set in the cluster, the cluster controller will retrieve the infrastructure object. If the object has not been seen before, it will start watching it. Also, if the object's owner is not set, it will set to the Cluster object and will set the cluster's `InfrastructureState` to "Provisioning". 
+
+When an infrastructure object is updated, the provider controller will check the owner reference. If it is set, it will retrieve the cluster object to obtain the required cluster specification and starts the provisioning process. When the process finishes, it sets the `InfrastructureStatusRef` in the cluster object. When the cluster controller detects the `InfrastructureStatusRef` is set, it sets the status to "Provisioned". 


The way we determine the value of Machine.Status.InfrastructureReady is by retrieving the object referenced by Machine.Spec.InfrastructureRef and evaluating a required boolean field .Status.Ready. I recommend doing the same thing here for consistency. This is also the responsibility of the cluster controller in cluster-api; it should not be done by a provider controller.

docs/proposals/20190709-cluster-spec-crds.md

vincepri · 2019-07-09T18:57:09Z

docs/proposals/20190709-cluster-spec-crds.md

+
+The sequence diagram below describes the hight-level process, collaborations and state transitions.  
+
+```


Can we use the same diagrams @dlipovetsky contributed to the other proposal?

+1 to using plantUML here, it will help with updating/modifying if necessary.

I'll update the proposal with the diagrams ASAP.

That said, is sad to realize ascii art is no longer appreciated ... like vinyl discs ... 😢

lol, I appreciate the ascii art, I just don't want to have to attempt to update said ascii art 😂

docs/proposals/20190709-cluster-spec-crds.md

vincepri · 2019-07-09T19:27:33Z

docs/proposals/20190709-cluster-spec-crds.md

+
+##### Transition Conditions
+
+- The Cluster object has the `InfrastructureRef` field set 


I would expect the InfrastructureRef to be set at creation time, I'm curious to know how this affects the Phases

Assuming the reference is always set when the cluster is created will make the pending phase unnecessary.

The current phases consider the possibility that the two objects, cluster and infrastructure are not created in a given order. It particularly opens the possibility of some tool watching for new clusters an creating the infrastructure for the cluster automatically.

@vincepri I know there are some providers today that don't use the Cluster resource. If we can get them to switch to using it, primarily as a means to link related machines together, maybe we need to keep InfrastructureRef optional? Otherwise, they'd have to create a dummy reference to something...

If it is too difficult to make the InfrastructureRef optional, maybe we can provide a Noop Infrastructure provider as part of cluster-api

As I mentioned here #1137 (comment), I'd prefer InfrastructureRef to be required like it is for Machines. If folks don't want to use the Cluster object, they can already do so today by not specifying a Cluster label on Machines.

If you don't use a Cluster, you don't get machine.status.nodeRef, which means that machineSet.status is broken (the controller can't calculate ready/available/etc replicas). I'd like to suggest in a separate discussion that Cluster be required, but that's not up for debate as part of this proposal 😄.

vincepri · 2019-07-09T19:28:08Z

docs/proposals/20190709-cluster-spec-crds.md

+##### Transition Conditions
+
+- The Cluster object has the `InfrastructureRef` field set 
+- The Infrastructure object has its owner set to the Cluster


The Cluster Controller would set this reference for us

I'm not sure I understand this comment. The Cluster Controller handles the common cluster object (independent of the provider). Some other entity (the provider's version of clusterctl, maybe? is responsible for setting this ref.

This state transition doesn't assume the reference is set when the object is created in the cluster api.

FYI, there will no longer be provider-specific clusterctls going forward.

docs/proposals/20190709-cluster-spec-crds.md

detiber

Overall, this proposal looks like a great start. I'll echo some of the things that @ncdc and @vincepri mentioned around consolidating the Spec/Status into a single object and using a boolean field rather than InfrastructureState

docs/proposals/20190709-cluster-spec-crds.md

detiber · 2019-07-10T13:54:23Z

docs/proposals/20190709-cluster-spec-crds.md

+
+The sequence diagram below describes the hight-level process, collaborations and state transitions.  
+
+```


+1 to using plantUML here, it will help with updating/modifying if necessary.

docs/proposals/20190709-cluster-spec-crds.md

- Fix spelling, wrong references to "machine" instead of "cluster". - Make `InfrastructureRef` a top level element in `ClusterSpec` and make it mandatory - Remove `InfrastructureStatusRef` and introduce `Infrastructure.Status.Ready` field - Update collaboration sequence - Add step for copying status from Infrastructure to Cluster status to collabotation sequence - Complete state transition descriptions

Replace the InfrastructureState field by a boolean that reflects when the cluster infrastructure is ready or not. Adjust the collaboration diagram and state transition description accordingly.

docs/proposals/20190709-cluster-spec-crds.md

vincepri

This is looking good imo, we'll just need to fix the diagrams

docs/proposals/20190709-cluster-spec-crds.md

vincepri · 2019-07-18T17:10:31Z

/test pull-cluster-api-test

vincepri · 2019-07-18T17:24:24Z

If the ASCII diagrams are a blocker, I'd be ok merging this in now and opening an issue to fix them up later so we don't hold up the proposal longer.

docs/proposals/20190709-cluster-spec-crds.md

pablochacin · 2019-07-18T20:38:18Z

If the ASCII diagrams are a blocker, I'd be ok merging this in now and opening an issue to fix them up later so we don't hold up the proposal longer.

Finally replaced. Retro look is now gone 😞

vincepri

/lgtm
/approve
/hold

k8s-ci-robot · 2019-07-18T20:39:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pablochacin, 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-07-19T15:16:02Z

If there are no major comments today, I'd like to get this merged as we discussed during last community meeting 🙂

/cc @detiber @ncdc @justinsb @timothysc @davidewatson @dlipovetsky

vincepri · 2019-07-19T20:50:06Z

/hold cancel

WIP: Initial Proposal

0b141c0

Signed-off-by: Pablo Chacin <[email protected]>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 9, 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 9, 2019

k8s-ci-robot requested review from detiber and justinsb July 9, 2019 18:50

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 Jul 9, 2019

ncdc reviewed Jul 9, 2019

View reviewed changes

vincepri reviewed Jul 9, 2019

View reviewed changes

detiber reviewed Jul 10, 2019

View reviewed changes

pablochacin added 2 commits July 14, 2019 18:22

Update state transition model

64e5ae9

Replace the InfrastructureState field by a boolean that reflects when the cluster infrastructure is ready or not. Adjust the collaboration diagram and state transition description accordingly.

detiber reviewed Jul 15, 2019

View reviewed changes

docs/proposals/20190709-cluster-spec-crds.md Outdated Show resolved Hide resolved

docs/proposals/20190709-cluster-spec-crds.md Outdated Show resolved Hide resolved

vincepri reviewed Jul 15, 2019

View reviewed changes

docs/proposals/20190709-cluster-spec-crds.md Outdated Show resolved Hide resolved

docs/proposals/20190709-cluster-spec-crds.md Outdated Show resolved Hide resolved

Fix typos. Regenerate TOC

0b17541

ncdc reviewed Jul 16, 2019

View reviewed changes

docs/proposals/20190709-cluster-spec-crds.md Outdated Show resolved Hide resolved

Make explicit status fields copied

1259b0c

vincepri reviewed Jul 18, 2019

View reviewed changes

docs/proposals/20190709-cluster-spec-crds.md Show resolved Hide resolved

pablochacin added 2 commits July 18, 2019 22:23

Fix asciiart diagrams

10d81d8

Fix list of reviewers

31a03f2

vincepri approved these changes Jul 18, 2019

View reviewed changes

k8s-ci-robot assigned vincepri Jul 18, 2019

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

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

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

k8s-ci-robot requested review from davidewatson, detiber, dlipovetsky, ncdc and timothysc July 19, 2019 15:16

vincepri mentioned this pull request Jul 19, 2019

Cluster controller and types for v1alpha2 #1177

Merged

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

k8s-ci-robot merged commit 7303333 into kubernetes-sigs:master Jul 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Externalize provider specific specs and status in separated CRDs #1137

Externalize provider specific specs and status in separated CRDs #1137

pablochacin commented Jul 9, 2019 •

edited

Loading

k8s-ci-robot commented Jul 9, 2019

ncdc commented Jul 9, 2019

serbrech commented Jul 9, 2019

ncdc left a comment

ncdc Jul 9, 2019

vincepri Jul 9, 2019

detiber Jul 10, 2019

pablochacin Jul 10, 2019

detiber Jul 10, 2019

vincepri Jul 9, 2019

pablochacin Jul 10, 2019

ncdc Jul 10, 2019

detiber Jul 10, 2019

vincepri Jul 10, 2019 •

edited

Loading

ncdc Jul 10, 2019

vincepri Jul 9, 2019

pablochacin Jul 10, 2019

ncdc Jul 10, 2019

detiber left a comment

detiber Jul 10, 2019

vincepri left a comment

vincepri commented Jul 18, 2019

vincepri commented Jul 18, 2019

pablochacin commented Jul 18, 2019

vincepri left a comment

k8s-ci-robot commented Jul 18, 2019

vincepri commented Jul 19, 2019 •

edited

Loading

vincepri commented Jul 19, 2019


		When the `InfrastructureRef` is set in the cluster, the cluster controller will retrieve the infrastructure object. If the object has not been seen before, it will start watching it. Also, if the object's owner is not set, it will set to the Cluster object and will set the cluster's `InfrastructureState` to "Provisioning".

		When an infrastructure object is updated, the provider controller will check the owner reference. If it is set, it will retrieve the cluster object to obtain the required cluster specification and starts the provisioning process. When the process finishes, it sets the `InfrastructureStatusRef` in the cluster object. When the cluster controller detects the `InfrastructureStatusRef` is set, it sets the status to "Provisioned".


		The sequence diagram below describes the hight-level process, collaborations and state transitions.

		```


		##### Transition Conditions

		- The Cluster object has the `InfrastructureRef` field set

Externalize provider specific specs and status in separated CRDs #1137

Externalize provider specific specs and status in separated CRDs #1137

Conversation

pablochacin commented Jul 9, 2019 • edited Loading

k8s-ci-robot commented Jul 9, 2019

ncdc commented Jul 9, 2019

serbrech commented Jul 9, 2019

ncdc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri Jul 10, 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

detiber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

vincepri commented Jul 18, 2019

vincepri commented Jul 18, 2019

pablochacin commented Jul 18, 2019

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 18, 2019

vincepri commented Jul 19, 2019 • edited Loading

vincepri commented Jul 19, 2019

pablochacin commented Jul 9, 2019 •

edited

Loading

vincepri Jul 10, 2019 •

edited

Loading

vincepri commented Jul 19, 2019 •

edited

Loading