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 (100ade5)

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

[knative-prow]

@elijah-rou: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
unit-tests_serving_main	100ade5	link	true	/test unit-tests
upgrade-tests_serving_main	100ade5	link	true	/test upgrade-tests
istio-latest-no-mesh_serving_main	100ade5	link	true	/test istio-latest-no-mesh

Your PR dashboard.

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. I understand the commands that are listed here.

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