Enable Mount Propagation as a Optional Feature

[elijah-rou]

Proposal: Feature-Gated Mount Propagation

At my org we use the JuiceFS filesystem internally, accessed through PVCs. In order to recover the mount point in case of a network drop, it is required that the pod set mountPropagation to HostToContainer or Bidirectional for the volumeMount to facilitate this functionality. Currently, Knative does not allow mountPropagation to be used. Since it's a feature that should have care when using, this proposal is to to add the ability to use Mount Propagation gated under an optional feature the user will have to enable explicitly. I have implemented this internally already, but think this could be useful to the broader community.

Add the ability to use mountPropagation for volumeMounts, gated under kubernetes.podspec-mount-propagation

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: elijah-rou / name: Elijah Roussos (c1238e6, e2b64ea)

[knative-prow]

Welcome @elijah-rou! It looks like this is your first PR to knative/serving 🎉

[knative-prow]

Hi @elijah-rou. Thanks for your PR.

I'm waiting for a knative 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-sigs/prow repository.

[dprotaso]

/ok-to-test

[dprotaso]

/lgtm
/approve

thanks for the change!

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, elijah-rou

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:

• OWNERS [dprotaso]
• pkg/apis/OWNERS [dprotaso]

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

[dprotaso]

please take a look at the unit test failure

[skonto]

In pkg/reconciler/route/resources/service_test.go pls add the corresponding flag.

[skonto]

As in #15669 pls add the right validation for this new flag. Also pls you will need to create some docs in knative/docs repo.

[skonto]

/hold

[elijah-rou]

Will fix everything today :) docs PR will probably only come tomorrow

[elijah-rou]

As in #15669 pls add the right validation for this new flag. Also pls you will need to create some docs in knative/docs repo.

Mostly done. Just one comment with this. What validation needs to be done? I don't believe there are specific restrictions using mountPropagation beyond specifying an appropriate mode (None, HostToContainer, Bidirectional), validation which should be taken care of by K8s

[knative-prow]

New changes are detected. LGTM label has been removed.

[skonto]

What validation needs to be done?

Fail when user uses the feature but the flag is disabled, for example for the hostPath we have:

	if volume.HostPath != nil && features.PodSpecVolumesHostPath != config.Enabled {
		errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("HostPath volume support is disabled, "+
			"but found HostPath volume %s", volume.Name)})
	}

There is more to it in the link.

[skonto]

I don't believe there are specific restrictions using mountPropagation beyond specifying an appropriate mode (None, HostToContainer, Bidirectional), validation which should be taken care of by K8s

There are actually see here, especially if we want to fail fast:

	if *mountPropagation == core.MountPropagationBidirectional && !privileged {
		allErrs = append(allErrs, field.Forbidden(fldPath, "Bidirectional mount propagation is available only to privileged containers"))
	}

...

if we add support for some other  flags there are more restrictions:

	case core.RecursiveReadOnlyEnabled, core.RecursiveReadOnlyIfPossible:
....
		if mount.MountPropagation != nil && *mount.MountPropagation != core.MountPropagationNone {
			allErrs = append(allErrs, field.Forbidden(fldPath, "may only be specified when mountPropagation is None or not specified"))
		}

[elijah-rou]

I don't believe there are specific restrictions using mountPropagation beyond specifying an appropriate mode (None, HostToContainer, Bidirectional), validation which should be taken care of by K8s

There are actually see here, especially if we want to fail fast:

	if *mountPropagation == core.MountPropagationBidirectional && !privileged {
		allErrs = append(allErrs, field.Forbidden(fldPath, "Bidirectional mount propagation is available only to privileged containers"))
	}

...

if we add support for some other  flags there are more restrictions:

	case core.RecursiveReadOnlyEnabled, core.RecursiveReadOnlyIfPossible:
....
		if mount.MountPropagation != nil && *mount.MountPropagation != core.MountPropagationNone {
			allErrs = append(allErrs, field.Forbidden(fldPath, "may only be specified when mountPropagation is None or not specified"))
		}

Sorry, didn't realise about the privileged field. Will do it.

What validation needs to be done?

Fail when user uses the feature but the flag is disabled, for example for the hostPath we have:

	if volume.HostPath != nil && features.PodSpecVolumesHostPath != config.Enabled {
		errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("HostPath volume support is disabled, "+
			"but found HostPath volume %s", volume.Name)})
	}

There is more to it in the link.

Sure I'll get this done as well.

[codecov]

Codecov Report

Attention: Patch coverage is 87.09677% with 4 lines in your changes missing coverage. Please review.

Project coverage is 75.06%. Comparing base (c09ff6c) to head (e2b64ea).

Files with missing lines	Patch %	Lines
cmd/schema-tweak/overrides.go	0.00%	4 Missing ⚠️

❌ Your project check has failed because the head coverage (75.06%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #15758   +/-   ##
=======================================
  Coverage   75.05%   75.06%           
=======================================
  Files         222      222           
  Lines       18075    18098   +23     
=======================================
+ Hits        13566    13585   +19     
- Misses       4158     4162    +4     
  Partials      351      351           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.