[pve-devel] applied: [PATCH pve-network 2/2] vnetplugin: on_delete_hook : verify if vnet exist in vm && ct

[Dietmar Maurer]

applied, few questions inline -  i am not really happy with this patch.

> On 04 April 2019 at 16:12 Alexandre Derumier <aderumier at odiso.com> wrote:
> 
> 
> ---
>  PVE/API2/Network/Network.pm       |  3 +-
>  PVE/Network/Network/VnetPlugin.pm | 58 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Network/Network.pm b/PVE/API2/Network/Network.pm
> index 7a8b299..6ea8fe2 100644
> --- a/PVE/API2/Network/Network.pm
> +++ b/PVE/API2/Network/Network.pm
> @@ -131,7 +131,8 @@ __PACKAGE__->register_method ({
>  
>  		my $cfg = PVE::Network::Network::config();
>  
> -		if (my $scfg = PVE::Network::Network::network_config($cfg, $networkid, 1)) {
> +		my $scfg = undef;
> +		if ($scfg = PVE::Network::Network::network_config($cfg, $networkid, 1)) {
>  		    die "network object ID '$networkid' already defined\n";
>  		}
>  
> diff --git a/PVE/Network/Network/VnetPlugin.pm b/PVE/Network/Network/VnetPlugin.pm
> index 0b99e7e..f14db35 100644
> --- a/PVE/Network/Network/VnetPlugin.pm
> +++ b/PVE/Network/Network/VnetPlugin.pm
> @@ -6,6 +6,12 @@ use PVE::Network::Network::Plugin;
>  
>  use base('PVE::Network::Network::Plugin');
>  
> +use PVE::Cluster;
> +use PVE::LXC;
> +use PVE::LXC::Config;
> +use PVE::QemuServer;
> +use PVE::QemuConfig;

I guess this introduces a cyclic package dependency?

pve-network => qemu-server/pve-contaiber => pve-network

> +
>  sub type {
>      return 'vnet';
>  }
> @@ -65,6 +71,26 @@ sub on_delete_hook {
>      my ($class, $networkid, $scfg) = @_;

Do we really want to have such checks? AFAIK we don't doö that for normal network?

>      # verify than no vm or ct have interfaces in this bridge
> +    my $vmdata = read_local_vm_config();

Why only _local_ vms?

> +
> +    foreach my $vmid (sort keys %{$vmdata->{qemu}}) {
> +	my $conf = $vmdata->{qemu}->{$vmid};
> +	foreach my $netid (sort keys %$conf) {
> +	    next if $netid !~ m/^net(\d+)$/;
> +	    my $net = PVE::QemuServer::parse_net($conf->{$netid});
> +	    die "vnet $networkid is used by vm $vmid" if $net->{bridge} eq $networkid;
> +	}
> +    }
> +
> +    foreach my $vmid (sort keys %{$vmdata->{lxc}}) {
> +	my $conf = $vmdata->{lxc}->{$vmid};
> +	foreach my $netid (sort keys %$conf) {
> +	    next if $netid !~ m/^net(\d+)$/;
> +	    my $net = PVE::LXC::Config->parse_lxc_network($conf->{$netid});
> +	    die "vnet $networkid is used by ct $vmid" if $net->{bridge} eq $networkid;
> +	}
> +    }
> +
>  }
>  
>  sub on_update_hook {
> @@ -74,4 +100,36 @@ sub on_update_hook {
>  
>  }
>  
> +sub read_local_vm_config {
> +
> +    my $qemu = {};
> +    my $lxc = {};
> +
> +    my $vmdata = { qemu => $qemu, lxc => $lxc };
> +
> +    my $vmlist = PVE::Cluster::get_vmlist();
> +    return $vmdata if !$vmlist || !$vmlist->{ids};
> +    my $ids = $vmlist->{ids};
> +
> +    foreach my $vmid (keys %$ids) {
> +	next if !$vmid;
> +	my $d = $ids->{$vmid};
> +	next if !$d->{node};

why (only local) exactly?

> +	next if !$d->{type};
> +	if ($d->{type} eq 'qemu') {
> +	    my $cfspath = PVE::QemuConfig->cfs_config_path($vmid);
> +	    if (my $conf = PVE::Cluster::cfs_read_file($cfspath)) {
> +		$qemu->{$vmid} = $conf;
> +	    }
> +	} elsif ($d->{type} eq 'lxc') {
> +	    my $cfspath = PVE::LXC::Config->cfs_config_path($vmid);
> +	    if (my $conf = PVE::Cluster::cfs_read_file($cfspath)) {
> +		$lxc->{$vmid} = $conf;
> +	    }
> +	}
> +    }
> +
> +    return $vmdata;
> +};
> +
>  1;
> -- 
> 2.11.0
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel