public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Michael Köppl" <m.koeppl@proxmox.com>
To: "Proxmox VE development discussion" <pve-devel@lists.proxmox.com>
Cc: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH installer 14/14] gui: add support for pinning network interface names
Date: Tue, 21 Oct 2025 16:05:31 +0200	[thread overview]
Message-ID: <DDO232Q5XNFX.2VGDBBZ3VC83P@proxmox.com> (raw)
In-Reply-To: <20251014132207.1171073-15-c.heiss@proxmox.com>

3 comments inline

On Tue Oct 14, 2025 at 3:21 PM CEST, Christoph Heiss wrote:
> Adds an additional checkbox and option button in the network panel, the
> latter triggering a dialog for setting custom names per network
> interface present on the system.
>
> Pinning is enabled by default.
>
> Each pinned network interface name defaults to `nicN`, where N is the
> pinned ID from the low-level installer.
>
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  Proxmox/Sys/Net.pm |   9 +-
>  proxinstall        | 209 ++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 197 insertions(+), 21 deletions(-)
>
> diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
> index 2183d27..7fe800c 100644
> --- a/Proxmox/Sys/Net.pm
> +++ b/Proxmox/Sys/Net.pm
> @@ -8,7 +8,14 @@ use Proxmox::Sys::Udev;
>  use JSON qw();
>  
>  use base qw(Exporter);
> -our @EXPORT_OK = qw(parse_ip_address parse_ip_mask parse_fqdn);
> +our @EXPORT_OK = qw(parse_ip_address parse_ip_mask parse_fqdn MAX_IFNAME_LEN DEFAULT_PIN_PREFIX);
> +
> +# Maximum length of the (primary) name of a network interface
> +# IFNAMSIZ - 1 to account for NUL byte
> +use constant {
> +    MAX_IFNAME_LEN => 15,
> +    DEFAULT_PIN_PREFIX => 'nic',
> +};
>  
>  our $HOSTNAME_RE = "(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]{,61}?[a-zA-Z0-9])?)";
>  our $FQDN_RE = "(?:${HOSTNAME_RE}\\.)*${HOSTNAME_RE}";
> diff --git a/proxinstall b/proxinstall
> index 5ba65fa..35e948a 100755
> --- a/proxinstall
> +++ b/proxinstall
> @@ -37,7 +37,7 @@ use Proxmox::Sys;
>  use Proxmox::Sys::Block qw(get_cached_disks);
>  use Proxmox::Sys::Command qw(syscmd);
>  use Proxmox::Sys::File qw(file_read_all file_write_all);
> -use Proxmox::Sys::Net qw(parse_ip_address parse_ip_mask);
> +use Proxmox::Sys::Net qw(parse_ip_address parse_ip_mask MAX_IFNAME_LEN DEFAULT_PIN_PREFIX);
>  use Proxmox::UI;
>  
>  my $step_number = 0; # Init number for global function list
> @@ -340,11 +340,117 @@ my $create_basic_grid = sub {
>      return $grid;
>  };
>  
> +my sub create_network_interface_pin_view {
> +    my ($done_cb) = @_;
> +
> +    my $dialog = Gtk3::Dialog->new();
> +    $dialog->set_title('Interface Name Pinning Options');
> +    $dialog->add_button('_OK', 1);
> +
> +    my $content = $dialog->get_content_area();
> +
> +    my $hbox = Gtk3::Box->new('horizontal', 0);
> +    $content->pack_start($hbox, 1, 1, 5);
> +
> +    my $grid = Gtk3::Grid->new();
> +    $grid->set_column_spacing(10);
> +    $grid->set_row_spacing(10);
> +
> +    # make the list scrollable, in case there are lots of interfaces
> +    my $scrolled_window = Gtk3::ScrolledWindow->new();
> +    $scrolled_window->set_hexpand(1);
> +    $scrolled_window->set_propagate_natural_height(1);
> +
> +    $scrolled_window->add($grid);
> +    $scrolled_window->set_policy('never', 'automatic');
> +    $scrolled_window->set_visible(1);
> +    $scrolled_window->set_min_content_height(200);
> +    $scrolled_window->set_margin_end(10);
> +
> +    $hbox->pack_start($scrolled_window, 1, 0, 5);
> +
> +    my $interfaces = Proxmox::Install::RunEnv::get()->{network}->{interfaces};
> +    my $mapping = Proxmox::Install::Config::get_network_interface_pin_map();
> +
> +    my $inputs = {};
> +    my $row = 0;
> +    for my $ifname (sort keys $interfaces->%*) {
> +        my $iface = $interfaces->{$ifname};
> +
> +        my $name = $mapping->{ $iface->{mac} };
> +        my $label_text = "$iface->{mac} ($ifname, $iface->{driver}, $iface->{state})";
> +
> +        # if the interface has addresses assigned through DHCP, show them for
> +        # reference
> +        if (defined($iface->{addresses})) {
> +            $label_text .=
> +                "\n  " . join(', ', map { "$_->{address}/$_->{prefix}" } @{ $iface->{addresses} });
> +        }
> +
> +        my ($label, $input) = create_text_input($name, $label_text);
> +        $label->set_xalign(0.);
> +
> +        $grid->attach($label, 0, $row, 1, 1);
> +        $grid->attach($input, 1, $row, 1, 1);
> +        $row++;
> +
> +        $inputs->{ $iface->{mac} } = $input;
> +    }
> +
> +    $hbox->show_all();
> +
> +    $dialog->signal_connect(
> +        response => sub {
> +            my $new_mapping = {};
> +            my $reverse_mapping = {};
> +            foreach my $mac (keys %$inputs) {
> +                my $name = $inputs->{$mac}->get_text();
> +
> +                if (!defined($name) || $name eq '') {
> +                    Proxmox::UI::message("interface name mapping for $mac cannot be empty");
> +                    $inputs->{$mac}->grab_focus();
> +                    return;
> +                }
> +
> +                if ($reverse_mapping->{$name}) {
> +                    Proxmox::UI::message(
> +                        "duplicate interface name mapping '$name' for: $mac, $reverse_mapping->{$name}"
> +                    );
> +                    $inputs->{$mac}->grab_focus();
> +                    return;
> +                }
> +
> +                if (length($name) > MAX_IFNAME_LEN) {
> +                    Proxmox::UI::message(
> +                        "interface name mapping '$name' for $mac cannot be longer than "
> +                            . MAX_IFNAME_LEN
> +                            . " characters");
> +                    $inputs->{$mac}->grab_focus();
> +                    return;
> +                }

same as for the TUI installer, imo the check for only-numeric interface
names would make sense. Or is there a reason it was omitted?

> +
> +                $new_mapping->{$mac} = $name;
> +                $reverse_mapping->{$name} = $mac;
> +            }
> +
> +            Proxmox::Install::Config::set_network_interface_pin_map($new_mapping);
> +            $dialog->destroy();
> +            $done_cb->();
> +        },
> +    );
> +
> +    $dialog->show();
> +    $dialog->run();
> +}
> +
>  sub create_ipconf_view {
>  
>      cleanup_view();
>      Proxmox::UI::display_html('ipconf.htm');
>  
> +    my $run_env = Proxmox::Install::RunEnv::get();
> +    my $ipconf = $run_env->{ipconf};
> +
>      my $grid = &$create_basic_grid();
>      $grid->set_row_spacing(10);
>      $grid->set_column_spacing(10);
> @@ -355,7 +461,7 @@ sub create_ipconf_view {
>  
>      my ($cidr_label, $cidr_box, $ipconf_entry_addr, $ipconf_entry_mask) = create_cidr_inputs($cidr);
>  
> -    my $device_model = Gtk3::ListStore->new('Glib::String', 'Glib::String');
> +    my $device_model = Gtk3::ListStore->new('Glib::String', 'Glib::String', 'Glib::String');
>      my $device_cb = Gtk3::ComboBox->new_with_model($device_model);
>      $device_cb->set_active(0);
>      $device_cb->set_visible(1);
> @@ -369,19 +475,59 @@ sub create_ipconf_view {
>      $device_cb->pack_start($cell, 0);
>      $device_cb->add_attribute($cell, 'text', 1);
>  
> -    my $get_device_desc = sub {
> -        my $iface = shift;
> -        return "$iface->{name} - $iface->{mac} ($iface->{driver})";
> +    my $refresh_device_cb = sub {
> +        # clear all entries and re-add them with their new names
> +        my $active = $device_cb->get_active();
> +        $device_model->clear();
> +
> +        my $mapping = Proxmox::Install::Config::get_network_interface_pin_map();
> +        my $i = 0;
> +        for my $index (sort keys $ipconf->{ifaces}->%*) {
> +            my $iface = $ipconf->{ifaces}->{$index};
> +            my $iter = $device_model->append();
> +
> +            my $symbol = "$iface->{state}" eq "UP" ? "\x{25CF}" : ' ';
> +            my $name = $gtk_state->{network_pinning_enabled} ? $mapping->{ $iface->{mac} } : $iface->{name};

This seems to need a `make tidy`?

> +
> +            $device_model->set(
> +                $iter,
> +                0 => $symbol,
> +                1 => "$name - $iface->{mac} ($iface->{driver})",
> +            );
> +            $i++;
> +        }
> +
> +        # re-set the currently active entry to keep the users selection
> +        $device_cb->set_active($active);
>      };
>  
> -    my $run_env = Proxmox::Install::RunEnv::get();
> -    my $ipconf = $run_env->{ipconf};
> +    my $name_pin_opts_button = Gtk3::Button->new('Options');
> +    $name_pin_opts_button->set_sensitive($gtk_state->{network_pinning_enabled});
> +    $name_pin_opts_button->signal_connect(
> +        clicked => sub {
> +            create_network_interface_pin_view($refresh_device_cb);
> +        },
> +    );
> +
> +    my $name_pin_checkbox = Gtk3::CheckButton->new('Pin network interface names');
> +    $name_pin_checkbox->set_active($gtk_state->{network_pinning_enabled});
> +    $name_pin_checkbox->signal_connect(
> +        toggled => sub {
> +            $name_pin_opts_button->set_sensitive(!!$name_pin_checkbox->get_active());
> +            $gtk_state->{network_pinning_enabled} = !!$name_pin_checkbox->get_active();
> +            $refresh_device_cb->();
> +        },
> +    );
>  
>      my ($device_active_map, $device_active_reverse_map) = ({}, {});
>  
>      my $device_change_handler = sub {
>          my $current = shift;
>  
> +        # happens during the clear + re-insertion of all interfaces after
> +        # the pinning changed, can be safely ignored
> +        return if $current->get_active() == -1;
> +
>          my $new = $device_active_map->{ $current->get_active() };
>          my $iface = $ipconf->{ifaces}->{$new};
>  
> @@ -389,6 +535,7 @@ sub create_ipconf_view {
>          return if defined($selected) && $iface->{name} eq $selected;
>  
>          Proxmox::Install::Config::set_mngmt_nic($iface->{name});
> +
>          $ipconf_entry_addr->set_text($iface->{inet}->{addr} || $iface->{inet6}->{addr})
>              if $iface->{inet}->{addr} || $iface->{inet6}->{addr};
>          $ipconf_entry_mask->set_text($iface->{inet}->{prefix} || $iface->{inet6}->{prefix})
> @@ -400,13 +547,6 @@ sub create_ipconf_view {
>      my $i = 0;
>      for my $index (sort keys $ipconf->{ifaces}->%*) {
>          my $iface = $ipconf->{ifaces}->{$index};
> -        my $iter = $device_model->append();
> -        my $symbol = "$iface->{state}" eq "UP" ? "\x{25CF}" : ' ';
> -        $device_model->set(
> -            $iter,
> -            0 => $symbol,
> -            1 => $get_device_desc->($iface),
> -        );
>          $device_active_map->{$i} = $index;
>          $device_active_reverse_map->{ $iface->{name} } = $i;
>  
> @@ -418,6 +558,9 @@ sub create_ipconf_view {
>          $i++;
>      }
>  
> +    # fill the combobox with entries
> +    $refresh_device_cb->();
> +
>      if (my $nic = Proxmox::Install::Config::get_mngmt_nic()) {
>          $initial_active_device_pos = $device_active_reverse_map->{$nic};
>      } else {
> @@ -443,7 +586,7 @@ sub create_ipconf_view {
>      $label->set_xalign(1.0);
>  
>      $grid->attach($label, 0, 0, 1, 1);
> -    $grid->attach($device_cb, 1, 0, 1, 1);
> +    $grid->attach($device_cb, 1, 0, 2, 1);
>  
>      my $fqdn = Proxmox::Install::Config::get_fqdn();
>      my $hostname = $run_env->{network}->{hostname} || $iso_env->{product};
> @@ -452,17 +595,17 @@ sub create_ipconf_view {
>  
>      my ($host_label, $hostentry) = create_text_input($fqdn, 'Hostname (FQDN)');
>      $grid->attach($host_label, 0, 1, 1, 1);
> -    $grid->attach($hostentry, 1, 1, 1, 1);
> +    $grid->attach($hostentry, 1, 1, 2, 1);
>  
>      $grid->attach($cidr_label, 0, 2, 1, 1);
> -    $grid->attach($cidr_box, 1, 2, 1, 1);
> +    $grid->attach($cidr_box, 1, 2, 2, 1);
>  
>      my $cfg_gateway = Proxmox::Install::Config::get_gateway();
>      my $gateway = $cfg_gateway // $ipconf->{gateway} || '192.168.100.1';
>  
>      my ($gw_label, $ipconf_entry_gw) = create_text_input($gateway, 'Gateway');
>      $grid->attach($gw_label, 0, 3, 1, 1);
> -    $grid->attach($ipconf_entry_gw, 1, 3, 1, 1);
> +    $grid->attach($ipconf_entry_gw, 1, 3, 2, 1);
>  
>      my $cfg_dns = Proxmox::Install::Config::get_dns();
>      my $dnsserver = $cfg_dns // $ipconf->{dnsserver} || $gateway;
> @@ -470,7 +613,10 @@ sub create_ipconf_view {
>      my ($dns_label, $ipconf_entry_dns) = create_text_input($dnsserver, 'DNS Server');
>  
>      $grid->attach($dns_label, 0, 4, 1, 1);
> -    $grid->attach($ipconf_entry_dns, 1, 4, 1, 1);
> +    $grid->attach($ipconf_entry_dns, 1, 4, 2, 1);
> +
> +    $grid->attach($name_pin_checkbox, 1, 5, 1, 1);
> +    $grid->attach($name_pin_opts_button, 2, 5, 1, 1);
>  
>      $gtk_state->{inbox}->show_all;
>      set_next(
> @@ -538,6 +684,8 @@ sub create_ipconf_view {
>              }
>              Proxmox::Install::Config::set_dns($dns_ip);
>  
> +            $gtk_state->{network_pinning_enabled} = !!$name_pin_checkbox->get_active();
> +
>              #print STDERR "TEST $ipaddress/$netmask $gateway_ip $dns_ip\n";
>  
>              $step_number++;
> @@ -573,6 +721,12 @@ sub create_ack_view {
>  
>      my $country = Proxmox::Install::Config::get_country();
>  
> +    my $mngmt_nic = Proxmox::Install::Config::get_mngmt_nic();
> +    my $iface = Proxmox::Install::RunEnv::get('network')->{interfaces}->{$mngmt_nic};
> +
> +    my $nic_mapping = Proxmox::Install::Config::get_network_interface_pin_map();
> +    my $interface = $gtk_state->{network_pinning_enabled} ? $nic_mapping->{ $iface->{mac} } : $iface->{name};

also `make tidy`



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


  parent reply	other threads:[~2025-10-21 14:05 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-10-14 13:21 [pve-devel] [PATCH installer 00/14] support network interface name pinning Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 01/14] test: parse-kernel-cmdline: fix module import statement Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 02/14] install: add support for network interface name pinning Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 03/14] run env: network: add kernel driver name to network interface info Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 04/14] common: utils: fix clippy warnings Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 05/14] common: setup: simplify network address list serialization Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 06/14] common: implement support for `network_interface_pin_map` config Christoph Heiss
2025-10-21 14:05   ` Michael Köppl
2025-10-14 13:21 ` [pve-devel] [PATCH installer 07/14] auto: add support for pinning network interface names Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 08/14] assistant: verify network settings in `validate-answer` subcommand Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 09/14] post-hook: avoid redundant Option<bool> for (de-)serialization Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 10/14] post-hook: add network interface name and pinning status Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 11/14] tui: views: move network options view to own module Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 12/14] tui: views: form: allow attaching user-defined data to children Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 13/14] tui: add support for pinning network interface names Christoph Heiss
2025-10-14 13:21 ` [pve-devel] [PATCH installer 14/14] gui: " Christoph Heiss
2025-10-14 15:04   ` Maximiliano Sandoval
2025-10-16 12:01     ` Christoph Heiss
2025-10-21 14:05   ` Michael Köppl [this message]
2025-10-21 14:04 ` [pve-devel] [PATCH installer 00/14] support network interface name pinning Michael Köppl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DDO232Q5XNFX.2VGDBBZ3VC83P@proxmox.com \
    --to=m.koeppl@proxmox.com \
    --cc=pve-devel-bounces@lists.proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal