Kubelet should honour the VolumeAttributes which are reported by the volume plugin by carlory · Pull Request #126806 · kubernetes/kubernetes

carlory · 2024-08-20T08:50:32Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

pods.yaml

apiVersion: v1
kind: Pod
metadata:
  name: pod1
  namespace: default
spec:
  containers:
  - image: registry.k8s.io/e2e-test-images/echoserver:2.3
    imagePullPolicy: IfNotPresent
    name: test
    volumeMounts:
    - mountPath: /volume
      name: volume
  volumes:
  - image:
      pullPolicy: IfNotPresent
      reference: quay.io/crio/artifact:v1
    name: volume
---
apiVersion: v1
kind: Pod
metadata:
  name: pod2
  namespace: default
spec:
  containers:
  - image: registry.k8s.io/e2e-test-images/echoserver:2.3
    imagePullPolicy: IfNotPresent
    name: test
    volumeMounts:
    - mountPath: /volume
      name: volume
  volumes:
  - image:
      pullPolicy: IfNotPresent
      reference: quay.io/crio/artifact:v1
    name: volume
---
apiVersion: v1
kind: Pod
metadata:
  name: pod3
  namespace: default
spec:
  containers:
  - image: registry.k8s.io/e2e-test-images/echoserver:2.3
    imagePullPolicy: IfNotPresent
    name: test
    volumeMounts:
    - mountPath: /etc/hosts
      name: volume
  volumes:
  - image:
      pullPolicy: IfNotPresent
      reference: quay.io/crio/artifact:v1
    name: volume
---
apiVersion: v1
kind: Pod
metadata:
  name: pod4
  namespace: default
spec:
  containers:
  - image: registry.k8s.io/e2e-test-images/echoserver:2.3
    imagePullPolicy: IfNotPresent
    name: test
    volumeMounts:
    - mountPath: /etc/hosts
      name: volume
  volumes:
  - emptyDir: {}
    name: volume

pod1 and pod2 share the same image volume source type and are mounted to /volume in the container.
pod3 and pod4 mount the volume to /etc/hosts in the container but use different volume source types.

without this fix

pod3 is running but pod4 has CreateContainerError status.

(base) ➜  kubernetes git:(fix-image-volume-mount) kubectl get po
NAME   READY   STATUS                 RESTARTS   AGE
pod1   1/1     Running                0          29s
pod2   1/1     Running                0          29s
pod3   1/1     Running                0          29s
pod4   0/1     CreateContainerError   0          29s

pod1 mount with rw option

root@crio2-control-plane:/# crictl inspect 5c76497deb40c |  jq .status.mounts[0]
{
  "containerPath": "/volume",
  "gidMappings": [],
  "hostPath": "/var/lib/containers/storage/overlay/82099f56ba7822d8408ac8517b90bde2aa036ac32ee2dd3db810348302edd618/merged",
  "propagation": "PROPAGATION_PRIVATE",
  "readonly": false,
  "recursiveReadOnly": false,
  "selinuxRelabel": false,
  "uidMappings": []
}

root@crio2-control-plane:/# crictl inspect 5c76497deb40c | jq .info.runtimeSpec.mounts[10]
{
  "destination": "/volume",
  "type": "bind",
  "source": "/var/lib/containers/storage/overlay/82099f56ba7822d8408ac8517b90bde2aa036ac32ee2dd3db810348302edd618/merged",
  "options": [
    "rbind",
    "rprivate",
    "rw",
    "bind"
  ]
}

with this fix

pod3 and pod4 has CreateContainerError status.

(base) ➜  kubernetes git:(fix-image-volume-mount) kubectl get po
NAME   READY   STATUS                 RESTARTS   AGE
pod1   1/1     Running                0          44m
pod2   1/1     Running                0          44m
pod3   0/1     CreateContainerError   0          44m
pod4   0/1     CreateContainerError   0          44m

(base) ➜  kubernetes git:(fix-image-volume-mount) kubectl describe po pod3
Events:
  Type     Reason     Age                  From               Message
  ----     ------     ----                 ----               -------
  Normal   Scheduled  51m                  default-scheduler  Successfully assigned default/pod3 to crio-control-plane
  Normal   Pulled     50m (x8 over 51m)    kubelet            Container image "registry.k8s.io/e2e-test-images/echoserver:2.3" already present on machine
  Warning  Failed     50m (x8 over 51m)    kubelet            Error: container create failed: mount `/var/lib/containers/storage/overlay/82099f56ba7822d8408ac8517b90bde2aa036ac32ee2dd3db810348302edd618/merged` to `etc/hosts`: Not a directory
  Normal   Pulled     78s (x229 over 51m)  kubelet            Container image "quay.io/crio/artifact:v1" already present on machine

(base) ➜  kubernetes git:(fix-image-volume-mount) kubectl describe po pod4
Events:
  Type     Reason     Age                  From               Message
  ----     ------     ----                 ----               -------
  Normal   Scheduled  51m                  default-scheduler  Successfully assigned default/pod4 to crio-control-plane
  Warning  Failed     49m (x12 over 51m)   kubelet            Error: container create failed: mount `/var/lib/kubelet/pods/983847e3-109e-4a2e-89f6-5cd0fef3b4cc/volumes/kubernetes.io~empty-dir/volume` to `etc/hosts`: Not a directory
  Normal   Pulled     81s (x233 over 51m)  kubelet            Container image "registry.k8s.io/e2e-test-images/echoserver:2.3" already present on machine

pod1 mount with ro option

(base) ➜  kubernetes git:(fix-image-volume-mount) docker exec -it 92a9726219db bash
root@crio-control-plane:/# crictl inspect 90e489c2aa5e2 | jq .status.mounts[0]
{
  "containerPath": "/volume",
  "gidMappings": [],
  "hostPath": "/var/lib/containers/storage/overlay/82099f56ba7822d8408ac8517b90bde2aa036ac32ee2dd3db810348302edd618/merged",
  "propagation": "PROPAGATION_PRIVATE",
  "readonly": true,
  "recursiveReadOnly": false,
  "selinuxRelabel": false,
  "uidMappings": []
}

root@crio-control-plane:/# crictl inspect 90e489c2aa5e2 | jq .info.runtimeSpec.mounts[10]
{
  "destination": "/volume",
  "type": "bind",
  "source": "/var/lib/containers/storage/overlay/82099f56ba7822d8408ac8517b90bde2aa036ac32ee2dd3db810348302edd618/merged",
  "options": [
    "rbind",
    "rprivate",
    "ro",
    "bind"
  ]
}

Which issue(s) this PR fixes:

Related to kubernetes/enhancements#4639

Special notes for your reviewer:

Does this PR introduce a user-facing change?

1. When the kubelet constructs the cri mounts for the container which references an `image` volume source type, It passes the missing mount attributes to the CRI implementation, including `readOnly`, `propagation`, and `recursiveReadOnly`. When the readOnly field of the containerMount is explicitly set to false, the kubelet will take the `readOnly`as true to the CRI implementation because the image volume plugin requires the mount to be read-only. 
2. Fix a bug where the pod is unexpectedly running when the `image` volume source type is used and mounted to `/etc/hosts` in the container.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2024-08-20T08:50:42Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

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-sigs/prow repository.

carlory · 2024-08-20T08:50:44Z

/hold

carlory · 2024-08-20T09:03:55Z

/cc @jsafrane @gnufied @saschagrunert

saschagrunert · 2024-08-20T09:17:06Z

The refCount of the layer is being rapidly increased. What's wrong with my changes?

I tested that locally and every container creation will trigger a new mount, but that's just a counter and not a real system mount. I think that's not a big issue.

carlory · 2024-08-20T09:30:30Z

kubectl create -f pod1.yaml -f pod3.yaml

crio log:

root@crio-control-plane:/run/containers/storage/overlay-layers# cat mountpoints.json | jq .[0]
{
  "id": "82099f56ba7822d8408ac8517b90bde2aa036ac32ee2dd3db810348302edd618",
  "path": "/var/lib/containers/storage/overlay/82099f56ba7822d8408ac8517b90bde2aa036ac32ee2dd3db810348302edd618/merged",
  "count": 620
}
root@crio-control-plane:/run/containers/storage/overlay-layers# journalctl -u crio -e | grep 'Image ref to mount' | wc -l
5
root@crio-control-plane:/run/containers/storage/overlay-layers# journalctl -u crio -e | grep 'Image ID to mount' | wc -l
5

saschagrunert · 2024-08-20T10:00:56Z

/test pull-kubernetes-node-crio-cgrpv2-imagevolume-e2e

saschagrunert · 2024-08-20T10:32:33Z

@carlory we may have to change something SELinux related: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/126806/pull-kubernetes-node-crio-cgrpv2-imagevolume-e2e/1825835412535382016

waiting:
                message: 'relabel failed /var/lib/containers/storage/overlay/82099f56ba7822d8408ac8517b90bde2aa036ac32ee2dd3db810348302edd618/merged:
                  lsetxattr /var/lib/containers/storage/overlay/82099f56ba7822d8408ac8517b90bde2aa036ac32ee2dd3db810348302edd618/merged/dir:
                  read-only file system'
                reason: CreateContainerError

SergeyKanzhelev · 2024-09-13T20:17:47Z

 			vol.SELinuxLabeled = true
 			relabelVolume = true
 		}
-		hostPath, err := volumeutil.GetPath(vol.Mounter)


I am concerned of moving and changing so much code that is not under the feature gate. Was it added alongside the feature?

Ideally we want to minimize the number of potential regressions and keep existing codepaths as close to what they were without the feature gate as possible

I am concerned of moving and changing so much code that is not under the feature gate. Was it added alongside the feature?

It just adds a new conditon check for the previous code. It doesn't change the code itself. I add the feature-gate check in line 289. If the feature-gate is disabled, the image will be nil and old logic will be executed. @SergeyKanzhelev

SergeyKanzhelev · 2024-09-13T20:19:17Z

 			Name:              mount.Name,
 			ContainerPath:     containerPath,
 			HostPath:          hostPath,
+			Image:             image,


I wonder how it worked without the image before? What is the image field is being used for if it was not passed here before and nothing broke?

It aims to add all missing mount information to the runtime when the volume source is image and let them decide.

@SergeyKanzhelev some volume attributes reported by the image volume plugin, i.e. relabel, read-only, etc., aren't sent to the implementation of CRI runtime in previous kubelet implementation.

func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, hostDomain string, podIPs []string, podVolumes kubecontainer.VolumeMap, hu hostutil.HostUtils, subpather subpath.Interface, expandEnvs []kubecontainer.EnvVar, supportsRRO bool, imageVolumes kubecontainer.ImageVolumes) ([]kubecontainer.Mount, func(), error) { mountEtcHostsFile := shouldMountHostsFile(pod, podIPs) klog.V(3).InfoS("Creating hosts mount for container", "pod", klog.KObj(pod), "containerName", container.Name, "podIPs", podIPs, "path", mountEtcHostsFile) mounts := []kubecontainer.Mount{} var cleanupAction func() for i, mount := range container.VolumeMounts { // Check if the mount is referencing an OCI volume if imageVolumes != nil && utilfeature.DefaultFeatureGate.Enabled(features.ImageVolume) { if image, ok := imageVolumes[mount.Name]; ok { mounts = append(mounts, kubecontainer.Mount{ Name: mount.Name, ContainerPath: mount.MountPath, Image: image, }) continue } } ...skipped.. mounts = append(mounts, kubecontainer.Mount{ Name: mount.Name, ContainerPath: containerPath, HostPath: hostPath, ReadOnly: mount.ReadOnly || mustMountRO, RecursiveReadOnly: rro, SELinuxRelabel: relabelVolume, Propagation: propagation, }) } ...skipped.. return mounts, cleanupAction, nil

What's the result? Please see the without this fix section in my PR's description.

gnufied · 2024-09-13T21:08:45Z

After bind-mount stuff was fixed for selinux stuff - cri-o/cri-o#8521, what is the new behaviour? @carlory can you please confirm without your change and with your change?

carlory · 2024-09-14T04:46:34Z

After bind-mount stuff was fixed for selinux stuff - cri-o/cri-o#8521, what is the new behaviour? @carlory can you please confirm without your change and with your change?

@gnufied
This PR is not related to selinux issue, but that is detected by this PR when running the pull-kubernetes-node-crio-cgrpv2-imagevolume-e2e job, in order to make the job pass, the PR changes the volume attributes reported by the image volume plugin, see #126806 (comment) for more details. At that time, we think the issue is a known limitation for all volume plugins, so we open a PR for the website to document it. We also asked about the limitation with the sig-storage in the community meeting but got a different answer. Thanks a lot for all the help from the community. @saschagrunert raised a PR to fix it in the CRI-O repo and kubernetes/website#47588 was closed, so no new behavior is introduced and #126991 added a new test case for node e2e tests to verify the fix. The result is passed.

since cri-o/cri-o#8521 is merged, the root cause is fixed, so it won't occur again with or without this PR. Why not revert selinux configuration in this PR, please see #126806 (comment).

This PR mainly aims to add all missing mount information to the runtime and let them decide.

carlory · 2024-09-14T07:30:04Z

How any of those changes tested? Why no existing tests broke? Can you please add tests demonstrating the wrong behavior and the fix

No existing test is broken. I am adding new test.

carlory · 2024-09-14T09:33:42Z

for release notes, I would suggest to rephrase the first one from the SHOULD statement to the description on what was changed

updated.

SergeyKanzhelev · 2024-11-05T07:12:57Z

lost track of this PR. Now it only have test files in it. Perhaps a wrong force push?

carlory · 2024-11-05T07:32:38Z

I am concerned of moving and changing so much code that is not under the feature gate. Was it added alongside the feature?

Ideally we want to minimize the number of potential regressions and keep existing codepaths as close to what they were without the feature gate as possible

Copid from #126806 (comment)

In order to make the new test to check the new feature without the fix, I pushed the first commit to the same PR.

@SergeyKanzhelev I'm trying to update the makeMounts function to reduce the number of changed lines. I will push the second commit to the same PR once I achieve the above goal.

Edit:

New tests fail as expected without the fix.

https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/126806/pull-kubernetes-unit/1853696165627826176

carlory · 2024-11-05T08:40:16Z

/cc @saschagrunert @SergeyKanzhelev @gnufied

Please review it again.

Edit: I'm trying to reduce the number of changed lines but fail to achieve that. I give up and revert it to the previous state.

It just adds a new conditon check for the previous code. It doesn't change the code itself. I add the feature-gate check in line 304. If the feature-gate is disabled, the image will be nil and old logic will be executed.

… references an `image` volume source type, It passes the missing mount attributes to the CRI implementation, including `readOnly`, `propagation`, and `recursiveReadOnly`. When the readOnly field of the containerMount is explicitly set to false, the kubelet will take the `readOnly`as true to the CRI implementation because the image volume plugin requires the mount to be read-only. 2. Fix a bug where the pod is unexpectedly running when the `image` volume source type is used and mounted to `/etc/hosts` in the container.

SergeyKanzhelev

/lgtm
/approve

thank you for resolving all comments

k8s-ci-robot · 2024-11-05T21:10:37Z

LGTM label has been added.

Details

Git tree hash: 6e55785d79059f36a6f30a4b66504a8ec513e314

k8s-ci-robot · 2024-11-05T21:10:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlory, saschagrunert, SergeyKanzhelev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/kubelet/OWNERS~~ [SergeyKanzhelev]
~~pkg/volume/image/OWNERS~~ [SergeyKanzhelev]

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

Kubelet should honour the VolumeAttributes which are reported by the volume plugin

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Aug 20, 2024

k8s-ci-robot requested review from dims and sjenning August 20, 2024 08:51

carlory commented Aug 20, 2024

View reviewed changes

Comment thread pkg/kubelet/kubelet_pods.go

k8s-ci-robot requested review from gnufied, jsafrane and saschagrunert August 20, 2024 09:03

saschagrunert reviewed Aug 20, 2024

View reviewed changes

Comment thread pkg/kubelet/kubelet_pods.go Outdated

This comment was marked as outdated.

Sign in to view

k8s-ci-robot requested review from kerthcet and wzshiming August 20, 2024 10:03

carlory force-pushed the fix-image-volume-mount branch from 331e052 to 6d88f17 Compare August 20, 2024 10:38

SergeyKanzhelev reviewed Sep 13, 2024

View reviewed changes

carlory force-pushed the fix-image-volume-mount branch from 1a81cfa to d05abe1 Compare September 14, 2024 09:36

carlory force-pushed the fix-image-volume-mount branch from d05abe1 to 69d6e0e Compare November 5, 2024 06:37

TestMakeMounts: add new cases for the image volume feature

2529d7d

carlory force-pushed the fix-image-volume-mount branch from 69d6e0e to 2529d7d Compare November 5, 2024 07:09

k8s-ci-robot requested review from SergeyKanzhelev and saschagrunert November 5, 2024 08:40

carlory force-pushed the fix-image-volume-mount branch from a79af52 to b6c9c2d Compare November 5, 2024 11:47

fix pull-kubernetes-linter-hints

c6e6fad

SergeyKanzhelev approved these changes Nov 5, 2024

View reviewed changes

k8s-ci-robot assigned SergeyKanzhelev Nov 5, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2024

k8s-ci-robot merged commit b5e6456 into kubernetes:master Nov 5, 2024

k8s-ci-robot added this to the v1.32 milestone Nov 5, 2024

carlory deleted the fix-image-volume-mount branch November 6, 2024 02:05

pacoxu mentioned this pull request Nov 6, 2024

[Failing Test] gci-gce-alpha-enabled-default #128600

Closed

mhan8796 pushed a commit to mhan8796/kubernetes that referenced this pull request Jun 27, 2026

Merge pull request kubernetes#126806 from carlory/fix-image-volume-mount

e044130

Kubelet should honour the VolumeAttributes which are reported by the volume plugin

hakman mentioned this pull request Aug 14, 2026

etcd-manager: remove kops-utils-cp and use image volumes on all supported Kubernetes versions kubernetes/kops#18688

Merged

Conversation

carlory commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Aug 20, 2024

Uh oh!

carlory commented Aug 20, 2024

Uh oh!

Uh oh!

carlory commented Aug 20, 2024

Uh oh!

saschagrunert commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

carlory commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

saschagrunert commented Aug 20, 2024

Uh oh!

saschagrunert commented Aug 20, 2024

Uh oh!

SergeyKanzhelev Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

carlory Sep 14, 2024

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

carlory Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlory Sep 14, 2024

Choose a reason for hiding this comment

Uh oh!

gnufied commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlory commented Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlory commented Sep 14, 2024

Uh oh!

carlory commented Sep 14, 2024

Uh oh!

SergeyKanzhelev commented Nov 5, 2024

Uh oh!

carlory commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlory commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 5, 2024

Uh oh!

k8s-ci-robot commented Nov 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carlory commented Aug 20, 2024 •

edited

Loading

saschagrunert commented Aug 20, 2024 •

edited

Loading

carlory commented Aug 20, 2024 •

edited

Loading

carlory Sep 14, 2024 •

edited

Loading

gnufied commented Sep 13, 2024 •

edited

Loading

carlory commented Sep 14, 2024 •

edited

Loading

carlory commented Nov 5, 2024 •

edited

Loading

carlory commented Nov 5, 2024 •

edited

Loading