* [pve-devel] [PATCH container/manager/network] RFC: lxc: add ipam support
@ 2021-05-10 15:37 Alexandre Derumier
2021-05-10 15:37 ` [pve-devel] [PATCH pve-container 1/1] " Alexandre Derumier
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Alexandre Derumier @ 2021-05-10 15:37 UTC (permalink / raw)
To: pve-devel
This patch serie add ipam support to lxc for ipam enabled zone.
The current implementation:
- vnet with ipam enabled:
- ip=static && value=empty: ip is auto assign from ipam
- ip=static when defined ip manually: - ip will registered to ipam
- verify than ip match an associated subnet
- vnet without ipam enable:
- ip=static when defined ip manually: verify than ip match an associated subnet
Alexandre Derumier (1):
add ipam support
src/PVE/LXC.pm | 3 +
src/PVE/LXC/Config.pm | 168 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 171 insertions(+)
Alexandre Derumier (1):
sdn: get_local_vnets : add ipam && vlanaware values
PVE/Network/SDN.pm | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Alexandre Derumier (1):
ui: lxc: network: sdn display improvements
www/manager6/form/BridgeSelector.js | 2 +-
www/manager6/lxc/Network.js | 34 +++++++++++++++++++++++++++++
2 files changed, 35 insertions(+), 1 deletion(-)
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH pve-container 1/1] add ipam support 2021-05-10 15:37 [pve-devel] [PATCH container/manager/network] RFC: lxc: add ipam support Alexandre Derumier @ 2021-05-10 15:37 ` Alexandre Derumier 2021-05-12 12:27 ` Wolfgang Bumiller 2021-05-10 15:37 ` [pve-devel] [PATCH pve-network 1/1] sdn: get_local_vnets : add ipam && vlanaware values Alexandre Derumier 2021-05-10 15:37 ` [pve-devel] [PATCH pve-manager 1/1] ui: lxc: network: sdn display improvements Alexandre Derumier 2 siblings, 1 reply; 6+ messages in thread From: Alexandre Derumier @ 2021-05-10 15:37 UTC (permalink / raw) To: pve-devel 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}); + 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) { + 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'; + }; + 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}; + + #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}) { + #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}; + $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}; + } + } + }; + 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')) { + 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') { + eval { + PVE::Network::SDN::Vnets::del_cidr($bridge, $net->{ip}, $hostname, $description); + }; + warn $@ if $@; + } +} + 1; -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH pve-container 1/1] add ipam support 2021-05-10 15:37 ` [pve-devel] [PATCH pve-container 1/1] " Alexandre Derumier @ 2021-05-12 12:27 ` Wolfgang Bumiller 2021-05-18 11:43 ` aderumier 0 siblings, 1 reply; 6+ messages in thread From: Wolfgang Bumiller @ 2021-05-12 12:27 UTC (permalink / raw) To: Alexandre Derumier; +Cc: pve-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH pve-container 1/1] add ipam support 2021-05-12 12:27 ` Wolfgang Bumiller @ 2021-05-18 11:43 ` aderumier 0 siblings, 0 replies; 6+ messages in thread From: aderumier @ 2021-05-18 11:43 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pve-devel 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] [PATCH pve-network 1/1] sdn: get_local_vnets : add ipam && vlanaware values 2021-05-10 15:37 [pve-devel] [PATCH container/manager/network] RFC: lxc: add ipam support Alexandre Derumier 2021-05-10 15:37 ` [pve-devel] [PATCH pve-container 1/1] " Alexandre Derumier @ 2021-05-10 15:37 ` Alexandre Derumier 2021-05-10 15:37 ` [pve-devel] [PATCH pve-manager 1/1] ui: lxc: network: sdn display improvements Alexandre Derumier 2 siblings, 0 replies; 6+ messages in thread From: Alexandre Derumier @ 2021-05-10 15:37 UTC (permalink / raw) To: pve-devel to be able to use them in ui bridgeselector Signed-off-by: Alexandre Derumier <aderumier@odiso.com> --- PVE/Network/SDN.pm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/PVE/Network/SDN.pm b/PVE/Network/SDN.pm index 314b515..d3399ce 100644 --- a/PVE/Network/SDN.pm +++ b/PVE/Network/SDN.pm @@ -198,7 +198,9 @@ sub get_local_vnets { my $zone_config = PVE::Network::SDN::Zones::sdn_zones_config($zones_cfg, $zoneid); next if defined($zone_config->{nodes}) && !$zone_config->{nodes}->{$nodename}; - $vnets->{$vnetid} = { type => 'vnet', active => '1', comments => $comments }; + my $ipam = $zone_config->{ipam} ? 1 : 0; + my $vlanaware = $vnet->{vlanaware} ? 1 : 0; + $vnets->{$vnetid} = { type => 'vnet', active => '1', ipam => $ipam, vlanaware => $vlanaware, comments => $comments }; } return $vnets; -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] [PATCH pve-manager 1/1] ui: lxc: network: sdn display improvements 2021-05-10 15:37 [pve-devel] [PATCH container/manager/network] RFC: lxc: add ipam support Alexandre Derumier 2021-05-10 15:37 ` [pve-devel] [PATCH pve-container 1/1] " Alexandre Derumier 2021-05-10 15:37 ` [pve-devel] [PATCH pve-network 1/1] sdn: get_local_vnets : add ipam && vlanaware values Alexandre Derumier @ 2021-05-10 15:37 ` Alexandre Derumier 2 siblings, 0 replies; 6+ messages in thread From: Alexandre Derumier @ 2021-05-10 15:37 UTC (permalink / raw) To: pve-devel - Display "auto ipam" for empty text when ipam is enabled on vnet - clear ip/gw value on vnet change - disable/enable vlan tag for non/vlanaware vnets Signed-off-by: Alexandre Derumier <aderumier@odiso.com> --- www/manager6/form/BridgeSelector.js | 2 +- www/manager6/lxc/Network.js | 34 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/www/manager6/form/BridgeSelector.js b/www/manager6/form/BridgeSelector.js index 350588cd..227976af 100644 --- a/www/manager6/form/BridgeSelector.js +++ b/www/manager6/form/BridgeSelector.js @@ -5,7 +5,7 @@ Ext.define('PVE.form.BridgeSelector', { bridgeType: 'any_bridge', // bridge, OVSBridge or any_bridge store: { - fields: ['iface', 'active', 'type'], + fields: ['iface', 'active', 'ipam', 'type', 'vlanaware'], filterOnLoad: true, sorters: [ { diff --git a/www/manager6/lxc/Network.js b/www/manager6/lxc/Network.js index 9fe479f8..a0274201 100644 --- a/www/manager6/lxc/Network.js +++ b/www/manager6/lxc/Network.js @@ -131,6 +131,40 @@ Ext.define('PVE.lxc.NetworkInputPanel', { fieldLabel: gettext('Bridge'), value: cdata.bridge, allowBlank: false, + listeners: { + change: function(combo, newValue) { + var store = combo.getStore(); + var rec = store.findRecord('iface', newValue, 0, false, true, true); + if (!rec) { + return; + } + + if (rec.data.type === 'vnet' && !rec.data.vlanaware) { + me.down('field[name=tag]').setDisabled(true); + } else { + me.down('field[name=tag]').setDisabled(false); + } + + if (rec.data.type === 'vnet') { + me.down('field[name=ip]').setValue(''); + me.down('field[name=gw]').setValue(''); + me.down('field[name=ip6]').setValue(''); + me.down('field[name=gw6]').setValue(''); + } + + if (!rec.data.ipam) { + me.down('field[name=ip]').setEmptyText(Proxmox.Utils.NoneText); + me.down('field[name=gw]').setEmptyText(Proxmox.Utils.NoneText); + me.down('field[name=ip6]').setEmptyText(Proxmox.Utils.NoneText); + me.down('field[name=gw6]').setEmptyText(Proxmox.Utils.NoneText); + } else { + me.down('field[name=ip]').setEmptyText('auto (ipam)'); + me.down('field[name=gw]').setEmptyText('auto (ipam)'); + me.down('field[name=ip6]').setEmptyText('auto (ipam)'); + me.down('field[name=gw6]').setEmptyText('auto (ipam)'); + } + }, + }, }, { xtype: 'pveVlanField', -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-18 11:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-10 15:37 [pve-devel] [PATCH container/manager/network] RFC: lxc: add ipam support Alexandre Derumier 2021-05-10 15:37 ` [pve-devel] [PATCH pve-container 1/1] " Alexandre Derumier 2021-05-12 12:27 ` Wolfgang Bumiller 2021-05-18 11:43 ` aderumier 2021-05-10 15:37 ` [pve-devel] [PATCH pve-network 1/1] sdn: get_local_vnets : add ipam && vlanaware values Alexandre Derumier 2021-05-10 15:37 ` [pve-devel] [PATCH pve-manager 1/1] ui: lxc: network: sdn display improvements Alexandre Derumier
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.