Cluster controller and types for v1alpha2 #1177

vincepri · 2019-07-19T17:00:16Z

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

What this PR does / why we need it:
This PR is an implementation following the proposal outlined in #1137

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

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-07-19T17:00:46Z

[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 · 2019-07-19T18:19:29Z

pkg/controller/cluster/cluster_controller.go

+// or if any resources linked to this cluster aren't yet deleted.
+func (r *ReconcileCluster) isDeleteReady(ctx context.Context, cluster *v1alpha2.Cluster) error {
+	// TODO(vincepri): List and delete MachineDeployments, MachineSets, Machines, and
+	// block deletion until they're all deleted.


We probably do not need to actively delete (just yet) and can likely just start with blocking deletion while any Machine* objects linked to this Cluster exist.

Would a user need to delete these before deleting the Cluster?

Yes, but technically that is no different than the state of the world today 😂

@detiber we'll need to forward-port #1180

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

pkg/apis/cluster/v1alpha2/cluster_types.go

pablochacin · 2019-07-20T08:59:21Z

pkg/apis/cluster/v1alpha2/cluster_types.go

-	// field, akin to component config.
-	// +optional
-	Value *runtime.RawExtension `json:"value,omitempty"`
+	InfrastructureRef *corev1.ObjectReference `json:"infrastructureRef,omitempty"`


Looks like this field is finally considered optional and not mandatory and set at creation time as it was discussed in the data model change proposal. I personally like this more.

The collaboration model between cluster controller and infrastructure controller was simplified and adapted to this assumption.

The initial proposal, however, considered it optional and possibly set after the cluster object was created.

For the sake of having a documentation for the infrastructure provider implementers, should we amend the proposal to reflect we are considering this field optional and how clusterphase Pending consider this case?

The Cluster is going to be in Pending phase if the infrastructure reference doesn't exist. I think that's fine for providers/users that don't want to use the Cluster for Infrastructure

This is one of the reasons I was thinking we didn't strictly need a cluster-wide phase at this time, given that we only have 1 boolean (infra ready, or not).

I don't mind either way, if we want to remove it we can do it later on or I can make the changes in this PR if needed

I think it's a good idea to keep it for consistency and it gives end users a better experience.

As I said in the data model proposal, I don't have any strong opinion about either option, even when I see value in having a consistent approach across controllers (machine, cluster). If we are keeping it, I would like to update the data model accordingly.

pkg/apis/cluster/v1alpha2/cluster_types.go

pkg/controller/cluster/cluster_controller.go

pkg/controller/cluster/cluster_controller_phases.go

pkg/controller/machinedeployment/machinedeployment_controller.go

pablochacin · 2019-07-20T10:14:24Z

@vincepri as we are removing the actuator interface, we should also remove the test actuator and ~~move this logic to~~ create a test infrastructure controller which sets the infrastructure status. Test must be changed accordingly (I think we don't need events anymore) otherwise integration tests won't pass.

Edit: I volunteer for helping with tests, if needed

pablochacin · 2019-07-21T10:59:34Z

@vincepri I think this PR also fixes https://github.com/kubernetes-sigs/cluster-api/issues/158

vincepri · 2019-07-22T17:52:08Z

/test pull-cluster-api-integration

vincepri · 2019-07-22T19:07:53Z

This should be ready for an in-depth review and serve as a starting point @detiber @pablochacin

vincepri · 2019-07-23T13:36:05Z

/assign @ncdc

cmd/clusterctl/validation/validate_cluster_api_objects.go

pkg/apis/cluster/common/pointers.go

pkg/apis/cluster/v1alpha2/cluster_types.go

pkg/controller/cluster/cluster_controller.go

ncdc · 2019-07-23T16:38:17Z

pkg/controller/cluster/cluster_controller_phases.go

+		return err
+	}
+
+	if cluster.Status.InfrastructureReady || !infraConfig.GetDeletionTimestamp().IsZero() {


Should we split this and move the infra ready check up above the call to r.reconcileExternal?

This looks fine to me, if we return early above then we don't reconcile the errors. I think the Machine reconcileBootstrap method needs to be fixed as well

What flow are you thinking about? Cluster infraReady=true, but something possibly has changed to cause reconcileExternal to fail?

pkg/controller/cluster/cluster_controller_phases.go

pkg/apis/cluster/v1alpha2/cluster_phase_types.go

ncdc · 2019-07-24T16:28:42Z

pkg/controller/cluster/cluster_controller.go

+
+	if cluster.Spec.InfrastructureRef != nil {
+		_, err := external.Get(r.Client, cluster.Spec.InfrastructureRef, cluster.Namespace)
+		if err != nil && !apierrors.IsNotFound(err) {


If you wanted, this might look a little cleaner:

switch { case apierrors.IsNotFound(err): // This is what we want - no need to requeue case err != nil: return errors.Wrapf(...) default: // or case err == nil return requeue }

ncdc · 2019-07-24T16:31:51Z

pkg/controller/cluster/cluster_controller.go

+		gvk := cluster.GroupVersionKind()
+		if err := r.Client.Patch(ctx, cluster, patchCluster); err != nil {
+			klog.Errorf("Error Patching Cluster %q in namespace %q: %v", cluster.Name, cluster.Namespace, err)
+			if reterr != nil {


I don't think reterr can ever be non-nil here?

This is flipped, should have been == nil
proceeds to drink more coffee

pkg/controller/cluster/cluster_controller.go

ncdc · 2019-07-24T16:32:33Z

pkg/controller/cluster/cluster_controller.go

+		cluster.SetGroupVersionKind(gvk)
+		if err := r.Client.Status().Patch(ctx, cluster, patchCluster); err != nil {
+			klog.Errorf("Error Patching Cluster status %q in namespace %q: %v", cluster.Name, cluster.Namespace, err)
+			if reterr != nil {


If you return early if the main patch failed, then if we made it this far, reterr should still be nil, right?

pkg/controller/cluster/cluster_controller_phases.go

ncdc · 2019-07-24T16:48:57Z

pkg/controller/cluster/cluster_controller_phases.go

+		return err
+	}
+
+	if cluster.Status.InfrastructureReady || !infraConfig.GetDeletionTimestamp().IsZero() {


What flow are you thinking about? Cluster infraReady=true, but something possibly has changed to cause reconcileExternal to fail?

moshloop · 2019-07-24T17:09:42Z

pkg/apis/cluster/v1alpha2/cluster_types.go

@@ -106,29 +90,52 @@ type NetworkRanges struct {
 type ClusterStatus struct {
 	// APIEndpoint represents the endpoint to communicate with the IP.
 	// +optional
-	APIEndpoints []APIEndpoint `json:"apiEndpoints,omitempty"`
+	APIEndpoint APIEndpoint `json:"apiEndpoint,omitempty"`


Why move from multiple to single endpoints?

For providers that don't have native load-balancers this would be the ideal place to retrieve a list of endpoints to load balance across (via client-side load balancing for example).

This has been discussed in #1137 and the ultimate goal was to reduce friction.

/cc @detiber

I don't think a change in the data model was in scope for #1137 (outside of externalizing it), nor was it highlighted in the PR summary or proposal body - This should have been a separate PR and elicited more discussion and/or highlighted as a change in the proposal body.

Can you open an issue to follow-up? Or PR to the current proposal? The change is small enough to be a follow-up to this PR

vincepri · 2019-07-24T20:23:02Z

@ncdc Good to merge?

ncdc · 2019-07-24T21:04:05Z

@vincepri I won't likely have any more review time today. Feel free to ask others or I can come back to it tomorrow.

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

ncdc · 2019-07-25T17:01:27Z

/lgtm

Can do additional fixes in follow-ups as needed.

vincepri · 2019-07-25T17:01:55Z

/hold cancel

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 19, 2019

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

k8s-ci-robot requested review from justinsb and ncdc July 19, 2019 17:00

vincepri force-pushed the cluster-v1a2 branch 2 times, most recently from b7cbb5d to 10ed36f Compare July 19, 2019 17:25

detiber reviewed Jul 19, 2019

View reviewed changes

vincepri force-pushed the cluster-v1a2 branch from 10ed36f to 5b1a6a4 Compare July 19, 2019 20:42

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

vincepri force-pushed the cluster-v1a2 branch from 5b1a6a4 to d6495d9 Compare July 19, 2019 20:47

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 19, 2019

vincepri changed the title ~~WIP: Cluster controller and types for v1alpha2~~ Cluster controller and types for v1alpha2 Jul 19, 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 Jul 19, 2019

pablochacin suggested changes Jul 20, 2019

View reviewed changes

ncdc mentioned this pull request Jul 22, 2019

Externalize provider specific specs and status in separated CRDs #833

Closed

vincepri force-pushed the cluster-v1a2 branch from d6495d9 to f72f412 Compare July 22, 2019 17:41

vincepri force-pushed the cluster-v1a2 branch from f72f412 to cf234c1 Compare July 22, 2019 19:05

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 22, 2019

vincepri force-pushed the cluster-v1a2 branch from cf234c1 to de48d24 Compare July 22, 2019 19:12

vincepri force-pushed the cluster-v1a2 branch 2 times, most recently from bdde5f6 to 7180218 Compare July 23, 2019 16:39

ncdc reviewed Jul 23, 2019

View reviewed changes

vincepri force-pushed the cluster-v1a2 branch 6 times, most recently from 84eefa4 to 95df206 Compare July 23, 2019 18:46

ncdc mentioned this pull request Jul 23, 2019

Make cluster.status.phase and machine.status.phase non-pointers #1188

Closed

vincepri force-pushed the cluster-v1a2 branch from 95df206 to 6b9a38b Compare July 24, 2019 16:29

ncdc reviewed Jul 24, 2019

View reviewed changes

vincepri force-pushed the cluster-v1a2 branch from 6b9a38b to 894a68c Compare July 24, 2019 16:56

moshloop reviewed Jul 24, 2019

View reviewed changes

vincepri force-pushed the cluster-v1a2 branch 5 times, most recently from e8948d5 to 4af32da Compare July 24, 2019 18:52

k8s-ci-robot requested a review from detiber July 24, 2019 18:56

vincepri added 2 commits July 25, 2019 08:25

Update vendor dependencies

447fb63

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

Cluster controller and types for v1alpha2

11c3ac2

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

vincepri force-pushed the cluster-v1a2 branch from 4af32da to 11c3ac2 Compare July 25, 2019 15:28

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 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 Jul 25, 2019

k8s-ci-robot merged commit 835ee87 into kubernetes-sigs:master Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster controller and types for v1alpha2 #1177

Cluster controller and types for v1alpha2 #1177

vincepri commented Jul 19, 2019 •

edited by ncdc

Loading

k8s-ci-robot commented Jul 19, 2019

detiber Jul 19, 2019

vincepri Jul 19, 2019

detiber Jul 19, 2019

ncdc Jul 24, 2019

pablochacin Jul 20, 2019

vincepri Jul 22, 2019

ncdc Jul 22, 2019

vincepri Jul 22, 2019

detiber Jul 22, 2019

pablochacin Jul 23, 2019

pablochacin commented Jul 20, 2019 •

edited

Loading

pablochacin commented Jul 21, 2019

vincepri commented Jul 22, 2019 •

edited

Loading

vincepri commented Jul 22, 2019

vincepri commented Jul 23, 2019

ncdc Jul 23, 2019

vincepri Jul 23, 2019

ncdc Jul 24, 2019

ncdc Jul 24, 2019

ncdc Jul 24, 2019

vincepri Jul 24, 2019

ncdc Jul 24, 2019

ncdc Jul 24, 2019

moshloop Jul 24, 2019

vincepri Jul 24, 2019

moshloop Jul 24, 2019

vincepri Jul 24, 2019

vincepri commented Jul 24, 2019

ncdc commented Jul 24, 2019

ncdc commented Jul 25, 2019

vincepri commented Jul 25, 2019

Cluster controller and types for v1alpha2 #1177

Cluster controller and types for v1alpha2 #1177

Conversation

vincepri commented Jul 19, 2019 • edited by ncdc Loading

k8s-ci-robot commented Jul 19, 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

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

pablochacin commented Jul 20, 2019 • edited Loading

pablochacin commented Jul 21, 2019

vincepri commented Jul 22, 2019 • edited Loading

vincepri commented Jul 22, 2019

vincepri commented Jul 23, 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

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

ncdc commented Jul 24, 2019

ncdc commented Jul 25, 2019

vincepri commented Jul 25, 2019

vincepri commented Jul 19, 2019 •

edited by ncdc

Loading

pablochacin commented Jul 20, 2019 •

edited

Loading

vincepri commented Jul 22, 2019 •

edited

Loading