The subject should be rephrased to e.g. "partially fix #2164", as the Bugzilla entry also mentions setting up bonds.
Didn't do any testing yet, but some comments inline: On Mon Sep 22, 2025 at 2:30 PM CEST, Florian Paul Azim Hoberg wrote: > To ensure a Promox node can access a desired network > resource which requires a given vlan tag, this feature > adds the possibility to optionally define a vlan tag > within the auto installer, tui and gui. s/this feature adds/add/g Also could mention that it only applies to the selected management interface/bridge. > > Signed-off-by: Florian Paul Azim Hoberg @gyptazy <[email protected]> > --- > > initial commit: > * Add possibility to optionally add vlan tag in: > - auto installer > - tui > - gui > * Tagged interface will be generated as: > $interface.$tag in /etc/network/interfaces > * Simple validation of given vlan tag: > - Must be digit > - Must be smaller than int(4094) That can all be part of the commit message, as that is useful information. [..] > diff --git a/html/ack_template.htm b/html/ack_template.htm > index 48d7213..4404321 100644 > --- a/html/ack_template.htm > +++ b/html/ack_template.htm > @@ -54,6 +54,8 @@ > > <tr><td>Management Interface:</td> <td>__interface__</td></tr> > > + <tr><td>Management Interface vlan tag:</td> <td>__vlan__</td></tr> VLAN should be consistently spelled uppercase everywhere user-visible. Also; wondering if we maybe should just hide this if no VLAN tag is set, no not clutter the summary page unnecessarily? > + > <tr><td>Hostname:</td> <td>__hostname__</td></tr> > > <tr><td>IP CIDR:</td> <td>__cidr__</td></tr> > diff --git a/proxinstall b/proxinstall > index 5ba65fa..13ece1d 100755 > --- a/proxinstall > +++ b/proxinstall > @@ -472,6 +472,14 @@ sub create_ipconf_view { > $grid->attach($dns_label, 0, 4, 1, 1); > $grid->attach($ipconf_entry_dns, 1, 4, 1, 1); > > + my $cfg_vlan = Proxmox::Install::Config::get_vlan(); > + my $vlan = defined($cfg_vlan) ? $cfg_vlan : ''; Can be simplified to my $cfg_vlan = Proxmox::Install::Config::get_vlan() // ''; > + > + my ($vlan_label, $ipconf_entry_vlan) = create_text_input($vlan, 'VLAN > Tag (Optional)'); > + > + $grid->attach($vlan_label, 0, 5, 1, 1); > + $grid->attach($ipconf_entry_vlan, 1, 5, 1, 1); > + > $gtk_state->{inbox}->show_all; > set_next( > undef, > @@ -538,7 +546,19 @@ sub create_ipconf_view { > } > Proxmox::Install::Config::set_dns($dns_ip); > > - #print STDERR "TEST $ipaddress/$netmask $gateway_ip $dns_ip\n"; > + my $vlan = $ipconf_entry_vlan->get_text(); > + if ($vlan !~ /^\d*$/) { > + Proxmox::UI::message("VLAN must contain only digits."); > + $ipconf_entry_vlan->grab_focus(); > + return; > + } > + if ($vlan ne '' && $vlan >= 4094) { > + Proxmox::UI::message("VLAN must be smaller than 4094."); > + $ipconf_entry_vlan->grab_focus(); > + return; > + } Should also check for 0, as that is also a reserved value. > + $vlan = undef if $vlan eq ''; > + Proxmox::Install::Config::set_vlan($vlan); > > $step_number++; > create_ack_view(); > @@ -585,6 +605,7 @@ sub create_ack_view { > __cidr__ => Proxmox::Install::Config::get_cidr(), > __gateway__ => Proxmox::Install::Config::get_gateway(), > __dnsserver__ => Proxmox::Install::Config::get_dns(), > + __vlan__ => Proxmox::Install::Config::get_vlan(), Depending on above, whether to hide it if unset or not, in the latter case this should at least default to 'None' as in the TUI. > ); > > while (my ($k, $v) = each %config_values) { > diff --git a/proxmox-auto-installer/src/answer.rs > b/proxmox-auto-installer/src/answer.rs > index 88f4c87..cea5cb1 100644 > --- a/proxmox-auto-installer/src/answer.rs > +++ b/proxmox-auto-installer/src/answer.rs > @@ -186,6 +186,7 @@ struct NetworkInAnswer { > pub cidr: Option<CidrAddress>, > pub dns: Option<IpAddr>, > pub gateway: Option<IpAddr>, > + pub vlan: Option<u16>, > #[serde(default)] > pub filter: BTreeMap<String, String>, > } > @@ -219,6 +220,7 @@ impl TryFrom<NetworkInAnswer> for Network { > cidr: network.cidr.unwrap(), > dns: network.dns.unwrap(), > gateway: network.gateway.unwrap(), > + vlan: network.vlan, Before setting this, the same check as for the GUI/TUI can be implemented above. And it's missing the check that `vlan` is unset as with the other properties below in the DHCP case. > filter: network.filter, > }), > }) > @@ -254,6 +256,7 @@ pub struct NetworkManual { > pub cidr: CidrAddress, > pub dns: IpAddr, > pub gateway: IpAddr, > + pub vlan: Option<u16>, > pub filter: BTreeMap<String, String>, > } > > diff --git a/proxmox-auto-installer/src/utils.rs > b/proxmox-auto-installer/src/utils.rs > index 7d42f2c..2fdb6df 100644 > --- a/proxmox-auto-installer/src/utils.rs > +++ b/proxmox-auto-installer/src/utils.rs > @@ -66,6 +66,7 @@ fn get_network_settings( > network_options.address = settings.cidr.clone(); > network_options.dns_server = settings.dns; > network_options.gateway = settings.gateway; > + network_options.vlan = settings.vlan; > network_options.ifname = get_single_udev_index(&settings.filter, > &udev_info.nics)?; > } > info!("Network interface used is '{}'", &network_options.ifname); > @@ -494,6 +495,7 @@ pub fn parse_answer( > domain: network_settings.fqdn.domain(), > cidr: network_settings.address, > gateway: network_settings.gateway, > + vlan: network_settings.vlan, > dns: network_settings.dns_server, > > first_boot: InstallFirstBootSetup::default(), [..] > diff --git > a/proxmox-auto-installer/tests/resources/parse_answer/network_vlan.toml > b/proxmox-auto-installer/tests/resources/parse_answer/network_vlan.toml > new file mode 100644 > index 0000000..fad56c6 > --- /dev/null > +++ b/proxmox-auto-installer/tests/resources/parse_answer/network_vlan.toml > @@ -0,0 +1,19 @@ > +[global] > +keyboard = "de" > +country = "at" > +fqdn = "pveauto.testinstall" > +mailto = "[email protected]" > +timezone = "Europe/Vienna" > +root-password = "12345678" > + > +[network] > +source = "from-answer" > +cidr = "10.10.10.10/24" > +dns = "10.10.10.1" > +vlan = 123 > +gateway = "10.10.10.1" > +filter.ID_NET_NAME = "enp129s0f1np1" > + > +[disk-setup] > +filesystem = "ext4" > +disk-list = ["sda"] [..] > diff --git > a/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.toml > b/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.toml > index 901f894..723dfc8 100644 > --- a/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.toml > +++ b/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.toml > @@ -10,6 +10,7 @@ root-password = "12345678" > source = "from-answer" > cidr = "10.10.10.10/24" > dns = "10.10.10.1" > +vlan = 123 Any reason for adding it to this test too, as the `network_vlan` test above seems to be the exact same? > gateway = "10.10.10.1" > filter.ID_NET_NAME_MAC = "*a0369f0ab382" > [..] > diff --git a/proxmox-installer-common/src/setup.rs > b/proxmox-installer-common/src/setup.rs > index 66cea72..23e43d2 100644 > --- a/proxmox-installer-common/src/setup.rs > +++ b/proxmox-installer-common/src/setup.rs > @@ -580,6 +580,7 @@ pub struct InstallConfig { > #[serde(serialize_with = "serialize_as_display")] > pub cidr: CidrAddress, > pub gateway: IpAddr, > + pub vlan: Option<u16>, Should be annotated with #[serde(skip_serializing_if = "Option::is_none")] to avoid serializing it as `null` if unset. > pub dns: IpAddr, > > pub first_boot: InstallFirstBootSetup, > diff --git a/proxmox-tui-installer/src/main.rs > b/proxmox-tui-installer/src/main.rs > index 15ee5d3..21ef16f 100644 > --- a/proxmox-tui-installer/src/main.rs > +++ b/proxmox-tui-installer/src/main.rs [..] > @@ -552,6 +560,23 @@ fn network_dialog(siv: &mut Cursive) -> InstallerView { > .parse::<IpAddr>() > .map_err(|err| err.to_string())?; > > + let vlan_str = view > + .get_value::<EditView, _>(5) > + .ok_or("failed to retrieve VLAN")?; > + > + let vlan = if vlan_str.trim().is_empty() { > + None > + } else { > + let parsed_vlan = vlan_str.trim().parse::<u16>() > + .map_err(|e| format!("Invalid VLAN: {e}"))?; > + > + if parsed_vlan >= 4094 { > + return Err(format!("VLAN must be smaller than > 4094.")); > + } Same tag 0 check as for the GUI. [..] > diff --git a/proxmox-tui-installer/src/options.rs > b/proxmox-tui-installer/src/options.rs > index c80877f..c59712e 100644 > --- a/proxmox-tui-installer/src/options.rs > +++ b/proxmox-tui-installer/src/options.rs > @@ -72,6 +72,7 @@ impl InstallerOptions { > SummaryOption::new("Keyboard layout", kb_layout), > SummaryOption::new("Administrator email", &self.password.email), > SummaryOption::new("Management interface", &self.network.ifname), > + SummaryOption::new("Management interface vlan tag", > &self.network.vlan.map(|v| v.to_string()).unwrap_or("None".to_string())), Uppercase spelling as in the GUI. _______________________________________________ pve-devel mailing list [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
