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

CRI: define the mount behavior when host path does not exist #61460

Merged
merged 1 commit into from Mar 29, 2018

Conversation

@feiskyer
Member

feiskyer commented Mar 21, 2018

What this PR does / why we need it:

This PR defines the mounting behavior when host path does not exist in CRI. Specifically,

  • If the hostPath doesn't exist (e.g. hostPath volume), runtimes should report errors
  • If the specified hostPath is a symlink, runtimes should follow the symlink and mount the real destination to the container

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #52318

Special notes for your reviewer:

Release note:

CRI: define the mount behavior when host path does not exist: runtime should report error if the host path doesn't exist
@feiskyer

This comment has been minimized.

Member

feiskyer commented Mar 21, 2018

@k8s-ci-robot k8s-ci-robot requested review from Random-Liu and yujuhong Mar 21, 2018

@feiskyer

This comment has been minimized.

Member

feiskyer commented Mar 21, 2018

/sig node

@dixudx

A breaking change for volume plugin HostPath.

@@ -266,6 +266,13 @@ func (m *kubeGenericRuntimeManager) makeMounts(opts *kubecontainer.RunContainerO
for idx := range opts.Mounts {
v := opts.Mounts[idx]
if _, err := m.osInterface.Stat(v.HostPath); os.IsNotExist(err) {

This comment has been minimized.

@dixudx

dixudx Mar 21, 2018

Member

xref #46597.

This is a breaking change for volume plugin HostPath, although no harm comes. Only when the type of hostpath in the PodSpec is set to "DirectoryOrCreate", a new dir will be created automatically. The default behavior is a no-op, which is mainly for backward compatibility. In this change, when the HostPathType is unset, a new dir will still be created, this is not what we suggested.

For other types of volume plugins, I think we should let the plugin mounter handle it independently as well. WDYT?

This comment has been minimized.

@feiskyer

feiskyer Mar 21, 2018

Member

The default behavior is a no-op, which is mainly for backward compatibility. In this change, when the HostPathType is unset, a new dir will still be created, this is not what we suggested.

Although there is no-op in volume manager, docker will create a new dir for the hostPath. This PR won't change this behavior and aligns all other CRI runtimes with the same behavior.

This comment has been minimized.

@dixudx

dixudx Mar 21, 2018

Member

docker will create a new dir for the hostPath. This PR won't change this behavior and aligns all other CRI runtimes with the same behavior.

AFAIK Rkt won't create such dir if not existed.

moby/moby#21666 added back the deprecated auto-creation of host-directories, although no consensus had been reached.

Just as the hostpath type proposal doc #34058 said,

This is rarely what the user actually wants because hostPath volumes are typically used to express a dependency on an existing external file or directory.

it was suggested that orchestration systems could better manage volume creation than Docker, but Docker does so as well anyways.

IMO it is kubelet that should be responsible for such auto-creation, instead of docker, rkt, or other CRI runtimes.

This comment has been minimized.

@feiskyer

feiskyer Mar 22, 2018

Member

IMO it is kubelet that should be responsible for such auto-creation, instead of docker, rkt, or other CRI runtimes.

This is what this PR trying to do. I think we should define the logic clearly in kubelet and CRI runtimes shouldn't care about this issue.

This comment has been minimized.

@dixudx

dixudx Mar 23, 2018

Member

This is what this PR trying to do. I think we should define the logic clearly in kubelet and CRI runtimes shouldn't care about this issue.

Let the volume plugins/mgrs handle it. WDYT?

And this PR is breaking the behavior of volume plugin HostPath. For non-existent dir, a new dir should not be created unless HostPathType is set to DirectoryOrCreate.

This comment has been minimized.

@Random-Liu

Random-Liu Jun 5, 2018

Member

I don't understand the discussion here. I think @dixudx is right. We should not enforce if the hostPath doesn't exist, then runtimes should report error, because that breaks the backward compatibility. ALL existing pods using non-existent host path will fail. That is unacceptable.

@dixudx Why don't we default to DirectoryOrCreate? That is docker's behavior today IIRC. Default to that is no different with default to Unset for docker. And if we default to that, this PR will be ok.

AFAIK Rkt won't create such dir if not existed.

rkt in tree integration is deprecated.

If we have to default to Unset, we should define If the hostPath doesn't exist, runtime should create it instead. @feiskyer @yujuhong

This comment has been minimized.

@Random-Liu

Random-Liu Jun 5, 2018

Member

NVM, saw https://github.com/kubernetes/kubernetes/pull/46597/files#r130686032 which makes sense to me.

Given so, I think we should either not define this in CRI or define If the hostPath doesn't exist, runtime should create a directory with 0755. @feiskyer

This comment has been minimized.

@feiskyer

feiskyer Jun 5, 2018

Member

If so, it's a same operation for all runtimes. why not create the dir in kubelet instead?

This comment has been minimized.

@Random-Liu

Random-Liu Jun 5, 2018

Member

@feiskyer We can default HostPathUnset to create directory in kubelet if not exist, but we are not doing that today.

Given this is not finalized, we should at least remove current definition which doesn't work with today's kubelet implementation.

This comment has been minimized.

@feiskyer

feiskyer Jun 6, 2018

Member

How about adding creation logic in kuberuntime and keep CRI as is? In such way, runtimes should still error if the hostpath doesn't exist, right?

@feiskyer

This comment has been minimized.

@k8s-ci-robot k8s-ci-robot added size/XS and removed size/S labels Mar 23, 2018

@dixudx

This comment has been minimized.

Member

dixudx commented Mar 23, 2018

If the hostPath doesn't exist (e.g. hostPath volume), kubelet now creates the hostPath before creating containers

@feiskyer Please edit this description.

@dixudx

This comment has been minimized.

Member

dixudx commented Mar 23, 2018

/lgtm

@feiskyer

This comment has been minimized.

Member

feiskyer commented Mar 29, 2018

/retest

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Mar 29, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 29, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, feiskyer, yujuhong

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 29, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 29, 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 675f270 into kubernetes:master Mar 29, 2018

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation feiskyer 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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@feiskyer feiskyer deleted the feiskyer:host-path branch Mar 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment