Fix directory mismatch for `volume.SetVolumeOwnership()` #117022

mpatlasov · 2023-03-30T20:53:46Z

In most cases dir arg of SetUpAt() method of volume.Mounter interface is the same as mounter.GetPath() because we usually call SetUpAt() from SetUp() like this:

func (ed *emptyDir) SetUp(mounterArgs volume.MounterArgs) error {
	return ed.SetUpAt(ed.GetPath(), mounterArgs)
}

(this example is from volume/emptydir/empty_dir.go, but there are plenty other examples like that in volume/*)

However, there is currently one exception. This is from volume/projected/projected.go:

	if err := wrapped.SetUpAt(dir, mounterArgs); err != nil {
		return err
	}

(see

kubernetes/pkg/volume/projected/projected.go

Line 203 in 96306f1

if err := wrapped.SetUpAt(dir, mounterArgs); err != nil {

)

In this case dir is not equal to wrapped.GetPath() and volume.SetVolumeOwnership() fails when called from SetUpAt() of wrapped volume:

lstat /var/lib/kubelet/pods/a2f6e58f-7edf-4c48-a97c-ef1b8fd3caf6/volumes/kubernetes.io~empty-dir/wrapped_kube-api-access-knvkv: no such file or directory

To fix the issue let's pass dir arg to volume.SetVolumeOwnership() explicitly, and use it instead of mounter.GetPath().

What type of PR is this?

/kind bug
/sig storage

What this PR does / why we need it:

Numerous failed volume.SetVolumeOwnership() calls pollute metric storage_operation_duration_seconds_count for emptyDir volumes.

Which issue(s) this PR fixes:

Fixes #117021

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Number of errors reported to the metric `storage_operation_duration_seconds_count` for emptyDir decreased significantly because previously one error was reported for each projected volume created.

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

k8s-ci-robot · 2023-03-30T20:53:51Z

Please note that we're already in Test Freeze for the release-1.27 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.27.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Thu Mar 30 16:31:51 UTC 2023.

k8s-ci-robot · 2023-03-30T20:53:54Z

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.

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.

k8s-ci-robot · 2023-03-30T20:53:55Z

Hi @mpatlasov. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

mpatlasov · 2023-03-30T20:54:12Z

/assign @jsafrane

jsafrane · 2023-04-03T09:30:25Z

/ok-to-test

jsafrane · 2023-04-03T09:46:10Z

@mpatlasov you need to update SetVolumeOwnership params for non-Linux platforms, https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/volume_unsupported.go

In most cases `dir` arg of `SetUpAt()` method of `volume.Mounter` interface is the same as `mounter.GetPath()` because we usually call `SetUpAt()` from `SetUp()` like this:" ``` func (ed *emptyDir) SetUp(mounterArgs volume.MounterArgs) error { return ed.SetUpAt(ed.GetPath(), mounterArgs) } ``` (this example is from `volume/emptydir/empty_dir.go`, but there are plenty other examples like that in `volume/*`) However, there is currently one exception. This is from `volume/projected/projected.go`: ``` if err := wrapped.SetUpAt(dir, mounterArgs); err != nil { return err } ``` (see https://github.com/kubernetes/kubernetes/blob/96306f144a3c917a97fe27c9ad5dd89e4213f741/pkg/volume/projected/projected.go#L203) In this case `dir` is not equal to `wrapped.GetPath()` and `volume.SetVolumeOwnership()` fails when called from `SetUpAt()` of wrapped volume: ``` lstat /var/lib/kubelet/pods/a2f6e58f-7edf-4c48-a97c-ef1b8fd3caf6/volumes/kubernetes.io~empty-dir/wrapped_kube-api-access-knvkv: no such file or directory ``` To fix the issue let's pass `dir` arg to `volume.SetVolumeOwnership()` explicitly, and use it instead of `mounter.GetPath()`.

gnufied · 2023-04-03T19:49:51Z

/lgtm

k8s-ci-robot · 2023-04-03T19:49:58Z

LGTM label has been added.

Git tree hash: 6c57e3b05a9f5a6378d1991de6bdc56b12033f20

jsafrane · 2023-04-04T13:37:45Z

/retest

jsafrane · 2023-04-04T14:49:08Z

/approve
/retest

The unit test failures are not related

k8s-ci-robot · 2023-04-04T14:49:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, mpatlasov

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:

~~pkg/volume/OWNERS~~ [jsafrane]

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

humblec · 2023-04-25T07:19:38Z

@mpatlasov can you also file 1.24 cherrypick PR as its the same code path and affected with this bug?

…17022-upstream-release-1.25 Automated cherry pick of #117022: Fix directory mismatch for `volume.SetVolumeOwnership()`

…17022-upstream-release-1.24 Automated cherry pick of #117022: Fix directory mismatch for `volume.SetVolumeOwnership()`

…17022-upstream-release-1.26 Automated cherry pick of #117022: Fix directory mismatch for `volume.SetVolumeOwnership()`

…17022-upstream-release-1.27 Automated cherry pick of #117022: Fix directory mismatch for `volume.SetVolumeOwnership()`

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 30, 2023

k8s-ci-robot requested a review from cofyc March 30, 2023 20:53

k8s-ci-robot requested a review from gnufied March 30, 2023 20:53

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 30, 2023

k8s-ci-robot assigned jsafrane Mar 30, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 3, 2023

mpatlasov force-pushed the fix-dir-mismatch-for-SetVolumeOwnership branch from cba4aa6 to 0a37f09 Compare April 3, 2023 19:36

k8s-ci-robot assigned gnufied Apr 3, 2023

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

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

k8s-ci-robot merged commit 8722ab2 into kubernetes:master Apr 12, 2023

k8s-ci-robot added this to the v1.28 milestone Apr 12, 2023

mpatlasov mentioned this pull request Apr 25, 2023

Automated cherry pick of #117022: Fix directory mismatch for volume.SetVolumeOwnership() #117599

Merged

k8s-ci-robot added a commit that referenced this pull request May 3, 2023

Merge pull request #117577 from mpatlasov/automated-cherry-pick-of-#1…

ff0de1e

…17022-upstream-release-1.25 Automated cherry pick of #117022: Fix directory mismatch for `volume.SetVolumeOwnership()`

k8s-ci-robot added a commit that referenced this pull request May 3, 2023

Merge pull request #117599 from mpatlasov/automated-cherry-pick-of-#1…

937a41f

…17022-upstream-release-1.24 Automated cherry pick of #117022: Fix directory mismatch for `volume.SetVolumeOwnership()`

k8s-ci-robot added a commit that referenced this pull request May 3, 2023

Merge pull request #117575 from mpatlasov/automated-cherry-pick-of-#1…

426bb5b

…17022-upstream-release-1.26 Automated cherry pick of #117022: Fix directory mismatch for `volume.SetVolumeOwnership()`

k8s-ci-robot added a commit that referenced this pull request May 3, 2023

Merge pull request #117574 from mpatlasov/automated-cherry-pick-of-#1…

23a15d0

…17022-upstream-release-1.27 Automated cherry pick of #117022: Fix directory mismatch for `volume.SetVolumeOwnership()`

mpatlasov mentioned this pull request Dec 22, 2023

REQUEST: New membership for mpatlasov kubernetes/org#4636

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix directory mismatch for `volume.SetVolumeOwnership()` #117022

Fix directory mismatch for `volume.SetVolumeOwnership()` #117022

mpatlasov commented Mar 30, 2023 •

edited

Loading

k8s-ci-robot commented Mar 30, 2023

k8s-ci-robot commented Mar 30, 2023

k8s-ci-robot commented Mar 30, 2023

mpatlasov commented Mar 30, 2023

jsafrane commented Apr 3, 2023

jsafrane commented Apr 3, 2023

gnufied commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

jsafrane commented Apr 4, 2023

jsafrane commented Apr 4, 2023

k8s-ci-robot commented Apr 4, 2023

humblec commented Apr 25, 2023

Fix directory mismatch for volume.SetVolumeOwnership() #117022

Fix directory mismatch for volume.SetVolumeOwnership() #117022

Conversation

mpatlasov commented Mar 30, 2023 • edited Loading

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.:

k8s-ci-robot commented Mar 30, 2023

k8s-ci-robot commented Mar 30, 2023

k8s-ci-robot commented Mar 30, 2023

mpatlasov commented Mar 30, 2023

jsafrane commented Apr 3, 2023

jsafrane commented Apr 3, 2023

gnufied commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

jsafrane commented Apr 4, 2023

jsafrane commented Apr 4, 2023

k8s-ci-robot commented Apr 4, 2023

humblec commented Apr 25, 2023

Fix directory mismatch for `volume.SetVolumeOwnership()` #117022

Fix directory mismatch for `volume.SetVolumeOwnership()` #117022

mpatlasov commented Mar 30, 2023 •

edited

Loading