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

kubeadm: chroot to new --rootfs arg #54935

Merged
merged 2 commits into from Aug 27, 2018

Conversation

@anguslees
Member

anguslees commented Nov 1, 2017

What this PR does / why we need it:

This change adds a new --rootfs=path option to kubeadm, and (if
provided) chroot()s to this path before performing file operations.

This makes it possible to run the kubeadm binary from a container, but
perform remaining file operations against the host filesystem using
something like:

    docker run -v /:/rootfs --net=host --uts=host --pid=host \
       kubeadm:latest init ...

(Assuming something like the included examples/kubeadm/Dockerfile which sets CMD to kubeadm --rootfs=/rootfs - Edit: Dockerfile has been removed from this PR, but you get the idea)

Fixes kubernetes/kubeadm#503

Special notes for your reviewer:

  • I'm not sure where is best to put the Dockerfile, or hook it up to the build process. Advice sought.

  • The kubeadm command line arg handling was less unified than I was expecting to find. I've implemented this arg for init and join. I can add it to all the others too, if we're happy with the approach. An alternative would be to add the arg in the parent KubeadmCommand, possibly with a PersistantFlag - then it would automatically exist for all kubeadm subcommands.

  • It would be slightly preferable if we could order --rootfs before the subcommand so we could apply the arg automatically with ENTRYPOINT ["kubeadm", "--rootfs=/rootfs"]. This would be the only such flag in kubeadm however, so I have not implemented it that way atm. (Another alternative would be an env var)

Release note:

Adds a new EXPERIMENTAL `--rootfs` flag to kubeadm, which (if specified) causes kubeadm to chroot before performing any file operations.  This is expected to be useful when setting up kubernetes on a different filesystem, such as invoking kubeadm from docker.
@kad

what about cmd/kubeadm/app/phases/* ?

@kad

This comment has been minimized.

Show comment
Hide comment
@kad

kad Nov 1, 2017

Member

For new flags, it would be good also to have release note.

Member

kad commented Nov 1, 2017

For new flags, it would be good also to have release note.

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Nov 2, 2017

Member

@kad: do you want me to duplicate this code for all these subcommands (and phases)? That seems to be the current pattern, but if I were writing kubeadm I would instead lift this (and perhaps other common options) up to a single PersistentFlags declaration and move the runtime logic up to the root cobra.Command's Run method (ie: don't just use cmdutil.SubCmdRunE by itself).

I'm happy to do the cut+paste-everywhere version, I just want to be sure that is the direction you want me to go before doing it.

Member

anguslees commented Nov 2, 2017

@kad: do you want me to duplicate this code for all these subcommands (and phases)? That seems to be the current pattern, but if I were writing kubeadm I would instead lift this (and perhaps other common options) up to a single PersistentFlags declaration and move the runtime logic up to the root cobra.Command's Run method (ie: don't just use cmdutil.SubCmdRunE by itself).

I'm happy to do the cut+paste-everywhere version, I just want to be sure that is the direction you want me to go before doing it.

@kad

This comment has been minimized.

Show comment
Hide comment
@kad

kad Nov 2, 2017

Member

About moving to root cobra.Command I have no strong opinion. Most probably it will be better, but I haven't investigated that in details.

My point was purely about functionality: if we introduce functionality in some main commands (join/init), we should be able to do the same consistently in phases (if kubeadm is used as part of more complex solution which executes phases step-by-step) and in other main commands (reset, upgrade).

Member

kad commented Nov 2, 2017

About moving to root cobra.Command I have no strong opinion. Most probably it will be better, but I haven't investigated that in details.

My point was purely about functionality: if we introduce functionality in some main commands (join/init), we should be able to do the same consistently in phases (if kubeadm is used as part of more complex solution which executes phases step-by-step) and in other main commands (reset, upgrade).

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Nov 13, 2017

Member

PTAL.

Moved into a PersistentPreRun on the top level kubeadm command, so it now automatically applies to all current (and future) subcommands. It also makes the argument order more suitable for Dockerfile CMD (before the subcommand) - so now the example Dockerfile just automatically applies the --rootfs=/rootfs arg.

Note that the chroot happens before reading the config file - so the --config path is on the host rootfs, not inside the docker image (for the docker use-case example).

Member

anguslees commented Nov 13, 2017

PTAL.

Moved into a PersistentPreRun on the top level kubeadm command, so it now automatically applies to all current (and future) subcommands. It also makes the argument order more suitable for Dockerfile CMD (before the subcommand) - so now the example Dockerfile just automatically applies the --rootfs=/rootfs arg.

Note that the chroot happens before reading the config file - so the --config path is on the host rootfs, not inside the docker image (for the docker use-case example).

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Nov 23, 2017

Member

(Given the release freeze, I'm going to wait until I get a review rather than just sit here rebasing this every day. Please don't misinterpret an outstanding conflict as an indication that I no longer care about this PR.)

Member

anguslees commented Nov 23, 2017

(Given the release freeze, I'm going to wait until I get a review rather than just sit here rebasing this every day. Please don't misinterpret an outstanding conflict as an indication that I no longer care about this PR.)

@kad

This comment has been minimized.

Show comment
Hide comment
@kad

kad Nov 23, 2017

Member

In overall, looks good to me. @anguslees have you thought about looking at creating that kubeadm image as part of release artefacts ? If build scripts will be building this image and populating GCR at the same time as any other release images, it might be easier method for users to get kubeadm.

Member

kad commented Nov 23, 2017

In overall, looks good to me. @anguslees have you thought about looking at creating that kubeadm image as part of release artefacts ? If build scripts will be building this image and populating GCR at the same time as any other release images, it might be easier method for users to get kubeadm.

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Nov 23, 2017

Member

If build scripts will be building this image and populating GCR at the same time as any other release images, it might be easier method for users to get kubeadm.

Agreed 100% and that is my desire. As I said in the original PR comment, I have no idea how to go about getting this added/moved to the release images. Any pointers? (to docs or people/channels where I can ask again?)

Member

anguslees commented Nov 23, 2017

If build scripts will be building this image and populating GCR at the same time as any other release images, it might be easier method for users to get kubeadm.

Agreed 100% and that is my desire. As I said in the original PR comment, I have no idea how to go about getting this added/moved to the release images. Any pointers? (to docs or people/channels where I can ask again?)

@kad

This comment has been minimized.

Show comment
Hide comment
@kad

kad Nov 23, 2017

Member

I think first place to look at is: build/lib/release.sh, somewhere around function kube::release::create_docker_images_for_server(). But then there is bazel build, and this probably better to ask on #bazel channel in Slack.

Member

kad commented Nov 23, 2017

I think first place to look at is: build/lib/release.sh, somewhere around function kube::release::create_docker_images_for_server(). But then there is bazel build, and this probably better to ask on #bazel channel in Slack.

@errordeveloper

This comment has been minimized.

Show comment
Hide comment
@errordeveloper

errordeveloper Jan 9, 2018

Member

@anguslees is this still of interest? If yes, please rebase.

Member

errordeveloper commented Jan 9, 2018

@anguslees is this still of interest? If yes, please rebase.

@errordeveloper

This comment has been minimized.

Show comment
Hide comment
@errordeveloper

errordeveloper Jan 20, 2018

Member

Let's get this in, I like the idea and I think it would be useful in LinuxKit (cc @ijc).

Member

errordeveloper commented Jan 20, 2018

Let's get this in, I like the idea and I think it would be useful in LinuxKit (cc @ijc).

@ijc

This comment has been minimized.

Show comment
Hide comment
@ijc

ijc Jan 22, 2018

Contributor

LinuxKit's current model is to run kubeadm in the same container as kubelet is running, that container has any necessary binds to the host already in place etc and is naturally where kubelet expects to find configuration files anyway, so things mostly seem to work ok. This change would, I suppose, let you run kubeadm in a second container if you wanted, which might be useful (although I'm not immediately convinced).

FWIW the implementation looks ok to me, but I'm by no means an expert on kubeadm.

Contributor

ijc commented Jan 22, 2018

LinuxKit's current model is to run kubeadm in the same container as kubelet is running, that container has any necessary binds to the host already in place etc and is naturally where kubelet expects to find configuration files anyway, so things mostly seem to work ok. This change would, I suppose, let you run kubeadm in a second container if you wanted, which might be useful (although I'm not immediately convinced).

FWIW the implementation looks ok to me, but I'm by no means an expert on kubeadm.

@kad

This comment has been minimized.

Show comment
Hide comment
@kad

kad Feb 13, 2018

Member

@anguslees are you planning to update that PR ?

Member

kad commented Feb 13, 2018

@anguslees are you planning to update that PR ?

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Feb 14, 2018

Member

@anguslees are you planning to update that PR ?

Yes. I had to wait until the release freeze was over, and by then this had fallen off the top of my todo list :(

Adding a new docker image to the kube release process is also apparently a mildly impossible task unless you happen to be in a very small group of people (and a google employee). I'll give this another (more serious) attempt.

I'm not going to be able to put time into this again for another 2 weeks (at least). I'd welcome a PR-to-my-PR that I can just easily merge, if anyone else has the motivation and available cycles...

Member

anguslees commented Feb 14, 2018

@anguslees are you planning to update that PR ?

Yes. I had to wait until the release freeze was over, and by then this had fallen off the top of my todo list :(

Adding a new docker image to the kube release process is also apparently a mildly impossible task unless you happen to be in a very small group of people (and a google employee). I'll give this another (more serious) attempt.

I'm not going to be able to put time into this again for another 2 weeks (at least). I'd welcome a PR-to-my-PR that I can just easily merge, if anyone else has the motivation and available cycles...

@kad

This comment has been minimized.

Show comment
Hide comment
@kad

kad Feb 15, 2018

Member

@anguslees please at least rebase and update kubeadm part

Member

kad commented Feb 15, 2018

@anguslees please at least rebase and update kubeadm part

@kad

This comment has been minimized.

Show comment
Hide comment
@kad

kad Aug 8, 2018

Member

@anguslees I agree, all commands that require reading/writing any file should have --rootfs flag to be consistent in expected behaviour. Otherwise this PR looks good for me.

Member

kad commented Aug 8, 2018

@anguslees I agree, all commands that require reading/writing any file should have --rootfs flag to be consistent in expected behaviour. Otherwise this PR looks good for me.

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Aug 8, 2018

Member

@anguslees @kad
thank you for having a closer look at the commands that may or many not need --rootfs. i also think this PR is good to go now and it's not a big deal that we have commands that don't really benefit from the global flag right now.

thanks for the updates @anguslees and sorry for going back and forth with the reviews and also for this PR collecting dust for so long. i can do any further amends myself if you'd prefer.

/lgtm
pinging @timothysc @luxas @fabriziopandini for review/approval.

Member

neolit123 commented Aug 8, 2018

@anguslees @kad
thank you for having a closer look at the commands that may or many not need --rootfs. i also think this PR is good to go now and it's not a big deal that we have commands that don't really benefit from the global flag right now.

thanks for the updates @anguslees and sorry for going back and forth with the reviews and also for this PR collecting dust for so long. i can do any further amends myself if you'd prefer.

/lgtm
pinging @timothysc @luxas @fabriziopandini for review/approval.

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Aug 8, 2018

Member

/test pull-kubernetes-e2e-kops-aws

Member

neolit123 commented Aug 8, 2018

/test pull-kubernetes-e2e-kops-aws

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Aug 15, 2018

Member

/test all

Member

anguslees commented Aug 15, 2018

/test all

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Aug 15, 2018

Member

/retest

Member

anguslees commented Aug 15, 2018

/retest

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Aug 16, 2018

Member

/assign @fabriziopandini
please approve if you get a minute, we can take care of later adjustments to this, but this PR is since Nov 2017 and i think we should get it merged. :\

Member

neolit123 commented Aug 16, 2018

/assign @fabriziopandini
please approve if you get a minute, we can take care of later adjustments to this, but this PR is since Nov 2017 and i think we should get it merged. :\

@timothysc

This is useful, but there are weird side effects that may occur on chroots. I'm ok with this so long as --experiment-rootfs is given

/assign @timothysc

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Aug 23, 2018

Member

/retest

Member

anguslees commented Aug 23, 2018

/retest

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Aug 23, 2018

Member

I'm ok with this so long as --experiment-rootfs is given

To be clear: Are you saying the change is ok as-is, or you want me to rename the flag to --experiment-rootfs?

(You github-approved the change, but without an "/approve" for the k8s-bots to see)

Member

anguslees commented Aug 23, 2018

I'm ok with this so long as --experiment-rootfs is given

To be clear: Are you saying the change is ok as-is, or you want me to rename the flag to --experiment-rootfs?

(You github-approved the change, but without an "/approve" for the k8s-bots to see)

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Aug 23, 2018

Member

@anguslees @timothysc
i guess we can use --experimental-rootfs, but if decide to rename the flag this would break the user CLI.
maybe we can keep it --rootfs, but instead add the notes that this is experimental in both the release note and flag description:

Add the EXPERIMENTAL --rootfs flag to kubeadm, which (if specified) causes kubeadm to chroot before performing any file operations. This is expected to be useful when setting up kubernetes on a different filesystem, such as invoking kubeadm from docker.

"[EXPERIMENTAL] The path to the 'real' host root filesystem.",

WDYT?

Member

neolit123 commented Aug 23, 2018

@anguslees @timothysc
i guess we can use --experimental-rootfs, but if decide to rename the flag this would break the user CLI.
maybe we can keep it --rootfs, but instead add the notes that this is experimental in both the release note and flag description:

Add the EXPERIMENTAL --rootfs flag to kubeadm, which (if specified) causes kubeadm to chroot before performing any file operations. This is expected to be useful when setting up kubernetes on a different filesystem, such as invoking kubeadm from docker.

"[EXPERIMENTAL] The path to the 'real' host root filesystem.",

WDYT?

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Aug 24, 2018

Member

WDYT?

Happy to type whatever the latest approver wants to see, just need to know what that is.

Member

anguslees commented Aug 24, 2018

WDYT?

Happy to type whatever the latest approver wants to see, just need to know what that is.

@timothysc

This comment has been minimized.

Show comment
Hide comment
@timothysc

timothysc Aug 24, 2018

Member

+1 to "[EXPERIMENTAL] The path to the 'real' host root filesystem.",

Member

timothysc commented Aug 24, 2018

+1 to "[EXPERIMENTAL] The path to the 'real' host root filesystem.",

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Aug 25, 2018

Member

@anguslees ^^

also:

Add the EXPERIMENTAL --rootfs...

in the release note will be nice to have.
thanks

Member

neolit123 commented Aug 25, 2018

@anguslees ^^

also:

Add the EXPERIMENTAL --rootfs...

in the release note will be nice to have.
thanks

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 27, 2018

@anguslees

This comment has been minimized.

Show comment
Hide comment
@anguslees

anguslees Aug 27, 2018

Member

I have prepended [EXPERIMENTAL] to the option help text.

Member

anguslees commented Aug 27, 2018

I have prepended [EXPERIMENTAL] to the option help text.

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Aug 27, 2018

Member

@anguslees
thank you. please squash the commits.

Member

neolit123 commented Aug 27, 2018

@anguslees
thank you. please squash the commits.

@timothysc

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 27, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 27, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anguslees, neolit123, timothysc

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:

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

Contributor

k8s-ci-robot commented Aug 27, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anguslees, neolit123, timothysc

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:

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

@timothysc

This comment has been minimized.

Show comment
Hide comment
@timothysc

timothysc Aug 27, 2018

Member

/test pull-kubernetes-e2e-kops-aws

Member

timothysc commented Aug 27, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 27, 2018

Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

Contributor

k8s-merge-robot commented Aug 27, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 27, 2018

Contributor

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

Contributor

k8s-merge-robot commented Aug 27, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit 74d513f into kubernetes:master Aug 27, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation anguslees authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment