search

containers / docker-lvm-plugin

Docker volume plugin for LVM volumes
GNU Lesser General Public License v3.0
155 stars 64 forks source link

Add `fs` option to set filesystem type! #85

Closed justmisam closed 3 years ago

justmisam commented 4 years ago

I really want to add this arg in config file but the config parser are too ugly in this project so I gave up and just added an option arg!

shishir-a412ed commented 4 years ago

@misamplus Just curious why do you want to make the filesystem (fs) configurable? Do you have use cases where you want to use other filesystems too? like ext4

Not contending the PR, but would like to understand the use case first. Also, what do you mean by config parser are too ugly :) This is a docker plugin based on the docker plugin library so we have to adhere to the spec (interface) provided by docker.

justmisam commented 4 years ago

@shishir-a412ed yes, I want to use other filesystems. I know that I can change the filesystem after creating the volume but I think it's good to set it first. About config parser, add a new config variable is not easy! I see utils.getVolumegroupName and it doesn't use variables name to get value of them; It gets the first variable definition and only check variable name is in valid list or not! so adding a new variable need to a lot of change in the code.

shishir-a412ed commented 4 years ago

Not sure if I completely follow, but getVolumeGroupName is used for docker-lvm-plugin configuration parameters, and right now there is only one VOLUME_GROUP.

On the contrary, fs is a volume level parameter (which can be set on a per-volume basis) so not sure what are you trying to do? Or do you want to set fs at the plugin level and that filesystem to be used for all LVM volumes?

justmisam commented 4 years ago

@shishir-a412ed I think people choose a filesystem for all volumes as default and if they want to other filesystem, they change it. Perhaps it's good to have a default fs in the plugin level config and a fs in volume level parameter.

shishir-a412ed commented 4 years ago

@misamplus I feel making the fs configurable could open up a pandora's box, and users can start passing random filesystems they are not knowledgable about. We can start with a whitelist of acceptable filesystems. e.g. [xfs, ext3, ext4]

Right now, since there is only one property VOLUME_GROUP in /etc/docker/docker-lvm-plugin config file, every time a method needs to get the volume group name it calls getVolumeGroupName which goes and reads the file. This is not very optimal in terms of complexity and could be optimized too.

I am thinking:

1) We can have a map[string]string populated at daemon startup i.e at the daemon startup, it will load both VOLUME_GROUP and FILESYSTEM into this map.

2) We can replace getVolumeGroupName with getValue(key string) (string, error) which can take any key and returns the value from the map.

3) For Filesystem lookup, it will validate the value against the whitelist and if it's an acceptable filesystem it will allow the creation of it e.g. mkfs.xfs or mkfs.ext4

I am okay exposing it both at the plugin level (in the /etc/docker/docker-lvm-plugin config) and as a volume level parameter.

Feel free to open a PR (or update this PR) and I can review it. Also, let me know if you want me to add this feature.

shishir-a412ed commented 3 years ago

@justmisam After contemplating a bit more, I feel it's not a good idea to make filesystem configurable. Closing this for now.