design-proposal: Adjust virtiofs feature gates #313

jcanocan · 2024-07-23T10:17:12Z

What this PR does / why we need it:
This design document explains how and why virtiofs feature gates should be adjusted. The main purpose is to avoid mixing feature control and configuration with feature enablement.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # CNV-39324

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note:

Added proposal to introduce `EnableVirtioFsConfigVolumes` and `EnableVirtioFsPVC` feature gates.

jcanocan · 2024-07-23T10:17:33Z

/cc @germag

jcanocan · 2024-07-23T10:20:52Z

/cc @EdDev

jcanocan · 2024-08-05T10:35:14Z

/cc @stu-gott

EdDev

Thank you for posting this and sorry for the time it took me to feedback.

EdDev · 2024-08-05T12:37:16Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+## Motivation
+
+Currently, the usage of feature gates are mixed with the feature control. The feature gate `ExperimentalVirtiofsSupport`
+controls whether virtiofsd runs as root or not. Moreover, there is no way to disable it completely and this feature is


Where virtiofsd runs?

Virtiofsd runs inside a separate container in the virt-launcher pod. I've added the detail in the document.

EdDev · 2024-08-05T12:44:57Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+- Drop the usage of `ExperimentalVirtiofsSupport` feature gate, marking it as deprecated.
+- Add feature gate `EnableVirtioFsConfigVolumes`, which will enable the sharing of ConfigMaps, Secrets, DownwardAPI and ServiceAccount.
+- Add feature gate `EnableVirtioFsPVC`, which will enable the sharing of PVCs.


I think these are implementation details, not goals.

A goal could be something like: Control, per cluster, the ability to use virtiofs with PVC.
But in my phrasing, it is a configuration knob not a FG one, so it is probably not a fit.

You are right. I've rephrased the section.

EdDev · 2024-08-05T12:48:43Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+As a VM owner, I want to share Secrets, ConfigMaps, ServiceAccounts and/or DownwardAPI in a dynamic way.
+
+As a VM owner, I want to share PVCs with one or multiple VMs and be able to read and/or write them.


I think there is a need for a full story, end to end.
E.g. what the user wants to do and what he needs to change/update/do in order to accomplish his need.

What are the scenarios he wants to be aware of or not be aware of (e.g. distinguish between all these options).

I've extended the user stories a bit.

EdDev · 2024-08-05T12:51:41Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+Two feature gates will be introduced as a means to granular support virtiofs functionalities:
+- `EnableVirtioFsConfigVolumes` to allow sharing ConfigMaps, Secrets, DownwardAPI and ServiceAccount.
+- `EnableVirtioFsPVC` to allow sharing PVCs.
+The feature gates may start its path to GA once a VM will be able to live migrate while sharing data with virtiofs.


This means there will be no control on the options once GA is reached (because the FG will get removed).
If this is the intention, please be explicit about it.

As I see it, the FG are just means to phase the work done and to allow functionality that is not yet stable.
Once it is stable, we drop them.

Yes. The intention is to drop those FGs on GA. Included some text to explicitly reflect it.

EdDev · 2024-08-05T13:00:48Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+migration support will distinguish between different use cases:  sharing ConfigMaps, Secrets, DownwardAPI and
+ServiceAccounts and sharing PVCs. This distinction will be useful for a staggered and granular support.


I have hard time understanding why there is a need to split one FG into two.
Can you somehow express what is the problem with the current single FG?

EdDev · 2024-08-05T13:05:42Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+# Open Questions
+
+- Currently, it is possible to run rootful virtiofs containers.
+   - Should we support this use case at all?, since the possibility of running a rootful container will be dropped.


Are talking about a sidecar in the virt-launcher?
If so, my impression was that we do not support rootful pods, but we left a knob in a form of a FG (?) to still allow users to operate in a legacy rootful manner.

But then, I do not understand what will trigger continuing a feature that depends on such a rootful pod instead of just discontinue it or find an alternative.

Virtiofsd runs in a separate container in the launcher pod. The feature will be able to share ConfigMaps, secrets, etc. and PVCs without requiring a rootful container. Before including the required changes, the main pain point is to share PVCs, see: https://kubevirt.io/user-guide/storage/disks_and_volumes/#non-privileged-and-privileged-sharing-modes. However, as stated, the intention is to overtake those limitations, but some extra work is required. Until then, we do wonder if we should support rootful virtiofs containers to bring the user the possibility to overtake the limitations before required changes are onboarded.

Dropped this section.

kubevirt-bot · 2024-10-11T07:10:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jean-edouard for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

alicefr · 2024-10-11T08:47:48Z

@jcanocan the proposal looks good to me and is much more clear about the feature distinction.
A side note on my side, can we update the user-guide documentation with this distinction between PVCs and configmaps and secrets?

/lgtm

jcanocan · 2024-10-11T08:52:46Z

@jcanocan the proposal looks good to me and is much more clear about the feature distinction. A side note on my side, can we update the user-guide documentation with this distinction between PVCs and configmaps and secrets?

/lgtm

Sure, I will create a placeholder in the user-guide so that I do not forget about it. Thanks for the suggestion.

alicefr · 2024-10-14T11:04:33Z

@jcanocan I think this proposal is missing how we deal with kubevirt upgrade when the old feature gate is on.

alicefr · 2024-10-14T11:05:01Z

/lgtm cancel

xpivarc · 2024-10-17T08:01:34Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+## Update/Rollback Compatibility
+This is a breaking change, users using virtiofs in any way won’t be able to further use virtiofs without enabling the
+proper feature gate/s.


This is no go. There is no need to break anyone, we just deprecate the feature gate we use now but it if present it will mean that both new feature gates are on. Users will be advised to move to new features gates as the old one will be ignored in n+m release

Nice idea! Adjusted! Thanks for the suggestion.

Adjusted to allow the usage of ExperimentalVirtiofsSupport until 1.5. Later, it will be removed.

xpivarc · 2024-10-22T09:58:18Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+
+## Motivation
+Currently, the usage of feature gates are mixed with the feature control. The feature gate `ExperimentalVirtiofsSupport`
+controls whether virtiofsd runs as rootful container inside the launcher pod or not. Moreover, there is no way to


The Moreover, there is no way to disable it completely and this feature is not in GA. is little bit more cryptic than it could be. I believe what you are trying to say is: "The feature gate ExperimentalVirtiofsSupport
controls whether virtiofs can be used and it automatically runs as rootful container inside the launcher pod when needed. There is no way to only run virtiofs for usecases which doesn't require rootful virtiofsd or the the other way around."

xpivarc · 2024-10-22T09:58:58Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+disable it completely and this feature is not in GA.
+
+In order to GA virtiofs, VMs should be able to live migrate while sharing data with virtiofs. The virtiofsd’s live
+migration support will distinguish between different use cases:  sharing ConfigMaps, Secrets, DownwardAPI and


You already define term "ConfigVolume" so you can just use that.

You right. I have left it in Section "KubeVirt Feature Gates", in which we explain what the EnableVirtioFsConfigVolumes FG to avoid defining a term with the term itself.

xpivarc · 2024-10-22T10:01:24Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+migration support will distinguish between different use cases:  sharing ConfigMaps, Secrets, DownwardAPI and
+ServiceAccounts and sharing PVCs. 
+
+This distinction will be useful because in order to fully support sharing ConfigVolumes and PVCs while allowing


The first sentence actually doesn't justify the distinction. I would remove the "the distinction will` and move to the part where you explain that different set of changes are required for ConfigVolumes GA and PVC GA...

xpivarc · 2024-10-22T10:01:54Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+and granular support giving users time to test the ConfigVolumes sharing support and allowing the involved developers to
+get feedback before next steps to support sharing PVCs.
+
+## Goals


Should deprecation of the existing feature gate be a goal?

Yes, you right. Added.

xpivarc · 2024-10-22T10:05:36Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+## Update/Rollback Compatibility
+As stated before, `ExperimentalVirtiofsSupport` will be marked as deprecated. However, in order to do not disrupt current
+systems relaying in the `ExperimentalVirtiofsSupport` feature gate, it will allow users to share ConfigMaps, Secrets,
+DownwardAPI and ServiceAccount and PVCs until KubeVirt version 1.5. In versions equal or greater than 1.5, the feature gate


1.5 -> 1.6 as we are reaching FF of 1.4 and I don't believe this will make it.

awels

One typo but looks good otherwise.

awels · 2024-10-23T20:15:17Z

design-proposals/virtiofs-support/virtiofs-feature-gates.md

+
+## Update/Rollback Compatibility
+As stated before, `ExperimentalVirtiofsSupport` will be marked as deprecated. However, in order to do not disrupt current
+systems relaying in the `ExperimentalVirtiofsSupport` feature gate, it will allow users to share ConfigVolumes and PVCs


Suggested change

systems relaying in the `ExperimentalVirtiofsSupport` feature gate, it will allow users to share ConfigVolumes and PVCs

systems relying in the `ExperimentalVirtiofsSupport` feature gate, it will allow users to share ConfigVolumes and PVCs

Adjusted. Thanks for your review!

This design document explains how and why virtiofs feature gates should be adjusted. The main purpose is to avoid mixing feature control and configuration with feature enablement. Signed-off-by: Javier Cano Cano <jcanocan@redhat.com>

awels · 2024-10-24T12:00:49Z

/lgtm

jcanocan · 2024-10-24T12:27:00Z

@vladikr could you please take a look? Thanks in advance.

kubevirt-bot · 2024-10-31T12:37:09Z

Pull requests that are marked with lgtm should receive a review
from an approver within 1 week.

After that period the bot marks them with the label needs-approver-review.

/label needs-approver-review

xpivarc · 2024-11-18T15:24:06Z

/sig compute
/sig storage

kubevirt-bot requested review from aburdenthehand and jean-edouard July 23, 2024 10:17

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Jul 23, 2024

kubevirt-bot requested a review from germag July 23, 2024 10:17

jcanocan mentioned this pull request Jul 23, 2024

Deprecates ExperimentalVirtiofsSupport in favor of EnableVirtioFsConfigVolumes and EnableVirtioFsPVC FG kubevirt/kubevirt#11997

Open

8 tasks

kubevirt-bot requested a review from EdDev July 23, 2024 10:20

kubevirt-bot requested a review from stu-gott August 5, 2024 10:35

EdDev reviewed Aug 5, 2024

View reviewed changes

jcanocan force-pushed the virtiofs-fg branch from 75d512a to 2a97cd1 Compare August 6, 2024 09:30

alicefr mentioned this pull request Oct 9, 2024

Unable to write virtiofs filesystem without sudo kubevirt/kubevirt#13028

Closed

jcanocan force-pushed the virtiofs-fg branch from 2a97cd1 to 3d9f2ec Compare October 11, 2024 07:10

kubevirt-bot assigned alicefr Oct 11, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2024

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2024

xpivarc reviewed Oct 17, 2024

View reviewed changes

jcanocan force-pushed the virtiofs-fg branch 2 times, most recently from 1ddf18d to 9e5a78a Compare October 21, 2024 08:14

xpivarc reviewed Oct 22, 2024

View reviewed changes

jcanocan force-pushed the virtiofs-fg branch from 9e5a78a to ec82eac Compare October 22, 2024 12:33

awels reviewed Oct 23, 2024

View reviewed changes

design-proposal: Adjust virtiofs feature gates

3b6bbc6

This design document explains how and why virtiofs feature gates should be adjusted. The main purpose is to avoid mixing feature control and configuration with feature enablement. Signed-off-by: Javier Cano Cano <jcanocan@redhat.com>

jcanocan force-pushed the virtiofs-fg branch from ec82eac to 3b6bbc6 Compare October 24, 2024 07:15

kubevirt-bot assigned awels Oct 24, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2024

kubevirt-bot added the needs-approver-review Indicates that a PR requires a review from an approver. label Oct 31, 2024

kubevirt-bot added sig/compute sig/storage labels Nov 18, 2024

		As a VM owner, I want to share Secrets, ConfigMaps, ServiceAccounts and/or DownwardAPI in a dynamic way.

		As a VM owner, I want to share PVCs with one or multiple VMs and be able to read and/or write them.

		migration support will distinguish between different use cases: sharing ConfigMaps, Secrets, DownwardAPI and
		ServiceAccounts and sharing PVCs. This distinction will be useful for a staggered and granular support.

	systems relaying in the `ExperimentalVirtiofsSupport` feature gate, it will allow users to share ConfigVolumes and PVCs
	systems relying in the `ExperimentalVirtiofsSupport` feature gate, it will allow users to share ConfigVolumes and PVCs

design-proposal: Adjust virtiofs feature gates #313

Are you sure you want to change the base?

design-proposal: Adjust virtiofs feature gates #313

Conversation

jcanocan commented Jul 23, 2024

jcanocan commented Jul 23, 2024 • edited Loading

jcanocan commented Jul 23, 2024

jcanocan commented Aug 5, 2024

EdDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-bot commented Oct 11, 2024

alicefr commented Oct 11, 2024

jcanocan commented Oct 11, 2024 • edited Loading

alicefr commented Oct 14, 2024

alicefr commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels commented Oct 24, 2024

jcanocan commented Oct 24, 2024

kubevirt-bot commented Oct 31, 2024

xpivarc commented Nov 18, 2024

jcanocan commented Jul 23, 2024 •

edited

Loading

jcanocan commented Oct 11, 2024 •

edited

Loading