containers / podman

Podman: A tool for managing OCI containers and pods.
https://podman.io
Apache License 2.0
23.33k stars 2.38k forks source link

Remove the unused machine volume-driver #23145

Closed afbjorklund closed 2 months ago

afbjorklund commented 3 months ago

The driver is now hardcoded again, and there can only be one type of mounts at a time (which one changes over time)

Revert "Make it possible to select the volume driver" This reverts commit 6630e5cf66cf76aefcfe9caebe5df4f37dd0bdd5.

The driver is now always virtiofs(d).

Previously it was always virtfs (9p).

Does this PR introduce a user-facing change?

Remove the --volume-driver flag from podman machine init.

/cc @baude


The flag was introduced, so that users could choose between sshfs and virtfs drivers:

Mostly because some operating systems do not support 9p filesystems, such as RHEL

The real name should have been "reverse-sshfs", since it is calling back (to sftp on host)

rhatdan commented 3 months ago

LGTM @baude @ashley-cui PTAL

afbjorklund commented 3 months ago

Does this count as a breaking change, should we deprecate this first?

It was a breaking change already in 5.0, when the flag got disconnected...

commit 9bb191df51c7f2d242835e2af0bfa19d781b2256 ("70 files changed, 2372 insertions(+), 2473 deletions(-)")

But there were never any options for the flag anyway, so it was a no-op.

       var volumeType string
       switch opts.VolumeDriver {
       // "" is the default volume driver
       case "virtfs", "":
               volumeType = VolumeTypeVirtfs
       default:
               return fmt.Errorf("unknown volume driver: %s", opts.VolumeDriver)
       }

But if anyone is actually using it, it would break their command line.

Can probably remove it as the same time as changing to virtiofs, though? (5.2)

Otherwise it would just be a lie, since the actual implementation has changed.

mheon commented 3 months ago

Maybe mark the flag as hidden for a few releases (print a warning on use) and remove in ~5.4 timeframe later this year?

afbjorklund commented 3 months ago

Maybe mark the flag as hidden for a few releases (print a warning on use) and remove in ~5.4 timeframe later this year?

Wouldn't you have to change the value of it then? It would be weird to continue using --volume-driver=virtfs in 5.2

EDIT: Oh, you mean leave it as a no-op (like it is now) but just hide it from the init flags and the documentation

rhatdan commented 2 months ago

@mheon @ashley-cui PTANL?

afbjorklund commented 2 months ago

I marked it as "deprecated" instead, even though it was removed in Podman 5.0 already.

It is still hidden (and the docs removed), but now you get a warning if you try to use it:

Flag --volume-driver has been deprecated, will be ignored

rhatdan commented 2 months ago

/approve /lgtm

openshift-ci[bot] commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund, rhatdan

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](https://github.com/containers/podman/blob/main/OWNERS)~~ [rhatdan] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment