Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

hyperkube: add cloud-controller-manager #54197

Merged
merged 6 commits into from
Nov 15, 2017
Merged

hyperkube: add cloud-controller-manager #54197

merged 6 commits into from
Nov 15, 2017

Conversation

colemickens
Copy link
Contributor

@colemickens colemickens commented Oct 19, 2017 •
edited

What this PR does / why we need it:

Adds cloud-controller-manager to hyperkube. (fix #55732)

This is useful as a number of deployment tools run all of the kubernetes components from the hyperkube image/binary. It also makes testing easier as a single binary/image can be built and pushed quickly.

This PR follows the same pattern of the other kubernetes binaries being available as part of hyperkube.

(This PR also makes an error condition appropriately fatal.)

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): n/a

Special notes for your reviewer: n/a

Release note:

hyperkube: add cloud-controller-manager

/sig cluster-lifecycle
/area cloudprovider

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. area/cloudprovider size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 19, 2017
@colemickens colemickens force-pushed the hyperkube-add-cloud-controller-manager branch from 2a5257a to e284db2 Compare October 19, 2017 04:22
luxas
luxas reviewed Oct 19, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be too strict, but I´m not sure I wanna see the ccm being in hyperkube. I think that goes the wrong way (adding more deps on clouds rather than less)

The preferred way for clouds to do this should be to create their own binary with their own extensions ASAP. The ccm binary in-core is just a stop-gap.

That said, I see why it´s useful in the meantime, and we already build the binary in releases, creating images, etc. so we should probably be able to do this too.

@colemickens
Copy link
Contributor Author

@luxas I agree with both of your points. I assume that when CCM is removed from the tree, we would also remove it from hyperkube. In the meantime, this can be helpful for a couple releases. Given how big the binary and image already are, hopefully this can be tolerable?

@luxas
Copy link
Member

luxas commented Oct 25, 2017

@luxas I agree with both of your points. I assume that when CCM is removed from the tree, we would also remove it from hyperkube. In the meantime, this can be helpful for a couple releases. Given how big the binary and image already are, hopefully this can be tolerable?

Is there any way we could make this as hyperkube alpha cloud-controller-manager or something? That would alleviate the concerns on the hyperkube CLI API having to be stable for me at least

@colemickens
Copy link
Contributor Author

Sure, let me take a look at doing so. Is there an existing example of this already? Thanks!

@colemickens colemickens force-pushed the hyperkube-add-cloud-controller-manager branch from e284db2 to 522fbc0 Compare October 26, 2017 19:10
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 26, 2017
@colemickens colemickens force-pushed the hyperkube-add-cloud-controller-manager branch from 522fbc0 to a49ce26 Compare October 26, 2017 19:18
@colemickens
Copy link
Contributor Author

@luxas I hacked in an alpha subcommand and added tests for it. Let me know what you think.

@colemickens colemickens force-pushed the hyperkube-add-cloud-controller-manager branch 2 times, most recently from 5292407 to d54e3e2 Compare October 27, 2017 00:58
@colemickens
Copy link
Contributor Author

/test pull-kubernetes-unit

luxas
luxas approved these changes Oct 27, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of LGTM, left two nits

I'd like @wlan0 and @thockin sign off as well
/assign @wlan0 @thockin

cmd/hyperkube/hyperkube.go Outdated
if len(args) > 0 && len(args[0]) > 0 {
serverName = args[0]
args = args[1:]
hk.Printf("Warning: alpha command syntax is unable!\n\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is unable to what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a second, it's supposed to be an instability warning. s/unsable/unstable/g. Will push a new commit and can squash at the end.

cmd/hyperkube/cloud-controller-manager.go Outdated
@@ -0,0 +1,39 @@
/*
Copyright 2015 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 2017 :)

@colemickens colemickens force-pushed the hyperkube-add-cloud-controller-manager branch 3 times, most recently from c33f719 to 519cb04 Compare October 27, 2017 17:55
@luxas
Copy link
Member

luxas commented Oct 27, 2017

Thanks! Handing over to @wlan0 and @thockin then.

wlan0
wlan0 reviewed Oct 29, 2017
View reviewed changes
cmd/hyperkube/main.go Outdated
hk.AddAlphaServer(NewCloudControllerManager())

// Alpha servers
hk.AddAlphaServer(NewCloudControllerManager())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be here twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it shouldn't. Maybe a rebase error? Fixed in a new commit. (Will squash)

@wlan0
Copy link
Member

wlan0 commented Oct 29, 2017

@colemickens "alpha" command wiring is cool. I just have one comment.

@thockin thockin removed their assignment Nov 9, 2017
@thockin
Copy link
Member

thockin commented Nov 9, 2017

Assign back to me once it is LGTM'ed and ready for approval.

@colemickens colemickens force-pushed the hyperkube-add-cloud-controller-manager branch from 519cb04 to 49cd7fc Compare November 10, 2017 22:30
@colemickens
Copy link
Contributor Author

@wlan0 Thanks for the review. Fixed the issue you caught and pushed a new commit. If it looks good, I'll squash.

@luxas
Copy link
Member

luxas commented Nov 13, 2017

ready for approval @thockin

@thockin
Copy link
Member

thockin commented Nov 13, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017
@thockin
Copy link
Member

thockin commented Nov 13, 2017

This needs an associated issue and a closes #12345 in the FIRST COMMENT, or it can not be approved.

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2017
@dims
Copy link
Member

dims commented Nov 14, 2017

@thockin This is ready now with a link to a bug as you pointed out. PTAL

@wlan0
Copy link
Member

wlan0 commented Nov 15, 2017

/lgtm
/approve

@wlan0
Copy link
Member

wlan0 commented Nov 15, 2017

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: colemickens, thockin, wlan0

Associated issue: 55732

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit b262585 into kubernetes:master Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wlan0 wlan0 wlan0 left review comments

@luxas luxas luxas approved these changes

Assignees

@cheftako cheftako

@wlan0 wlan0

@mikedanese mikedanese

@luxas luxas

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

hyperkube should include cloud-controller-manager
9 participants
@colemickens @luxas @wlan0 @thockin @dims @k8s-github-robot @cheftako @mikedanese @k8s-ci-robot