Don't guess SELinux support on error #105934

jsafrane · 2021-10-27T09:58:56Z

When GetSELinuxSupport() fails, don't assume a mounted filesystem does not support SELinux at all. Try again instead in the next SetUp retry.

This may hurt performance a bit, since kubelet will call NodePublishVolume again, but it's better than providing wrong information to the container runtime that will then skip relabeling of the volume.

What type of PR is this?

/kind bug

Which issue(s) this PR fixes:

Fixes #105933

Does this PR introduce a user-facing change?

Fixed applying of SELinux labels to CSI volumes on very busy systems (with "error checking for SELinux support: could not get consistent content of /proc/self/mountinfo after 3 attempts")

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

k8s-ci-robot · 2021-10-27T09:59:04Z

@jsafrane: 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 · 2021-10-27T09:59:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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

When GetSELinuxSupport() fails, don't assume a mounted filesystem does not support SELinux at all. Try again instead in the next SetUp retry. This may hurt performance a bit, since kubelet will call NodePublishVolume again, but it's better than providing wrong information to the container runtime that will then skip relabeling of the volume.

mattcary · 2021-10-28T13:32:59Z

/lgtm

k8s-triage-robot · 2021-10-28T16:01:54Z

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 · 2021-10-28T18:45:04Z

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

gnufied · 2021-10-28T19:41:13Z

pkg/volume/csi/csi_mounter.go

@@ -276,7 +276,8 @@ func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error

 	c.supportsSELinux, err = c.kubeVolHost.GetHostUtil().GetSELinuxSupport(dir)
 	if err != nil {
-		klog.V(2).Info(log("error checking for SELinux support: %s", err))
+		// The volume is mounted. Return UncertainProgressError, so kubelet will unmount it when user deletes the pod.
+		return volumetypes.NewUncertainProgressError(fmt.Sprintf("error checking for SELinux support: %s", err))
 	}


I wonder if this will cause regression in environments that don't even have selinux enabled. Could we check if host supports selinux before even querying mount info, so as hosts that don't support selinux do not incur the potential overhead of reading mount table?

Added a check for SELinux support in the OS via pkg/util/selinux.SELinuxEnabled`, which should be cached after the first invocation.

gnufied · 2021-10-28T19:41:46Z

Putting on hold to address comment above.

/hold

Skip parsing of /proc/mounts if SELinux is disabled on the machine.

Previously, an error from GetSELinuxSupport was treated as a filesystem did not support SELinux. Now the error is treated separately.

jsafrane · 2021-11-01T14:13:55Z

Added SELinux detection + fixed unit tests not to return error from GetSELinuxSupport.

gnufied · 2021-11-05T14:01:33Z

/hold cancel
/lgtm

jsafrane · 2021-11-05T17:20:09Z

/retest
vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy unit test failed?

jsafrane · 2021-11-09T12:00:11Z

/priority important-soon

…5934-upstream-release-1.20 Automated cherry pick of #105934: Don't guess SELinux support on error

…5934-upstream-release-1.21 Automated cherry pick of #105934: Don't guess SELinux support on error

…5934-upstream-release-1.22 Automated cherry pick of #105934: Don't guess SELinux support on error

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 27, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2021

jsafrane force-pushed the dont-ignore-selinux branch from 722d99f to 186810e Compare October 27, 2021 10:00

k8s-ci-robot requested review from mattcary and saikat-royc October 27, 2021 10:00

jsafrane mentioned this pull request Oct 27, 2021

Volume manager re-conciler takes too long to reconcile under high pressure and large number of subpaths #105642

Closed

k8s-ci-robot assigned mattcary Oct 28, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2021

gnufied reviewed Oct 28, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2021

jsafrane added 2 commits November 1, 2021 14:32

Add shortcut for SELinux detection

7b07b1e

Skip parsing of /proc/mounts if SELinux is disabled on the machine.

Fixed unit test SELinux support

bc0c1bf

Previously, an error from GetSELinuxSupport was treated as a filesystem did not support SELinux. Now the error is treated separately.

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 1, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2021

k8s-ci-robot assigned gnufied Nov 5, 2021

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

k8s-ci-robot merged commit 6d9008b into kubernetes:master Nov 5, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 5, 2021

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 9, 2021

This was referenced Nov 9, 2021

Automated cherry pick of #105934: Don't guess SELinux support on error #106259

Merged

Automated cherry pick of #105934: Don't guess SELinux support on error #106260

Merged

Automated cherry pick of #105934: Don't guess SELinux support on error #106261

Merged

k8s-ci-robot added a commit that referenced this pull request Nov 9, 2021

Merge pull request #106261 from jsafrane/automated-cherry-pick-of-#10…

bca706b

…5934-upstream-release-1.20 Automated cherry pick of #105934: Don't guess SELinux support on error

k8s-ci-robot added a commit that referenced this pull request Nov 9, 2021

Merge pull request #106260 from jsafrane/automated-cherry-pick-of-#10…

90740dd

…5934-upstream-release-1.21 Automated cherry pick of #105934: Don't guess SELinux support on error

k8s-ci-robot added a commit that referenced this pull request Nov 9, 2021

Merge pull request #106259 from jsafrane/automated-cherry-pick-of-#10…

4b626f8

…5934-upstream-release-1.22 Automated cherry pick of #105934: Don't guess SELinux support on error

openshift-ci-robot mentioned this pull request Nov 15, 2021

[4.10] Bug 2017276: UPSTREAM: 105934: Don't guess SELinux support on error openshift/kubernetes#1052

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't guess SELinux support on error #105934

Don't guess SELinux support on error #105934

jsafrane commented Oct 27, 2021 •

edited

Loading

k8s-ci-robot commented Oct 27, 2021

k8s-ci-robot commented Oct 27, 2021

mattcary commented Oct 28, 2021

k8s-triage-robot commented Oct 28, 2021

k8s-triage-robot commented Oct 28, 2021

gnufied Oct 28, 2021

jsafrane Nov 1, 2021

gnufied commented Oct 28, 2021

jsafrane commented Nov 1, 2021

gnufied commented Nov 5, 2021

jsafrane commented Nov 5, 2021

jsafrane commented Nov 9, 2021

Don't guess SELinux support on error #105934

Don't guess SELinux support on error #105934

Conversation

jsafrane commented Oct 27, 2021 • edited Loading

What type of PR is this?

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Oct 27, 2021

k8s-ci-robot commented Oct 27, 2021

mattcary commented Oct 28, 2021

k8s-triage-robot commented Oct 28, 2021

k8s-triage-robot commented Oct 28, 2021

gnufied Oct 28, 2021

Choose a reason for hiding this comment

jsafrane Nov 1, 2021

Choose a reason for hiding this comment

gnufied commented Oct 28, 2021

jsafrane commented Nov 1, 2021

gnufied commented Nov 5, 2021

jsafrane commented Nov 5, 2021

jsafrane commented Nov 9, 2021

jsafrane commented Oct 27, 2021 •

edited

Loading