Re: [pve-devel] [PATCH pve-container 1/1] add ipam support

[aderumier@odiso.com]

thanks for the review wolfgang,

I send a v2 tomorrow, with cleanup, refactoring, tests  and support for
snapshot rollback (need to check ipam too for ip conflict).(and maybe
backup/restore if I have time)


Le mercredi 12 mai 2021 à 14:27 +0200, Wolfgang Bumiller a écrit :
> On Mon, May 10, 2021 at 05:37:07PM +0200, Alexandre Derumier wrote:
> > This add ipam support for nic using sdn vnets.
> > 
> > - if ips are specified manally, we verify that subnet exist on
> > vnet, and we register ip in ipam
> > - if nic is on a vnet, but no ip is specified, we auto find the
> > next available ips in this vnet subnet
> > - if a gateway is defined on the subnet, we override current vm nc
> > gateway
> > - extra informations like mac address,hostname are registered in
> > external ipam.
> > - if dns server exist in the zone, we register vm hostname in ipam
> > for each ip address
> > 
> > - ips addresses are removed on ct destroy, nic destroy, ip change
> > or vnet change
> > 
> > Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> > ---
> >  src/PVE/LXC.pm        |   3 +
> >  src/PVE/LXC/Config.pm | 168
> > ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 171 insertions(+)
> > 
> > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> > index 7e6f378..f6bd806 100644
> > --- a/src/PVE/LXC.pm
> > +++ b/src/PVE/LXC.pm
> > @@ -847,6 +847,9 @@ sub destroy_lxc_container {
> >      rmdir "/var/lib/lxc/$vmid/rootfs";
> >      unlink "/var/lib/lxc/$vmid/config";
> >      rmdir "/var/lib/lxc/$vmid";
> > +
> > +    PVE::LXC::Config->delete_ipam_ifaces($conf);
> > +
> >      if (defined $replacement_conf) {
> >         PVE::LXC::Config->write_config($vmid, $replacement_conf);
> >      } else {
> > diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> > index 7b82f65..c0be257 100644
> > --- a/src/PVE/LXC/Config.pm
> > +++ b/src/PVE/LXC/Config.pm
> > @@ -12,6 +12,14 @@ use PVE::INotify;
> >  use PVE::JSONSchema qw(get_standard_option);
> >  use PVE::Tools;
> >  
> > +my $have_sdn;
> > +eval {
> > +    require PVE::Network::SDN::Vnets;
> > +    require PVE::Network::SDN::Subnets;
> > +    $have_sdn = 1;
> > +};
> > +
> > +
> >  use base qw(PVE::AbstractConfig);
> >  
> >  use constant {FIFREEZE => 0xc0045877,
> > @@ -1044,6 +1052,10 @@ sub update_pct_config {
> >             $class->check_protection($conf, "can't remove CT $vmid
> > drive '$opt'");
> >         } elsif ($opt eq 'unprivileged') {
> >             die "unable to delete read-only option: '$opt'\n";
> > +       } elsif ($opt =~ m/^net(\d+)$/) {
> > +           my $netid = $1;
> > +           my $oldnet = $class->parse_lxc_network($conf->{$opt});
> > +           delete_net_ip($conf->{hostname}, $oldnet, "vm:$vmid
> > net:$netid");
> >         }
> >         $class->add_to_pending_delete($conf, $opt);
> >      }
> > @@ -1071,7 +1083,24 @@ sub update_pct_config {
> >             $value = PVE::LXC::verify_searchdomain_list($value);
> >         } elsif ($opt eq 'unprivileged') {
> >             die "unable to modify read-only option: '$opt'\n";
> > +       } elsif ($opt =~ m/^net(\d+)$/) {
> > +               my $netid = $1;
> > +               my $net = $class->parse_lxc_network($value);
> > +               my $oldnet = $class->parse_lxc_network($conf-
> > >{$opt});
> > +               my $hostname = $param->{hostname} ? $param-
> > >{hostname} : $conf->{hostname};
> > +               update_net_ip($net, $oldnet, $hostname, $hostname,
> > "vm:$vmid net:$netid");
> > +               $value = $class->print_lxc_network($net);
> > +       } elsif ($opt eq 'hostname') {
> > +           #if hostname change, update ipam + dns for each ip
> > +           foreach my $netopt (sort keys %$conf) {
> > +               next if $netopt !~ m/^net(\d+)$/;
> > +               my $netid = $1;
> > +               my $net = $class->parse_lxc_network($conf-
> > >{$netopt});
> > +               my $oldnet = $class->parse_lxc_network($conf-
> > >{$netopt});
> 
> ^ you can juse use $net twice, no need to parse the same value twice
> 
> > +               update_net_ip($net, $oldnet, $value, $conf-
> > >{hostname}, "vm:$vmid net:$netid");
> > +           }
> >         }
> > +
> >         $conf->{pending}->{$opt} = $value;
> >         $class->remove_from_pending_delete($conf, $opt);
> >      }
> > @@ -1663,4 +1692,143 @@ sub get_backup_volumes {
> >      return $return_volumes;
> >  }
> >  
> > +sub delete_ipam_ifaces {
> > +    my ($class, $conf) = @_;
> > +
> > +    return if !$have_sdn;
> > +
> > +    foreach my $opt (sort keys %$conf) {
> 
> ^ do we need this to be sorted?
> 
> > +       next if $opt !~ m/^net(\d+)$/;
> > +       my $netid = $1;
> > +       my $net = $class->parse_lxc_network($conf->{$opt});
> > +       my $bridge = $net->{bridge};
> > +       my $hostname = $conf->{hostname};
> > +       my $description = '';
> > +
> > +       my $subnets =
> > PVE::Network::SDN::Vnets::get_subnets($bridge);
> > +       next if !keys %{$subnets};
> > +
> > +       eval {
> > +           PVE::Network::SDN::Vnets::del_cidr($bridge, $net->{ip},
> > $hostname, $description) if $net->{ip} && $net->{ip} ne 'dhcp' &&
> > $net->{ip} ne 'manual';
> > +           PVE::Network::SDN::Vnets::del_cidr($bridge, $net-
> > >{ip6}, $hostname, $description) if $net->{ip6} && $net->{ip6} ne
> > 'dhcp' && $net->{ip6} ne 'manual' && $net->{ip6} ne 'auto';
> 
> ^ Can we please factor out the above conditions in the post-fix if
> statements into a separate helper (ie. 'is_static_ip' or something?),
> because you use the same condition multiple times, but there's a case
> further down below where it's inconsistent and probably wrong...
> (btw. a helper wouldn't necessarily need to bother distinguishing
> between ipv4 and ipv6 since in ipv4 'auto' simply never happens)
> 
> > +       };
> > +       if ($@) {
> > +           warn $@;
> > +       }
> > +    }
> > +}
> > +
> > +sub update_net_ip {
> > +    my ($net, $oldnet, $hostname, $oldhostname, $description) =
> > @_;
> > +
> > +    return if !$have_sdn;
> > +
> > +    my $oldbridge = $oldnet->{bridge};
> > +    my $bridge = $net->{bridge};
> > +    my $mac = $net->{hwaddr};
> > +
> > +    my $subnets = PVE::Network::SDN::Vnets::get_subnets($bridge);
> > +
> > +    return if !keys %{$subnets};
> 
> I have a feeling the code below might get a little more readable if
> we
> pull out some conditions into variables here, not sure, something
> like
> $ip_changed, $new_ip_not_static, $old_ip_not_static for both ipv4 and
> ipv6
> Though I'm not sure...
> 
> > +
> > +    #add new ip first
> > +    my $new_ipv4_allocated = undef;
> > +    eval {
> > +       if (!$net->{ip}) {
> > +           $net->{ip} =
> > PVE::Network::SDN::Vnets::get_next_free_cidr($bridge, $hostname,
> > $mac, $description);
> > +           $new_ipv4_allocated = 1;
> > +       } elsif ($net->{ip} ne 'dhcp' && $net->{ip} ne 'manual') {
> > +
> > +           if ($oldnet->{ip} && $net->{ip} eq $oldnet->{ip}) {
> 
> would be !$ip_changed
> 
> > +               #update ip attributes if no ip address change
> > +               PVE::Network::SDN::Vnets::update_cidr($bridge,
> > $net->{ip}, $hostname, $oldhostname, $mac, $description);
> > +           } else {
> > +               PVE::Network::SDN::Vnets::add_cidr($bridge, $net-
> > >{ip}, $hostname, $mac, $description) if !$oldnet->{ip} || $net-
> > >{ip} ne $oldnet->{ip};
> 
> I think it's better to move the post-fix condition up to replace the
> `else` with an `elsif`, since IMO it should also cover the line
> below,
> right?
> Note that the 2nd half of the condition is already implied at this
> point.
> 
> > +               $new_ipv4_allocated = 1;
> > +           }
> > +       }
> > +    };
> > +    if ($@) {
> > +       die $@;
> > +    }
> > +
> > +    #then add ip6
> > +    eval {
> > +       if (!$net->{ip6}) {
> > +           $net->{ip6} =
> > PVE::Network::SDN::Vnets::get_next_free_cidr($bridge, $hostname,
> > $mac, $description, 6);
> > +       } elsif ($net->{ip6} ne 'dhcp' && $net->{ip6} ne 'manual'
> > && $net->{ip6} ne 'auto') {
> > +           if ($oldnet->{ip6} && $net->{ip6} eq $oldnet->{ip6}) {
> > +               #update ipv6 attributes if no ip address change
> > +               PVE::Network::SDN::Vnets::update_cidr($bridge,
> > $net->{ip6}, $hostname, $oldhostname, $mac, $description);
> > +           } else {
> > +               PVE::Network::SDN::Vnets::add_cidr($bridge, $net-
> > >{ip6}, $hostname, $mac, $description) if !$oldnet->{ip6} || $net-
> > >{ip6} ne $oldnet->{ip6};
> 
> ^ same here
> 
> > +           }
> > +       }
> > +    };
> > +    if ($@) {
> > +       my $err = $@;
> > +       #if error, delete previously added ipv4
> > +       if ($new_ipv4_allocated) {
> > +           eval {
> > +               PVE::Network::SDN::Vnets::del_cidr($bridge, $net-
> > >{ip}, $hostname, $description);
> > +           };
> > +       }
> > +       die $err;
> > +    }
> > +
> > +    #delete old ip
> > +    if ($oldnet->{ip} && $oldnet->{ip} ne 'dhcp' && $oldnet->{ip}
> > ne 'manual') {
> > +       my $deletebridge = $oldbridge ne $bridge ? $oldbridge :
> > $bridge;
> > +       eval {
> > +           PVE::Network::SDN::Vnets::del_cidr($deletebridge,
> > $oldnet->{ip}, $hostname, $description) if !$net->{ip} || $net-
> > >{ip} ne $oldnet->{ip};
> > +       };
> > +       warn $@ if $@;
> > +    }
> > +
> > +    #delete old ip6
> > +    if ($oldnet->{ip6} && $oldnet->{ip6} ne 'dhcp' && $oldnet-
> > >{ip6} ne 'manual' && $net->{ip6} ne 'auto') {
> > +       my $deletebridge = $oldbridge ne $bridge ? $oldbridge :
> > $bridge;
> > +       eval {
> > +           PVE::Network::SDN::Vnets::del_cidr($deletebridge,
> > $oldnet->{ip6}, $hostname) if !$net->{ip6} || $net->{ip6} ne
> > $oldnet->{ip6};
> > +       };
> > +       warn $@ if $@;
> > +    }
> > +
> > +    #update gateway
> > +    if ($net->{ip} && $net->{ip} ne 'dhcp' && $net->{ip} ne
> > 'manual') {
> > +       my ($ip, $mask) = split(/\//, $net->{ip});
> > +       my ($subnetidv4, $subnetv4) =
> > PVE::Network::SDN::Subnets::find_ip_subnet($ip, $mask, $subnets);
> > +       $net->{gw} = $subnetv4->{gateway} if $subnetv4->{gateway};
> > +    }
> > +
> > +    if ($net->{ip6} && $net->{ip6} ne 'dhcp' && $net->{ip6} ne
> > 'manual' && $net->{ip6} ne 'auto') {
> > +       my ($ip6, $mask6) = split(/\//, $net->{ip6});
> > +       my ($subnetidv6, $subnetv6) =
> > PVE::Network::SDN::Subnets::find_ip_subnet($ip6, $mask6, $subnets);
> > +       $net->{gw6} = $subnetv6->{gateway} if $subnetv6->{gateway};
> > +    }
> > +
> > +}
> > +
> > +sub delete_net_ip {
> > +    my ($hostname, $net, $description) = @_;
> > +
> > +    return if !$have_sdn;
> > +
> > +    my $bridge = $net->{bridge};
> > +    my $subnets = PVE::Network::SDN::Vnets::get_subnets($bridge);
> > +    return if !keys %{$subnets};
> > +
> > +    if ($net->{ip6} && ($net->{ip6} eq 'auto' || $net->{ip6} eq
> > 'manual' || $net->{ip6} eq 'dhcp')) {
> 
> So here this reads as: "$net->{ip6} is one of 'auto', 'manual' or
> 'dhcp'"
> with the helper mentioned above:
>   if ($net->{ip6} && !ip_is_static($net->{ip6}))
> 
> > +       eval {
> > +           PVE::Network::SDN::Vnets::del_cidr($bridge, $net-
> > >{ip6}, $hostname, $description);
> > +       };
> > +       warn $@ if $@;
> > +    } elsif ($net->{ip} && $net->{ip} ne 'dhcp' && $net->{ip} ne
> > 'manual') {
> 
> but the IPv4 case here reads as: "$net->{ip} is set but is neither
> 'dhcp' nor 'manual'"
> with the helper mentioned above:
>   if ($net->{ip} && ip_is_static($net->{ip}))
> 
> this seems inconsistent (note the '!' on the `ip_is_static` call
> differ
> between the ipv4 & ipv6 cases)
> 
> I think you meant to use the same condition for both?
> 
> > +       eval {
> > +           PVE::Network::SDN::Vnets::del_cidr($bridge, $net->{ip},
> > $hostname, $description);
> > +       };
> > +       warn $@ if $@;
> > +    }
> > +}
> > +
> >  1;
> > -- 
> > 2.20.1
>