-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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.
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
azure: azkube v0.0.5 + deploy kube-system + hack/ script for hyperkube #23344
azure: azkube v0.0.5 + deploy kube-system + hack/ script for hyperkube #23344
Conversation
I'm dumping this review back into the pool. Hopefully someone who knows how other platforms are doing cluster bringup like this will be a better reviewer... |
@@ -34,6 +34,9 @@ KUBE_CONFIG_FILE="${KUBE_CONFIG_FILE:-"${DIR}/config-default.sh"}" | |||
source "${KUBE_CONFIG_FILE}" | |||
source "${KUBE_ROOT}/cluster/common.sh" | |||
|
|||
AZKUBE_VERSION="v0.0.4" | |||
REGISTER_MASTER_KUBELET="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is REGISTER_MASTER_KUBELET
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value is used by validate-cluster.sh
to check for the number of nodes reported. Relates to whether or not the master kubelet is brought up with --register-node={true,false}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, understand, I only did git grep REGISTER_MASTER_KUBELET -- hack
. Every single other user is in cluster
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just to clarify -- this usage is in cluster/
as well. The hack/
script isn't used in the mainline for the cluster bring-up. It's just something I'm using while iterating on a new cloudprovider plugin and thought other hyperkube-users might use as well.)
CC: @davidopp |
@mikedanese can you review this? |
Actually nevermind, this is pretty quick to review. |
kube::build::run_build_command hack/build-go.sh cmd/hyperkube | ||
|
||
REGISTRY="${KUBE_DOCKER_REGISTRY}/${KUBE_DOCKER_OWNER}" \ | ||
VERSION="latest" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be $KUBE_DOCKER_VERSION rather than hard coded to "latest". Will fix this asap.
2859db8
to
21b5d60
Compare
|
||
# This script will build and push the hyperkube image. | ||
# This is useful for rapidly testing changes in a cluster that is | ||
# powered by hyperkube. You can configure the cluster to use a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add more details to the comment starting at "You can configure"? For example
- how do you configure the cluster to use a hyperkube tag?
- what does "pulling the tag on the nodes" mean? was this an abbreviation for "using 'docker pull' to pull the tagged hyperkube image you built onto each node" ? If so, please write out the more complete wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidopp I've already updated the PR for the Azure docs to speak to "AZURE_HYPERKUBE_SPEC" which is how someone can use a specific hyperkube image for the cluster. Given that, I think the comment should be adjusted to something like:
"This script will build the hyperkube image and then push to to the repository referred to by KUBE_DOCKER_REGISTRY and KUBE_DOCKER_OWNER, and tagged with KUBE_DOCKER_VERSION."
The remainder of the comment is specific to Azure (or any other cluster that is bootstrapped with hyperkube).
That sound okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, it's updated and rebased.
da79c66
to
9924a38
Compare
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
fa0c487
to
348bb2d
Compare
ping @colemickens |
ping @colemickens -- if you want this in 1.3, it needs to be fixed by Monday |
348bb2d
to
579d179
Compare
CLAs look good, thanks! |
Rebased and pushed. |
LGTM |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 579d179. |
Automatic merge from submit-queue |
Update the Azure bring up.
Also added a script
hack/dev-push-hyperkube.sh
for fast iteration on changes. One can deploy a cluster with a given hyperkube image reference and then use this script to rapidly iterate on changes. Testing changes just requires pulling the updated hyperkube image on the boxes and then cycling kubelet/docker or restarting the node.This change is