[pve-devel] [PATCH installer 1/1] fix #2164: add option to define a vlan tag within the installer

[Florian Paul Azim Hoberg]

Hey Aaron, hey Christoph!

Thanks for your inputs. I already adjusted the code by Christoph’s suggestions which ensure that we have a proper validation of vlans (jftr, we start at vlan id 2 instead of 1 which is also special) and made the overview pages dynamically representing the given vlan tag id only when present. At this point, I just want to make sure this also fits your expectations to keep the review rounds short.

> Also; wondering if we maybe should just hide this if no VLAN tag is set,
> no not clutter the summary page unnecessarily?

Honestly, I have no better idea than this approach except of a complete refactor. So, I’m not sure if you’re fine with this approach within the GUI part where the HTML ack page would look like this:

	<tr><td>Management Interface:</td> <td>__interface__</td></tr>

        __vlan__

       <tr><td>Hostname:</td> <td>__hostname__</td></tr>

And the content gets set by:

my $vlan_row = defined $vlan ? "<tr><td>Management Interface VLAN tag:</td><td>$vlan</td></tr>" : '';

Which then gets pushed and set if present:

        if let Some(vlan) = self.network.vlan {
            summary.push(SummaryOption::new(
                "Management interface VLAN tag",
                &vlan.to_string(),
            ));
        }


> Instead of the dot notation, I would prefer the vlan-raw-device, vlan-id variant.
> This allows us to give the interface a name that matches the use, and the whole config is more explicit.

Would this fit to you?

        my $ifaces = "auto lo\niface lo inet loopback\n\n";
        my $ip_version = Proxmox::Install::Config::get_ip_version();
        my $ntype = $ip_version == 4 ? 'inet' : 'inet6';
        my $cidr = Proxmox::Install::Config::get_cidr();
        my $vlan = Proxmox::Install::Config::get_vlan();
        my $gateway = Proxmox::Install::Config::get_gateway();
        my $ethdev = Proxmox::Install::Config::get_mngmt_nic();
        my $bridgedev = "vmbr0";
        my $mgmtdev = "management";

        if ($iso_env->{cfg}->{bridged_network}) {
            $ifaces .= "iface $ethdev $ntype manual\n";
            $ifaces .=
                "\nauto $bridgedev\n"
                . "\tiface $bridgedev $ntype manual\n"
                . "\tbridge-ports $ethdev\n"
                . "\tbridge-stp off\n"
                . "\tbridge-fd 0\n"
                . "\n"
                "\nauto $mgmtdev\niface $mgmtdev $ntype static\n"
                . "\taddress $cidr\n"
                . "\tgateway $gateway\n"
                . "\tvlan-id $vlan\n"
                . "\tvlan-raw-device $bridgedev\n";
        } else {
            $ifaces .=
                "auto $ethdev\n"
                . "iface $ethdev $ntype manual\n"
                . "\n"
                "auto $mgmtdev\n"
                . "iface $mgmtdev $ntype static\n"
                . "\taddress $cidr\n"
                . "\tgateway $gateway\n"
                . "\tvlan-id $vlan\n"
                . "\tvlan-raw-device $ethdev\n";
        }


Happy to hear your feedback.

Thanks!