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

[Alexandre DERUMIER]

>>I guess this introduces a cyclic package dependency?
>>
>>pve-network => qemu-server/pve-contaiber => pve-network

Currently, the package is PVE::Network::Network, so no. 

but maybe could it be better to rename it to something like PVE::Network::SDN? (to avoid confusion)



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

currently, with ifupdown classic, we need to reboot the node. so the vm are stopped or migrated to another host before reboot.
with ifupdown2 reload (already implemented), we are already checking if tap/veth are running on bridge.

Personally, I really would like to have a check to avoid breaking network of all vm on miss removal.
(as It's not possible to reattach them, reverting the configuration, as we don't register tap/veth in /etc/network/interfaces bridges.)



>> + my $vmdata = read_local_vm_config(); 
>>
>>Why only _local_ vms? 

(I copy/paste sub from pve::firewall (with some small modifications to read all nodes), so maybe it could be move to a common class )

> + next if !$d->{node}; 

>>why (only local) exactly? 
oops, forget to remove it.

(I'm thing to add something common like read_vm_config($node), with $node optional)





----- Mail original -----
De: "dietmar" <dietmar at proxmox.com>
À: "pve-devel" <pve-devel at pve.proxmox.com>, "aderumier" <aderumier at odiso.com>
Envoyé: Vendredi 5 Avril 2019 06:21:15
Objet: applied: [pve-devel] [PATCH pve-network 2/2] vnetplugin: on_delete_hook : verify if vnet exist in vm && ct

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