Machine api #645

enxebre · 2018-12-18T09:35:01Z

What this PR does / why we need it:
Decouple the machine API from the Cluster API group so they can be promoted at separate cadence.

This will help to solidify boundaries between "cluster" and "machines" so one API is not necessarily holding back the other one, hence this will favour adoption for use cases where a consumer wants to leverage solely one of the APIs. The most common use case is where a user wants to leverage the machine API over a bespoke "cluster" deployment

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to:
#490
#497
#639
#644

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 · 2018-12-18T09:35:16Z

Hi @enxebre. 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.

enxebre · 2018-12-18T09:37:18Z

ping @roberthbailey @davidewatson @derekwaynecarr @hardikdr @detiber @justinsb

frobware

Overall LGTM and really nice to see this being split up.

Not sure that machine/common as a package makes sense anymore.

Isn't everything in machine/common only related to machines? If so, can this not be moved up into machine?

'common' as a package name doesn't really convey much.

frobware · 2018-12-18T09:55:07Z

cmd/clusterctl/validation/validate_cluster_api_objects_test.go

@@ -42,16 +45,16 @@ func newClusterStatus(errorReason common.ClusterStatusError, errorMessage string
 	}
 }

-func newMachineStatus(nodeRef *v1.ObjectReference, errorReason *common.MachineStatusError, errorMessage *string) v1alpha1.MachineStatus {
-	return v1alpha1.MachineStatus{
+func newMachineStatus(nodeRef *v1.ObjectReference, errorReason *machinecommon.MachineStatusError, errorMessage *string) machinev1alpha1.MachineStatus {


No strictly related to your overall change but machinecommon seems an oxymoron. It's not actually common if it is related to machines. And 'common' generally doesn't convey much.

frobware · 2018-12-18T09:59:51Z

pkg/apis/addtoscheme_machine_v1alpha1.go

+
+package apis
+
+//import (


Why are we committing commented out code?

frobware · 2018-12-18T10:04:02Z

pkg/apis/machine/common/consts.go

+limitations under the License.
+*/
+
+package common


So now I'm wondering why this is common. Can this not all move to ../machines? It would convey so much more as it is directly nested under machines.

frobware · 2018-12-18T10:22:10Z

pkg/util/util.go

@@ -177,33 +178,33 @@ func GetNamespaceOrDefault(namespace string) string {
 	return namespace
 }

-func ParseClusterYaml(file string) (*clusterv1.Cluster, error) {
+func ParseClusterYaml(file string) (*cluster.Cluster, error) {


[For consistency] Why is this no longer clusterv1?

alvaroaleman · 2018-12-18T19:16:42Z

I am very much not in favor of this. Was there a situation already in which one part of the API held back the other one?

The cluster-api repo is in my opinion about two things:

Types that are common among different implementations and are used by different implementers in order to ensure compatibility
A set of controller which an implementer may or may not use, depending on their needs

I think the actual critique here is not so much about the the types themselves being coupled strongly but about the machine actuator enforcing the usage of a cluster cr even when one does not want to manage the whole cluster with it.

That however is a shortcoming of the implementation that is in this repository. There are other implementations that work differently out there.

The fix for this issue should be one of:

Changing the implementation
Adding another implementation
Advising downstream consumers to build their own implementation if the machine actuator does not suit their needs

But not changing anything on this API itself.

enxebre · 2018-12-19T11:17:41Z

The cluster-api repo is in my opinion about two things:

Types that are common among different implementations and are used by different implementers in order to ensure compatibility
A set of controller which an implementer may or may not use, depending on their needs

@alvaroaleman thanks for feedback. I agree, upstream solution still should be valid and useful for many use cases, and provide the "right common" for guiding and therefore favouring adoption. As the cluster api overall project we'll still need to solve problems related to ha/cluster/control-plane/infra/bootstrapping which imho should evolve an API orthogonally to the "machine" API where the scope is more limited by nature. This is to provide balance between flexibility and having a manageable contained scope for the different layers of the project by giving clear boundaries and separation of responsibilities regardless the controllers implementation upstream/downstream

smarterclayton · 2018-12-19T16:23:20Z

Dropping in because this came up at KubeCon - historically, a Kube API group is intended to roughly encompass a lifecycle. That lifecycle has sometimes been SIG defined (autoscaling) but has also been related to orthogonal features (batch and v2beta1 being created for cronjob specifically, or the split between the various parts of what sig-auth controls). When boundaries can exist between lifecycles, we strongly recommend those be created. Federation v1 is a cautionary tale around coupling too closely between "high level API" (the workloads) and the low level api (the workload APIs on each cluster) - we erred a bit too far on putting everything all together, while federation v2 has focused on a much lower level primitive succeeding.

In this case, machine API is absolutely valuable regardless of the state of the cluster API. It is a building block, while the larger cluster API is a more opinionated and potentially differently-lifecycled API.

I would strongly encourage the SIG to consider having well factored building block APIs be at a different lifecycle, and potentially move into different groups. While there is a unifying philosophy (as there is for all Kubernetes objects), the practical realities of shipping APIs favors patterns where each API that can stand on its own can do so and evolve.

roberthbailey · 2019-01-11T20:16:10Z

/ok-to-test

k8s-ci-robot · 2019-01-15T09:24:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enxebre
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: justinsb

If they are not already assigned, you can assign the PR to them by writing /assign @justinsb in a comment when ready.

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

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

enxebre · 2019-01-16T10:48:18Z

/test pull-cluster-api-test

derekwaynecarr · 2019-01-16T21:34:24Z

I agree with @smarterclayton and @enxebre

roberthbailey · 2019-01-24T00:09:14Z

This came up during the subproject meeting today.

@vincepri and I are starting a doc about how/why we should split up api groups and we would love input there (both for and against).

@enxebre - do you want to hold this PR open in the mean time?

enxebre · 2019-01-25T11:40:31Z

@roberthbailey sounds good, I'll share my input in the doc

fejta-bot · 2019-04-29T15:43:10Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-05-29T16:25:22Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

detiber · 2019-06-19T14:38:07Z

Closing this since we discussed waiting until the types stabilize more before potentially breaking out into different api groups.

/close

k8s-ci-robot · 2019-06-19T14:38:09Z

@detiber: Closed this PR.

In response to this:

Closing this since we discussed waiting until the types stabilize more before potentially breaking out into different api groups.

/close

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.

k8s-ci-robot added 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 Dec 18, 2018

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 18, 2018

k8s-ci-robot requested review from karan and mkjelland December 18, 2018 09:35

frobware suggested changes Dec 18, 2018

View reviewed changes

enxebre force-pushed the machine-api branch 2 times, most recently from 58e44eb to 2e1a263 Compare December 18, 2018 10:56

enxebre force-pushed the machine-api branch from 2e1a263 to 9cf90d4 Compare December 20, 2018 13:16

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

enxebre force-pushed the machine-api branch from 9cf90d4 to c827431 Compare January 15, 2019 09:24

enxebre force-pushed the machine-api branch 2 times, most recently from b0ac693 to a863b27 Compare January 15, 2019 09:32

enxebre added 3 commits January 16, 2019 11:36

Decouple machine API from cluster API

ec92708

Run make generate after adding machine API

63872ee

Update config files for machine API

88a3276

enxebre force-pushed the machine-api branch from a863b27 to 88a3276 Compare January 16, 2019 10:36

roberthbailey mentioned this pull request Apr 12, 2019

Proposal: Split up the api group stable.agones.dev googleforgames/agones#703

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 29, 2019

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 29, 2019

k8s-ci-robot closed this Jun 19, 2019

enxebre mentioned this pull request Feb 5, 2020

REQUEST: New membership for enxebre kubernetes/org#1614

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Machine api #645

Machine api #645

enxebre commented Dec 18, 2018 •

edited

Loading

k8s-ci-robot commented Dec 18, 2018

enxebre commented Dec 18, 2018

frobware left a comment

frobware Dec 18, 2018

frobware Dec 18, 2018

frobware Dec 18, 2018

frobware Dec 18, 2018

alvaroaleman commented Dec 18, 2018

enxebre commented Dec 19, 2018 •

edited

Loading

smarterclayton commented Dec 19, 2018

roberthbailey commented Jan 11, 2019

k8s-ci-robot commented Jan 15, 2019

enxebre commented Jan 16, 2019

derekwaynecarr commented Jan 16, 2019

roberthbailey commented Jan 24, 2019

enxebre commented Jan 25, 2019

fejta-bot commented Apr 29, 2019

fejta-bot commented May 29, 2019

detiber commented Jun 19, 2019

k8s-ci-robot commented Jun 19, 2019

Machine api #645

Machine api #645

Conversation

enxebre commented Dec 18, 2018 • edited Loading

k8s-ci-robot commented Dec 18, 2018

enxebre commented Dec 18, 2018

frobware left a comment

Choose a reason for hiding this comment

frobware Dec 18, 2018

Choose a reason for hiding this comment

frobware Dec 18, 2018

Choose a reason for hiding this comment

frobware Dec 18, 2018

Choose a reason for hiding this comment

frobware Dec 18, 2018

Choose a reason for hiding this comment

alvaroaleman commented Dec 18, 2018

enxebre commented Dec 19, 2018 • edited Loading

smarterclayton commented Dec 19, 2018

roberthbailey commented Jan 11, 2019

k8s-ci-robot commented Jan 15, 2019

enxebre commented Jan 16, 2019

derekwaynecarr commented Jan 16, 2019

roberthbailey commented Jan 24, 2019

enxebre commented Jan 25, 2019

fejta-bot commented Apr 29, 2019

fejta-bot commented May 29, 2019

detiber commented Jun 19, 2019

k8s-ci-robot commented Jun 19, 2019

enxebre commented Dec 18, 2018 •

edited

Loading

enxebre commented Dec 19, 2018 •

edited

Loading