[pve-devel] Make LXC code independent of storage plugin names

[Fabian Grünbichler]

On Mon, Sep 19, 2016 at 10:37:01AM +0300, Dmitry Petuhov wrote:
> 19.09.2016 08:51, Fabian Grünbichler wrote:
> > ------------------------------------------------------------------------
> > general remark, rest of comments inline:
> > 
> > the indentation is all messed up. I know our codebase is not very clean
> > in this regard anyway, but for new / touched code we try to keep / make
> > it clean. our indentation for perl code is a mix of tabs and spaces,
> > with tabs being 8 spaces long.
> > 
> > the first indentation level is indented with 4 spaces, the second with
> > 1 tab, the third with 1 tab and 4 spaces, the fourth with 2 tabs, and
> > so on. if you use vim, I recommend enabling listmode to easily
> > differentiate between the two.
> Ok. Configured my mcedit. Maybe it would be good idea to all coding style
> memo to https://pve.proxmox.com/wiki/Developer_Documentation ?
> 

yes, that would probably be a good idea :)

> > you drop the $mounted_dev assignment in the else branch. after a quick
> > search of the calling code, this will probably at least break quota
> > support for such volumes?
> Actually it's not. mountpoint_mount() is being called in list context only
> once (in PVE::API2::LXC) and only $use_loopdev is being used there. In other
> places it is being called in scalar context, or its returned values are
> ignored at all. So currently it brakes nothing. But OK, I can keep this
> assingment for future.

our LXC prestart hook uses it to setup the devices for quota support,
see src/lxc-pve-prestart-hook, lines 79-108:

	my $devlist_file = "/var/lib/lxc/$vmid/devices";
	unlink $devlist_file;
	my $devices = [];

	my $setup_mountpoint = sub {
	    my ($ms, $mountpoint) = @_;

	    #return if $ms eq 'rootfs';
	    my (undef, undef, $dev) = PVE::LXC::mountpoint_mount($mountpoint, $rootdir, $storage_cfg);
	    push @$devices, $dev if $dev && $mountpoint->{quota};
	};

	PVE::LXC::Config->foreach_mountpoint($conf, $setup_mountpoint);

	my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
	$lxc_setup->pre_start_hook();

	if (@$devices) {
	    my $devlist = '';
	    foreach my $dev (@$devices) {
		my ($mode, $rdev) = (stat($dev))[2,6];
		next if !$mode || !S_ISBLK($mode) || !$rdev;
		my $major = int($rdev / 0x100);
		my $minor = $rdev % 0x100;
		$devlist .= "b:$major:$minor:$dev\n";
	    }
	    PVE::Tools::file_set_contents($devlist_file, $devlist);
	}
	return undef;
    }});

> > subvols are not only for size == 0 , e.g. ZFS supports subvols with and
> > without size (limit). ZFS is a bit tricky unfortunately, as for
> > containers we always want subvols (regular ZFS filesystems), and for
> > VMs we always want raw volumes ("zvols" in ZFS speak). so changing the
> > default format does not really work, and we still need a special case
> > for ZFS (and future storages with the same problem) here?
> But why we are limiting zfspool to subvols only? Following this logic, we
> should forbid raw images for LXC on any other filesystem-based storage and
> use only subvols there, like it was with OpenVZ. Or we can spread generic
> behaviour to zfspool, like I did.
> Maybe as compromise we could add configurable option to plugins to let user
> decide?

It's actually the opposite, we are limited for KVM/Qemu, where we can
only use zvols. ZFS datasets are vastly superior to zvols for container
usage, so that is the default (and only option) for them. In theory we
could allow zvols as well, but there is no reason to do so (they only
have disadvantages, and checking all the places were subvols are
implicitly assumed right now is tedious work for basically no gain).

All of this is orthogonal to the issue at hand though, which is that
your patch would break a currently existing feature: support for ZFS
subvols for containers WITH and without size (limit). This is a feature
which we definitely don't want to drop ;)

> Also, why I can't set volume size to zero on volume creation via WEB UI and
> so use subvols? Is it bug?

It's intentional. Some advanced features are not available in the GUI,
either to keep the GUI simple, or because they are only available to the
root user for security reasons. This one belongs in the first category
(IIRC, it's only available to allow setups similar to legacy OpenVZ
ones, but I may be wrong about that).