kubeadm: turn api into a real apigroup #34147

mikedanese · 2016-10-05T22:34:54Z

@kubernetes/sig-cluster-lifecycle

This change is

k8s-ci-robot · 2016-10-05T23:06:07Z

Jenkins verification failed for commit 2052a01. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

justinsb · 2016-10-06T15:28:47Z

+1 on the idea @mikedanese (sadly I don't know enough to review the code). And thanks for providing an example on how to use the k8s api machinery. (Did you find a nice doc on how to do this BTW?)

krousey

I'm fine with the code change, but are we sure we want to use the API machinery for what is essentially a versioned config file? cc @kubernetes/sig-api-machinery
Things I usually associate with an API group that don't apply here:

Swagger definitions
API discovery
Storage interfaces
Client
REST endpoints
alternate encodings

If we do use the API, would this work without ugorji codec?

krousey · 2016-10-06T17:57:29Z

cmd/kubeadm/app/api/docs.go

-func ValidateNodeConfiguration(o *MasterConfiguration) error {
-	return nil
-}
+// +groupName=kubeadm.k8s.io


Does our machinery require that this line come right before the package statement? Comments, even annotations like this, right before package statements show up as documentation on godoc.org.

It does. We need to fix it

lavalamp · 2016-10-06T19:56:28Z

cmd/kubeadm/app/api/install/install.go

@@ -0,0 +1,38 @@
+/*


please make the path to this file (and other api machinery files) be .../apis/<your group name>/..., not /api

What does registering an API group provide? I only need versioning, defaulting and documentation.

lavalamp · 2016-10-06T19:57:13Z

cmd/kubeadm/app/api/install/install.go

+		announced.VersionToSchemeFunc{
+			v1alpha1.SchemeGroupVersion.Version: v1alpha1.AddToScheme,
+		},
+	).Announce().RegisterAndEnable(); err != nil {


Do you actually need to register this group?

errordeveloper · 2016-10-07T06:15:06Z

I'm fine with the code change, but are we sure we want to use the API machinery for what is essentially a versioned config file?

Yes, good question, I wonder about this also.

errordeveloper · 2016-10-07T06:17:45Z

@mikedanese thanks a lot for doing this! As far as I understand, this is not in use yet, so are you planning to make use of it in a follow-up PR or you will add it here?

mikedanese · 2016-10-07T06:51:12Z

are you planning to make use of it in a follow-up PR or you will add it here?

End goal is to make this deserialize the internal config representation from a file passed in from the command line for the "configurable mode" experience. With this we can hopefully avoid diluting the streamlined command line experience.

mikedanese · 2016-10-07T06:52:40Z

I'm fine with the code change, but are we sure we want to use the API machinery for what is essentially a versioned config file?

I feel like this ship flew with componentconfig. Aren't these requirements (versioning, defaulting and documentation) the some of the core use cases for API groups?

lavalamp · 2016-10-07T17:37:31Z

It will make it an option in the --runtime-config flag, which I guess
doesn't matter if you are working in a separate binary.

On Thu, Oct 6, 2016 at 11:48 PM, Mike Danese notifications@github.com
wrote:

@mikedanese commented on this pull request.

In cmd/kubeadm/app/api/install/install.go
#34147:

@@ -0,0 +1,38 @@
+/*

What does registering an API group provide? I only need versioning,
defaulting and documentation.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#34147, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAngloDe6QTsTFxBpc3-Hq8c36ik01h9ks5qxeszgaJpZM4KPXKR
.

mikedanese · 2016-10-07T18:16:04Z

Ok unregistered the group.

mikedanese · 2016-10-07T18:17:39Z

What about roundtrip testing?

dgoodwin · 2016-10-07T18:51:41Z

I think previous commit 56ea178 may have broken something, the controller manager is unable to start up with a plain "kubeadm init master" now:

(root@centos1 ~) $ docker logs 589b79ef9fd9
E1007 18:44:49.091488       1 event.go:258] Could not construct reference to: '&api.Endpoints{TypeMeta:unversioned.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:api.ObjectMeta{Name:"kube-controller-manager", GenerateName:"", Namespace:"kube-system", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:unversioned.Time{Time:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*unversioned.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]api.OwnerReference(nil), Finalizers:[]string(nil), ClusterName:""}, Subsets:[]api.EndpointSubset(nil)}' due to: 'selfLink was empty, can't make reference'. Will not report event: 'Normal' '%v became leader' 'centos1.aos.example.com'
I1007 18:44:49.092504       1 leaderelection.go:214] sucessfully acquired lease kube-system/kube-controller-manager
I1007 18:44:49.098480       1 plugins.go:71] No cloud provider specified.
W1007 18:44:49.098495       1 controllermanager.go:232] Unsuccessful parsing of cluster CIDR : invalid CIDR address: 
W1007 18:44:49.098503       1 controllermanager.go:236] Unsuccessful parsing of service CIDR : invalid CIDR address: 
I1007 18:44:49.099198       1 replication_controller.go:220] Starting RC Manager
I1007 18:44:49.099747       1 nodecontroller.go:194] Sending events to api server.
F1007 18:44:49.099815       1 nodecontroller.go:206] NodeController: Must specify clusterCIDR if allocateNodeCIDRs == true.

mikedanese · 2016-10-07T19:28:05Z

@dgoodwin before in kubeadm init master --pod-network-cidr was --pod-network-cidr a required flag? Should it be required? I remember @errordeveloper saying it shouldn't be. If it shouldn't be then #34352 might fix your issue.

errordeveloper · 2016-10-09T11:18:57Z

@mikedanese @dgoodwin --pod-network-cidr should be optional.

mikedanese · 2016-10-10T15:28:10Z

@krousey is that an lgtm? :)

luxas · 2016-10-10T15:36:38Z

@errordeveloper Yes, that was probably a rebase conflict or something from some previous refactoring PR (after our "stable" v1.4 release)

@mikedanese I think that's a LGTM, yes

@errordeveloper Any comments on this one?

errordeveloper · 2016-10-10T15:45:59Z

@mikedanese:

End goal is to make this deserialize the internal config representation from a file passed in from the command line for the "configurable mode" experience. With this we can hopefully avoid diluting the streamlined command line experience.

Yes, I'm familiar with the goal already 👍 :)

I am just wondering how far does this change set get us, I cannot see any flags being added here explicitly, I'd expect there to be at least one more change set to plumb it through, yet I am not aware if there is any magic that would infer any automatic flags for passing a componentconfig file in...

krousey · 2016-10-10T16:10:36Z

Yes

On Oct 10, 2016 08:28, "Mike Danese" notifications@github.com wrote:

@krousey https://github.com/krousey is that an lgtm? :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34147 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJlm5XRGebcHtcSPLENtNM7d0qBHuTZks5qylmOgaJpZM4KPXKR
.

k8s-github-robot · 2016-10-10T16:21:05Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-10-10T16:59:53Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 5, 2016

mikedanese force-pushed the kubeadm-api branch from 78e4761 to 2052a01 Compare October 5, 2016 22:35

mikedanese assigned krousey Oct 5, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Oct 5, 2016

krousey reviewed Oct 6, 2016

View reviewed changes

lavalamp reviewed Oct 6, 2016

View reviewed changes

mikedanese force-pushed the kubeadm-api branch from 2052a01 to 80116c7 Compare October 7, 2016 18:05

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 7, 2016

kubeadm: turn api into a real apigroup

b3dae78

mikedanese force-pushed the kubeadm-api branch from 80116c7 to b3dae78 Compare October 7, 2016 18:15

k8s-github-robot mentioned this pull request Oct 7, 2016

[k8s.io] Deployment overlapping deployment should not fight with each other {Kubernetes e2e suite} #32947

Closed

krousey approved these changes Oct 7, 2016

View reviewed changes

mikedanese added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 10, 2016

mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2016

k8s-github-robot merged commit 851705e into kubernetes:master Oct 10, 2016

mikedanese deleted the kubeadm-api branch October 10, 2016 17:47

luxas mentioned this pull request Oct 15, 2016

release a new version of kubeadm #34884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: turn api into a real apigroup #34147

kubeadm: turn api into a real apigroup #34147

mikedanese commented Oct 5, 2016 •

edited by k8s-oncall

Loading

k8s-ci-robot commented Oct 5, 2016

justinsb commented Oct 6, 2016

krousey left a comment

krousey Oct 6, 2016

smarterclayton Oct 6, 2016

lavalamp Oct 6, 2016

mikedanese Oct 7, 2016

lavalamp Oct 6, 2016

errordeveloper commented Oct 7, 2016

errordeveloper commented Oct 7, 2016

mikedanese commented Oct 7, 2016

mikedanese commented Oct 7, 2016

lavalamp commented Oct 7, 2016

@mikedanese commented on this pull request.

mikedanese commented Oct 7, 2016

mikedanese commented Oct 7, 2016

dgoodwin commented Oct 7, 2016

mikedanese commented Oct 7, 2016

errordeveloper commented Oct 9, 2016

mikedanese commented Oct 10, 2016

luxas commented Oct 10, 2016

errordeveloper commented Oct 10, 2016 •

edited

Loading

krousey commented Oct 10, 2016

k8s-github-robot commented Oct 10, 2016

k8s-github-robot commented Oct 10, 2016

kubeadm: turn api into a real apigroup #34147

kubeadm: turn api into a real apigroup #34147

Conversation

mikedanese commented Oct 5, 2016 • edited by k8s-oncall Loading

k8s-ci-robot commented Oct 5, 2016

justinsb commented Oct 6, 2016

krousey left a comment

Choose a reason for hiding this comment

krousey Oct 6, 2016

Choose a reason for hiding this comment

smarterclayton Oct 6, 2016

Choose a reason for hiding this comment

lavalamp Oct 6, 2016

Choose a reason for hiding this comment

mikedanese Oct 7, 2016

Choose a reason for hiding this comment

lavalamp Oct 6, 2016

Choose a reason for hiding this comment

errordeveloper commented Oct 7, 2016

errordeveloper commented Oct 7, 2016

mikedanese commented Oct 7, 2016

mikedanese commented Oct 7, 2016

lavalamp commented Oct 7, 2016

@mikedanese commented on this pull request.

mikedanese commented Oct 7, 2016

mikedanese commented Oct 7, 2016

dgoodwin commented Oct 7, 2016

mikedanese commented Oct 7, 2016

errordeveloper commented Oct 9, 2016

mikedanese commented Oct 10, 2016

luxas commented Oct 10, 2016

errordeveloper commented Oct 10, 2016 • edited Loading

krousey commented Oct 10, 2016

k8s-github-robot commented Oct 10, 2016

k8s-github-robot commented Oct 10, 2016

mikedanese commented Oct 5, 2016 •

edited by k8s-oncall

Loading

errordeveloper commented Oct 10, 2016 •

edited

Loading