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.

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

Fix flexvolume in containerized kubelets #65549

Merged
merged 1 commit into from Jul 9, 2018

Conversation

@gnufied
Copy link
Member

gnufied commented Jun 27, 2018

Fixes flex volumes in containerized kubelets.

cc @jsafrane @chakri-nelluri @verult

Note to reviewers : e2e tests pass in local containarized cluster.

Fix flexvolume in containarized kubelets
// +build linux

/*
Copyright 2017 The Kubernetes Authors.

This comment has been minimized.

@jsafrane
@@ -0,0 +1,75 @@
// +build linux

This comment has been minimized.

@jsafrane

jsafrane Jun 28, 2018

Member

I'd put it directly into nsenter/exec.go, it's quite small package and it won't conflict with anything.

}

// Command returns a command wrapped with nenter
func (nsExecutor *Executor) Command(cmd string, args ...string) exec.Cmd {

This comment has been minimized.

@jsafrane

jsafrane Jun 28, 2018

Member

There already is Nsenter.Exec with the same arguments and implementation. It would be better if one called the other instead of copying the code.

This comment has been minimized.

@gnufied

gnufied Jun 28, 2018

Author Member

The NsEnter.Exec afaik can only execs "known" binaries. I had to duplicate the logic here, so as we can call flex binary installed on the host.

@gnufied gnufied force-pushed the gnufied:fix-flexvolume-containers branch from e80f006 to 2eb06b6 Jun 28, 2018

@@ -127,7 +131,7 @@ func (prober *flexVolumeProber) newProbeEvent(driverDirName string, op volume.Pr
Op: op,
}
if op == volume.ProbeAddOrUpdate {
plugin, pluginErr := prober.factory.NewFlexVolumePlugin(prober.pluginDir, driverDirName)
plugin, pluginErr := prober.factory.NewFlexVolumePlugin(prober.pluginDir, driverDirName, prober.runner)

This comment has been minimized.

@verult

verult Jun 30, 2018

Contributor

Is it OK to have multiple plugins reference the same runner?

This comment has been minimized.

@gnufied

gnufied Jul 2, 2018

Author Member

Yeah, should not be a problem because exec.New basically returns a stateless struct and each execution calls its own instance of osexec.Command

@@ -318,7 +319,7 @@ type fakePluginFactory struct {

var _ PluginFactory = fakePluginFactory{}

func (m fakePluginFactory) NewFlexVolumePlugin(_, driverName string) (volume.VolumePlugin, error) {
func (m fakePluginFactory) NewFlexVolumePlugin(_, driverName string, runner exec.Interface) (volume.VolumePlugin, error) {

This comment has been minimized.

@verult

verult Jun 30, 2018

Contributor

nit: _ exec.Interface

}

// NewNsenterExectutor returns new nsenter based executor
func NewNsenterExectutor(hostRootFsPath string, executor exec.Interface) *Executor {

This comment has been minimized.

@verult

verult Jun 30, 2018

Contributor

Exectutor -> Executor

@gnufied gnufied force-pushed the gnufied:fix-flexvolume-containers branch from 2eb06b6 to 72defe7 Jul 2, 2018

@gnufied gnufied force-pushed the gnufied:fix-flexvolume-containers branch from 72defe7 to baca5cc Jul 2, 2018

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jul 2, 2018

@verult fixed the comments. PTAL.

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Jul 2, 2018

/lgtm

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jul 3, 2018

/test pull-kubernetes-e2e-gce

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jul 3, 2018

/assign @smarterclayton @deads2k for approval in other areas.

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jul 3, 2018

@verult

This comment has been minimized.

Copy link
Contributor

verult commented Jul 3, 2018

Flexvolume plumbing LGTM


// Command returns a command wrapped with nenter
func (nsExecutor *Executor) Command(cmd string, args ...string) exec.Cmd {
hostProcMountNsPath := filepath.Join(nsExecutor.hostRootFsPath, mountNsPath)

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Jul 3, 2018

Contributor

Minor optimization - Can this be moved to "NewNsenterExecutor"?

This comment has been minimized.

@gnufied

gnufied Jul 3, 2018

Author Member

fixed

)

type flexVolumeProber struct {
mutex sync.Mutex
pluginDir string // Flexvolume driver directory
pluginDir string // Flexvolume driver directory
runner exec.Interface // interface to use for execing flex calls

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Jul 3, 2018

Contributor

Nit - Interface with capital I

This comment has been minimized.

@gnufied

gnufied Jul 3, 2018

Author Member

fixed

// NewNsenterExecutor returns new nsenter based executor
func NewNsenterExecutor(hostRootFsPath string, executor exec.Interface) *Executor {
nsExecutor := &Executor{
hostRootFsPath: hostRootFsPath,

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Jul 3, 2018

Contributor

*rename - hostRootFsPath? Ideally this can be any path right and need not be host root fs right?

This comment has been minimized.

@gnufied

gnufied Jul 3, 2018

Author Member

It has to be path where "/" of host is mounted.

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Jul 3, 2018

Contributor

If I pass the root of any other container, wouldn't it work? If not this utility library should it ideally be rootnsenter because of the implicit assumptions?

This comment has been minimized.

@gnufied

gnufied Jul 3, 2018

Author Member

Theoretically yes - you can pass root of any other container if you wanted. The problem is, how will mount propagation work from that container into the container which is started for pod?

So for most practical purposes, we will want this to be host's root.

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Jul 3, 2018

Contributor

Just the naming is a bit consuming given that this is in util and the assumption doesn't come out clearly. If everyone is fine, I am fine with it. ping @vishh

@chakri-nelluri
Copy link
Contributor

chakri-nelluri left a comment

Added minor comments

@gnufied gnufied force-pushed the gnufied:fix-flexvolume-containers branch from baca5cc to 8db5328 Jul 3, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 3, 2018

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jul 3, 2018

@chakri-nelluri @msau42 @verult can you please retag with lgtm again? I addressed comments that @chakri-nelluri left on the PR.

@jsafrane

This comment has been minimized.

Copy link
Member

jsafrane commented Jul 3, 2018

/lgtm

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor

chakri-nelluri commented Jul 3, 2018

/lgtm

@chakri-nelluri

This comment has been minimized.

Copy link
Contributor

chakri-nelluri commented Jul 3, 2018

Thanks @gnufied

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jul 6, 2018

/approve

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Jul 8, 2018

/retest

@eparis

This comment has been minimized.

Copy link
Member

eparis commented Jul 8, 2018

/approve

@jsafrane

This comment has been minimized.

Copy link
Member

jsafrane commented Jul 9, 2018

/assign @deads2k @lavalamp
for approval

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Jul 9, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 9, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chakri-nelluri, deads2k, derekwaynecarr, eparis, gnufied, jsafrane, msau42

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

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Jul 9, 2018

/approve

Controller manage changes look reasonable to me. I wonder about the general wisdom and reliability of requiring plugins to present inside pods running kube-controller-managers.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 9, 2018

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

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jul 9, 2018

@deads2k yeah I think ship sailed on that choice long ago. Regardless of this change, if a flexvolume plugin has attach/detach component then it must be present inside pod running the controller-manager. This PR does not in anyway changes controller-manager's mechanism.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 9, 2018

Automatic merge from submit-queue (batch tested with PRs 65456, 65549). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 3155ea2 into kubernetes:master Jul 9, 2018

11 of 17 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-bazel-test
Details
pull-kubernetes-bazel-test Job triggered.
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
cla/linuxfoundation gnufied authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Job succeeded.
Details
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 9, 2018

@gnufied: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-kubemark-e2e-gce-big 8db5328 link /test pull-kubernetes-kubemark-e2e-gce-big
pull-kubernetes-bazel-test 8db5328 link /test pull-kubernetes-bazel-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Aug 9, 2018

cc @tallclair
for new exec usage

@fabianvf fabianvf referenced this pull request Dec 12, 2018

Closed

Set UID + GID based on Pod SecurityContext #1930

2 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.