[pve-devel] [PATCH container] LXC needs krbd regardless of storage entry

[Thomas Lamprecht]

Am 07/25/2018 um 06:18 PM schrieb Alwin Antreich:
> This patch adds the lxc_krbd flag to the storage config, so the RBD
> storage plugin is aware of the need for krbd from lxc. Removed the check
> for krbd.
> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>   src/PVE/API2/LXC.pm | 10 +++++-----
>   src/PVE/LXC.pm      | 20 ++++++++++++++------
>   2 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 55e6310..10ebd9d 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -246,7 +246,7 @@ __PACKAGE__->register_method({
>   
>   	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, $pool, $param, []);
>   
> -	my $storage_cfg = cfs_read_file("storage.cfg");
> +	my $storage_cfg = PVE::LXC::storage_cfg_override(cfs_read_file("storage.cfg"));
>   

Move the call reading the storage.cfg read to the override helpers body,
results in nicer nicer caller interfaces.

>   
>   	my $archive;
> @@ -622,7 +622,7 @@ __PACKAGE__->register_method({
>   	# test if container exists
>   	my $conf = PVE::LXC::Config->load_config($vmid);
>   
> -	my $storage_cfg = cfs_read_file("storage.cfg");
> +	my $storage_cfg = PVE::LXC::storage_cfg_override(cfs_read_file("storage.cfg"));
>   
>   	PVE::LXC::Config->check_protection($conf, "can't remove CT $vmid");
>   
> @@ -1122,7 +1122,7 @@ __PACKAGE__->register_method({
>   	    die "snapshot '$snapname' does not exist\n" if !defined($snap);
>   	    $conf = $snap;
>   	}
> -	my $storage_cfg = PVE::Storage::config();
> +	my $storage_cfg = PVE::LXC::storage_cfg_override(PVE::Storage::config());
>   	#Maybe include later
>   	#my $nodelist = PVE::LXC::shared_nodes($conf, $storage_cfg);
>   	my $hasFeature = PVE::LXC::Config->has_feature($feature, $conf, $storage_cfg, $snapname);
> @@ -1569,7 +1569,7 @@ __PACKAGE__->register_method({
>   
>   	PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $param, []);
>   
> -	my $storage_cfg = cfs_read_file("storage.cfg");
> +	my $storage_cfg = PVE::LXC::storage_cfg_override(cfs_read_file("storage.cfg"));
>   
>   	my $code = sub {
>   
> @@ -1751,7 +1751,7 @@ __PACKAGE__->register_method({
>   		PVE::Cluster::log_msg('info', $authuser, "move volume CT $vmid: move --volume $mpkey --storage $storage");
>   
>   		my $conf = PVE::LXC::Config->load_config($vmid);
> -		my $storage_cfg = PVE::Storage::config();
> +		my $storage_cfg = PVE::LXC::storage_cfg_override(PVE::Storage::config());
>   
>   		my $new_volid;
>   
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index bc03792..9ab1df8 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1251,11 +1251,6 @@ sub mountpoint_mount {
>   
>   	my $scfg = PVE::Storage::storage_config($storage_cfg, $storage);
>   
> -	# early sanity checks:
> -	# we otherwise call realpath on the rbd url
> -	die "containers on rbd storage without krbd are not supported\n"
> -	    if $scfg->{type} eq 'rbd' && !$scfg->{krbd};
> -
>   	my $path = PVE::Storage::path($storage_cfg, $volid, $snapname);
>   
>   	my ($vtype, undef, undef, undef, undef, $isBase, $format) =
> @@ -1416,7 +1411,6 @@ sub alloc_disk {
>   
>   	} elsif ($scfg->{type} eq 'rbd') {
>   
> -	    die "krbd option must be enabled on storage type '$scfg->{type}'\n" if !$scfg->{krbd};
>   	    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
>   	    $do_format = 1;
>   	} else {
> @@ -1748,4 +1742,18 @@ sub copy_volume {
>       return $new_volid;
>   }
>   
> +# set lxc_krbd to let container work with RBD storage

I'd move this comment to the if with the rbd check below,
add a more general comment here - if one at all

> +sub storage_cfg_override {
> +    my ($storage_cfg) = shift;
> +
> +    foreach my $k (keys %{$storage_cfg->{ids}}) {
> +
> +	if ($storage_cfg->{ids}->{$k}->{type} eq 'rbd') {
> +	    $storage_cfg->{ids}->{$k}->{lxc_krbd} = 1;

just overwrite the 'krbd' key to always yes, like my initial idea was,
no need to add an extra key if it's used as exact synonym in your
storage patches anyway? No need to even touch storage initially.

If there containers can reside at all on a storage is controlled with
the contents property anyway.

Or did you have specific reasons to do it that way?

And add a references to the real store hash to make the code a bit more
readable:

my $scfg = cfs_read_file('storage.cfg');

foreach my $k (keys %{$scfg->{ids}}) {
     my $store = $scfg->{ids}->{$k};
     if ($store->{type} eq 'rbd') {
	# always map rbd images for CT - if CT are allowed is checked with 
'contents' property
	$store->{krbd} = 1;
     }
}

naturally above snipped is not tested ;) just an example, adjust to your
likings

> +	}
> +    }
> +
> +    return $storage_cfg;
> +}
> +
>   1;
>