>>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" <diet...@proxmox.com>
À: "pve-devel" <pve-devel@pve.proxmox.com>, "aderumier" <aderum...@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 <aderum...@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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
