cri-api: fix comment lines about PROPAGATION_PRIVATE #115704

AkihiroSuda · 2023-02-11T16:08:59Z

The current CRI implementations treat PROPAGATION_PRIVATE as "rprivate", not "private":

However, this is not always true for cri-dockerd, which treats PROPAGATION_PRIVATE as noop and lets dockerd use its default propagation mode:

https://github.com/Mirantis/cri-dockerd/blob/v0.3.1/libdocker/helpers.go#L235-L236 (The "private is default" comment in L236 is inaccurate)

dockerd's default propagation mode is "rprivate" for most cases, but dockerd changes its default propagation mode to "rslave" when the mount source contains the daemon root (/var/lib/docker):

This behavior was introduced in Docker 18.03 (moby/moby#36055).

/kind documentation

What this PR does / why we need it:

Fix a comment line about PROPAGATION_PRIVATE because it is inaccurate.

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

NONE

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

A manifest to verify the behavior:

---
apiVersion: v1
kind: Pod
metadata:
  name: propagation-test1
spec:
  containers:
    - name: sleep
      image: busybox
      command: ['sleep', 'infinity']
      volumeMounts:
        - mountPath: /mnt
          name: mnt
          mountPropagation: None
          # The mount propagation `None` is translated to:
          # - cri-dockerd v0.3.0, with Docker v20.10.23: rprivate
          # - containerd v1.6.15: rprivate
          # - CRI-O v1.24.1: rprivate
  volumes:
    - name: mnt
      hostPath:
        path: /mnt
---
apiVersion: v1
kind: Pod
metadata:
  name: propagation-test2
spec:
  containers:
    - name: sleep
      image: busybox
      command: ['sleep', 'infinity']
      volumeMounts:
        - mountPath: /mnt
          name: mnt
          mountPropagation: None
          # The mount propagation `None` is translated to:
          # - cri-dockerd v0.3.0, with Docker v20.10.23: rslave
          # - containerd v1.6.15: rprivate
          # - CRI-O v1.24.1: rprivate
          #
          # Docker changes the default propagation to "rslave",
          # because the mount source (`/`) contains `/var/lib/docker`.
          # - https://github.com/moby/moby/blob/v20.10.23/daemon/volumes.go#L137-L143
          # - https://github.com/moby/moby/blob/v20.10.23/daemon/volumes_linux.go#L11-L36
          #
          # This behavior was introduced in Docker 18.03: https://github.com/moby/moby/pull/36055
          #
          # containerd and CRI-O do not automatically change the propagation:
          # - https://github.com/containerd/containerd/blob/v1.6.15/pkg/cri/opts/spec_linux.go#L181
          # - https://github.com/cri-o/cri-o/blob/v1.24.1/server/container_create_linux.go#L967
  volumes:
    - name: mnt
      hostPath:
        path: /

pacoxu · 2023-02-13T05:06:28Z

/lgtm
/priority important-longterm
/triage accepted
/assign @mrunalp @SergeyKanzhelev

k8s-ci-robot · 2023-02-13T05:06:35Z

LGTM label has been added.

Details

Git tree hash: 5b8c1c8a52870e291470d37cb23b675afd59f163

SergeyKanzhelev · 2023-02-13T19:41:16Z

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

I wonder if other runtimes will have the same problem if attempt to mount their root directories or some other relevant directories that are used to run container. I wonder if this comment can be generalized once fixed for other runtimes...

@mikebrow @mrunalp

I'd argue that cri-dockerd's behavior is wrong and insecure (but unfixable for compatibility sake).

It should have just raised an explicit error when the requested propagation ("None" in the yaml) is not applicable, as falling back to "HostToContainer" (aka "rslave") may result in leaking unexpected data to the container, etc.

containerd, CRI-O, etc. may print a warning when the mount source contains the CRI daemon root, but changing the propagation is insecure as explained above, and raising an error will break compatibility.

Updated the PR to clarify the expected behavior

pacoxu · 2023-02-14T05:14:03Z

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

typo: appicable/applicable?

derekwaynecarr

please see suggestion, thanks!

derekwaynecarr · 2023-03-14T14:45:03Z

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

Is the desire in the API comment change to say that its ok for cri providers to warn/error?

I would expect the CRI provider to publish its known issues separate from API definition. Can we remove the cri-dockerd notice as a result?

agree the cri-dockerd issue is probably better placed in user facing .. provider specific docs...

s/An implementation/A runtime/

Removed cri-dockerd notice

The current CRI implementations treat `PROPAGATION_PRIVATE` as "rprivate", not "private": - https://github.com/containerd/containerd/blob/v1.6.16/pkg/cri/opts/spec_linux.go#L181 - https://github.com/cri-o/cri-o/blob/v1.26.1/server/container_create_linux.go#L982 However, this is not always true for cri-dockerd, which treats `PROPAGATION_PRIVATE` as noop and lets dockerd use its default propagation mode: - https://github.com/Mirantis/cri-dockerd/blob/v0.3.1/libdocker/helpers.go#L235-L236 (The "private is default" comment in L236 is inaccurate) dockerd's default propagation mode is "rprivate" for most cases, but dockerd changes its default propagation mode to "rslave" when the mount source contains the daemon root (`/var/lib/docker`): - https://github.com/moby/moby/blob/v20.10.23/volume/mounts/linux_parser.go#L145 - https://github.com/moby/moby/blob/v20.10.23/daemon/volumes.go#L137-L143 - https://github.com/moby/moby/blob/v20.10.23/daemon/volumes_linux.go#L11-L36 This behavior was introduced in Docker 18.03 (moby/moby PR 36055). Related: - kubernetes/website PR 39385 - Mirantis/cri-dockerd PR 159 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2023-04-04T17:01:17Z

@derekwaynecarr PTAL

marquiz

/lgtm

k8s-ci-robot · 2023-04-12T07:32:45Z

LGTM label has been added.

Details

Git tree hash: e0eb4cc23988aa3a5496235fc35d05f51ea6a10e

samuelkarp · 2023-04-13T01:00:46Z

LGTM

mikebrow

LGTM

pacoxu · 2023-04-25T09:22:23Z

Ping @mrunalp @dchen1107 @derekwaynecarr

AkihiroSuda · 2023-05-09T04:42:34Z

@derekwaynecarr PTAL

k8s-ci-robot · 2023-05-09T15:42:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AkihiroSuda, mrunalp, pacoxu, 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:

~~staging/src/k8s.io/cri-api/pkg/OWNERS~~ [mrunalp]

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

k8s-triage-robot · 2023-05-09T17:59:47Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2023-05-09T20:00:43Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot requested review from bobbypage and odinuge February 11, 2023 16:09

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 11, 2023

k8s-ci-robot assigned mrunalp Feb 13, 2023

k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Feb 13, 2023

k8s-ci-robot assigned SergeyKanzhelev Feb 13, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 13, 2023

k8s-ci-robot assigned pacoxu Feb 13, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2023

saschagrunert approved these changes Feb 13, 2023

View reviewed changes

SergeyKanzhelev reviewed Feb 13, 2023

View reviewed changes

AkihiroSuda force-pushed the cri-rprivate branch from 02b1b3b to b17e353 Compare February 14, 2023 05:04

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2023

k8s-ci-robot requested review from SergeyKanzhelev, mrunalp and pacoxu February 14, 2023 05:04

pacoxu reviewed Feb 14, 2023

View reviewed changes

derekwaynecarr suggested changes Mar 14, 2023

View reviewed changes

AkihiroSuda force-pushed the cri-rprivate branch from a6d30ee to af95a76 Compare March 22, 2023 12:38

k8s-ci-robot removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 22, 2023

k8s-ci-robot requested a review from pacoxu March 22, 2023 12:38

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 22, 2023

k8s-ci-robot requested a review from SergeyKanzhelev March 22, 2023 12:38

AkihiroSuda requested review from derekwaynecarr and removed request for SergeyKanzhelev, bobbypage, mrunalp, odinuge and pacoxu March 30, 2023 06:00

marquiz reviewed Apr 12, 2023

View reviewed changes

k8s-ci-robot assigned marquiz Apr 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2023

mikebrow reviewed Apr 24, 2023

View reviewed changes

mrunalp approved these changes May 9, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2023

k8s-ci-robot merged commit 4c45313 into kubernetes:master May 9, 2023

k8s-ci-robot added this to the v1.28 milestone May 9, 2023

cri-api: fix comment lines about PROPAGATION_PRIVATE #115704

cri-api: fix comment lines about PROPAGATION_PRIVATE #115704

Uh oh!

Conversation

AkihiroSuda commented Feb 11, 2023

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!

pacoxu commented Feb 13, 2023

Uh oh!

k8s-ci-robot commented Feb 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Feb 14, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Apr 4, 2023

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 12, 2023

Uh oh!

samuelkarp commented Apr 13, 2023

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

pacoxu commented Apr 25, 2023

Uh oh!

AkihiroSuda commented May 9, 2023

Uh oh!

k8s-ci-robot commented May 9, 2023

Uh oh!

k8s-triage-robot commented May 9, 2023

Uh oh!

k8s-triage-robot commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

AkihiroSuda Feb 14, 2023 •

edited

Loading