-
Notifications
You must be signed in to change notification settings - Fork 4.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
mounting kubectl from the host instead to installing in protokube #3550
mounting kubectl from the host instead to installing in protokube #3550
Conversation
ready to review, but I need to e2e @KashifSaadat / @andrewsykim / @alrs you mind reviewing? |
I tried this out but get the following failure with protokube:
|
Grumble - how the heck did it pass e2e... |
Do the E2E tests build a fresh protokube image and reference that for the build? Would be quite good to have, and same for nodeup. |
I have a WIP branch where I'm trying to get the protokube image and nodeup
build steps into the Travis CI tests.
The Makefile work is mostly-working, but I need to get past some
difficulties with Travis.
The work is in a Makefile called "NG.mk" in this PR:
#3505
|
f35edfd
to
0ce4369
Compare
1a25eb9
to
8e721f9
Compare
8e721f9
to
0706c21
Compare
@KashifSaadat e2e should be passing and I have tested it as well. I have not done testing with gossip yet. Yes, e2e tests everything, as committed, except for dns-controller (something we need to fix). I changed some path stuff so that paths won't mess with us on different distros. |
A few nits, but LGTM. What we have to be careful of here is that the version of kubectl isn't necessarily the version of the manifest we're applying. But I don't think this trickiness is actually made any worse what you're doing here. Want to make a pass over the code comments and then self-lgtm? |
@@ -32,9 +32,4 @@ cp /src/.build/local/protokube /src/.build/artifacts/ | |||
make channels | |||
cp /src/.build/local/channels /src/.build/artifacts/ | |||
|
|||
# channels uses protokube |
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.
Ooops - that comment wasn't right :-)
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.
SGTM
|
||
// KubectlPath returns distro based path for kubectl | ||
func (c *NodeupModelContext) KubectlPath() string { | ||
kubeletCommand := "/usr/local/bin" |
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.
nit: rename kubeletComamnd to kubectlPath
@@ -52,7 +52,7 @@ func (b *KubectlBuilder) Build(c *fi.ModelBuilderContext) error { | |||
} | |||
|
|||
t := &nodetasks.File{ | |||
Path: b.kubectlPath(), | |||
Path: b.KubectlPath() + "/" + assetName, |
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.
nit: use filepath.Join
// kubectl is downloaded an installed by other tasks | ||
if t.IsMaster { | ||
dockerArgs = append(dockerArgs, []string{ | ||
"-v", t.KubectlPath() + ":/opt/kops/bin:ro", |
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.
So docker can actually bind mount a file, which is nice because it avoids dragging in everything in the bin directory. Not sure whether I care, given we mount /
-> /rootfs
above...
A potential gotcha is that we have to be careful about shared libraries, but thankfully kubectl is statically linked.
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.
Right, but we have a chance that protokube will start before kubectl is installed ... async tasks ...
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 good point, and then we would hit the docker bug where it assumes a directory.
The tasks are technically ordered if we tell nodeup about our dependencies, but I do like your way better.
if t.IsMaster { | ||
dockerArgs = append(dockerArgs, []string{ | ||
"-v", t.KubectlPath() + ":/opt/kops/bin:ro", | ||
"--env", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/kops/bin", |
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.
Nit: Probably better to put /opt/kops/bin
at the start, except that we know that most of these dirs don't exist because we're in a container...
I guess the problem here is that channels
calls kubectl
, so we have to make sure it's on the path? If that's the case, can we have a comment, as otherwise the temptation will be to just change kubectl
calls to be a fully-qualified path.
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.
Well ... I disagree. We need it to be at the end, so that other stuff does not get whacked. First, wins in bash?
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.
OK - we don't expect many copies of kubectl :-)
Can we have a comment though about why we don't just call kubectl with an absolute path, i.e. why we are messing with the PATH at all
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.
Cause we do not want to make changes to channels :P Yes I will make a comment.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb 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 |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. |
So this will fix our protokube kubectl versioning issue. Kubectl is in on host, if we are on a master, and is always the right version, so let's use it! Refactored a bit to get the distro path for kubectl. Need to test on gossip. Set the path on protokube and mounted kubectl in
/opt/kops/bin
./approve
TODO
Fixes #3518