* [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
* [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
* 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
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox