From 940fff10e6b0bfd419240406eaa709463a798def Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Tue, 11 Jul 2023 13:11:19 +0200
Subject: [PATCH 1/7] ncm-network: support custom get_current_config_post (for
 subclassing)

---
 ncm-network/src/main/perl/network.pm | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/ncm-network/src/main/perl/network.pm b/ncm-network/src/main/perl/network.pm
index 8843c7131f..88da4f80fb 100755
--- a/ncm-network/src/main/perl/network.pm
+++ b/ncm-network/src/main/perl/network.pm
@@ -467,6 +467,13 @@ sub test_network_ccm_fetch
     }
 }
 
+
+sub get_current_config_post
+{
+    my ($self) = @_;
+    return "";
+}
+
 # Gather current network configuration using available tools
 # Is gathered for debugging in case of failure.
 sub get_current_config
@@ -487,10 +494,6 @@ sub get_current_config
     $fh = CAF::FileReader->new($RESOLV_CONF, log => $self);
     $output .= "\n$RESOLV_CONF\n$fh";
 
-    # output of nmstate if we using nmstate configs
-    if ($self->_is_executable($NMSTATECTL) && $IFCFG_DIR =~ /nmstate/ ) {
-        $output .= $self->runrun([$NMSTATECTL, "show"]);
-    }
     # when brctl is missing, this would generate an error.
     # but it is harmless to skip the show command.
     if ($self->_is_executable($BRIDGECMD)) {
@@ -507,6 +510,8 @@ sub get_current_config
         $output .= "\nMissing $OVS_VCMD executable or socket.\n";
     };
 
+    $output .= $self->get_current_config_post();
+
     return $output;
 }
 

From 007355d0f398471d7dde92f0af54e402f9585f59 Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Tue, 11 Jul 2023 13:12:30 +0200
Subject: [PATCH 2/7] ncm-network: cleanup link_aggregation code a bit

---
 ncm-network/src/main/perl/network.pm | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/ncm-network/src/main/perl/network.pm b/ncm-network/src/main/perl/network.pm
index 88da4f80fb..ee0f1ced00 100755
--- a/ncm-network/src/main/perl/network.pm
+++ b/ncm-network/src/main/perl/network.pm
@@ -76,6 +76,7 @@ An example:
 
 =cut
 
+use 5.10.1;
 use parent qw(NCM::Component CAF::Path);
 
 our $EC = LC::Exception::Context->new->will_store_all;
@@ -786,15 +787,18 @@ sub process_network
             if (defined($opts) && keys %$opts) {
                 $iface->{$attr} = [map {"$_=$opts->{$_}"} sort keys %$opts];
                 $self->debug(1, "Replaced $attr with ", join(' ', @{$iface->{$attr}}), " for interface $ifname");
+
                 # for bonding_opts, we need linkagregation settings for nmstate.
                 # this should not impact existing configs as it adds interface/$name/link_aggregation
                 if ($attr eq "bonding_opts"){
                     foreach my $opt (sort keys %$opts){
-                        if ($opt eq 'mode'){
-                            $iface->{link_aggregation}->{mode} = $opts->{mode};
-                        }else{
-                            $iface->{link_aggregation}->{options}->{$opt} = $opts->{$opt};
+                        $iface->{link_aggregation} ||= {};
+                        my $la = $iface->{link_aggregation};
+                        if ($opt ne 'mode') {
+                            $la->{options} ||= {};
+                            $la = $la->{options};
                         }
+                        $la->{$opt} = $opts->{$opt};
                     }
                 }
                 # TODO for briging_opts

From 4deb7d08ca055cfefa827623b407f4ca194654f5 Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Tue, 11 Jul 2023 13:13:37 +0200
Subject: [PATCH 3/7] ncm-network: export common readonly for subclassing

---
 ncm-network/src/main/perl/network.pm |  30 +++---
 ncm-network/src/main/perl/nmstate.pm | 141 +++++++++++++++------------
 2 files changed, 91 insertions(+), 80 deletions(-)

diff --git a/ncm-network/src/main/perl/network.pm b/ncm-network/src/main/perl/network.pm
index ee0f1ced00..97324f3f15 100755
--- a/ncm-network/src/main/perl/network.pm
+++ b/ncm-network/src/main/perl/network.pm
@@ -147,7 +147,6 @@ Readonly my $IPROUTE => [qw(ip route show)];
 Readonly my $OVS_VCMD => '/usr/bin/ovs-vsctl';
 Readonly my $HOSTNAME_CMD => '/usr/bin/hostnamectl';
 Readonly my $ROUTING_TABLE => '/etc/iproute2/rt_tables';
-Readonly my $NMSTATECTL => '/usr/bin/nmstatectl';
 
 Readonly my $NETWORK_PATH => '/system/network';
 Readonly my $HARDWARE_PATH => '/hardware/cards/nic';
@@ -181,6 +180,7 @@ Readonly my $DEVICE_REGEXP => qr{
 
 my $IFCFG_DIR = "/etc/sysconfig/network-scripts";
 my $BACKUP_DIR = "$IFCFG_DIR/.quattorbackup";
+
 Readonly my $NETWORKCFG => "/etc/sysconfig/network";
 
 Readonly my $RESOLV_CONF => '/etc/resolv.conf';
@@ -188,6 +188,7 @@ Readonly my $RESOLV_CONF_SAVE => '/etc/resolv.conf.save';
 Readonly my $RESOLV_SUFFIX => '.ncm-network';
 
 Readonly my $FAILED_SUFFIX => '-failed';
+
 Readonly my $REMOVE => -1;
 Readonly my $NOCHANGES => 0;
 Readonly my $UPDATED => 1;
@@ -195,22 +196,17 @@ Readonly my $NEW => 2;
 # changes to file, but same config (eg for new file formats)
 Readonly my $KEEPS_STATE => 3;
 
-# methods needed to get values from other modules.
-sub keeps_state { $KEEPS_STATE }
-sub updated_state { $UPDATED }
-sub remove_state { $REMOVE }
-sub nochanges_state { $NOCHANGES }
-sub new_state { $NEW }
-sub networkcfg { $NETWORKCFG }
-sub resolv_conf { $RESOLV_CONF }
-sub resolv_conf_save { $RESOLV_CONF_SAVE }
-sub resolv_suffix { $RESOLV_SUFFIX }
-sub failed_suffix { $FAILED_SUFFIX }
-sub network_path { $NETWORK_PATH }
-sub hostname_cmd { $HOSTNAME_CMD }
-sub nmstatectl { $NMSTATECTL }
-
-sub set_cfg_dir {
+
+# automatic exports
+our @EXPORT = qw($FAILED_SUFFIX
+    $REMOVE $NOCHANGES $UPDATED $NEW $KEEPS_STATE
+    $RESOLV_CONF $RESOLV_CONF_SAVE $RESOLV_SUFFIX
+    $NETWORKCFG $NETWORK_PATH $HOSTNAME_CMD
+    );
+
+
+sub set_cfg_dir
+{
     my ($self, $cfg_dir) = @_;
     $IFCFG_DIR = $cfg_dir;
     $BACKUP_DIR = "$IFCFG_DIR/.quattorbackup";
diff --git a/ncm-network/src/main/perl/nmstate.pm b/ncm-network/src/main/perl/nmstate.pm
index ad8d868650..aefcec0069 100644
--- a/ncm-network/src/main/perl/nmstate.pm
+++ b/ncm-network/src/main/perl/nmstate.pm
@@ -11,7 +11,7 @@ The I<network> component sets the network settings through C<< /etc/sysconfig/ne
 and the NM keyfile settings in files C<< /etc/NetworkManager/system-connections >>.
 
 New/changed settings are first tested by retrieving the latest profile from the
-CDB server (using ccm-fetch). 
+CDB server (using ccm-fetch).
 If this fails, the component reverts all settings to the previous values. This is no different to network module.
 
 During this test, a sleep value of 15 seconds is used to make sure the restarted network
@@ -22,12 +22,14 @@ Because of this, configuration changes may cause the ncm-ncd run to take longer
 Be aware that configuration changes can also lead to a brief network interruption.
 =cut
 
-use parent qw (NCM::Component::network);
+use parent qw(NCM::Component::network);
+use NCM::Component::network;  # required for the import of the (default) exports
 
 our $EC = LC::Exception::Context->new->will_store_all;
 use EDG::WP4::CCM::TextRender;
 use Readonly;
 
+Readonly my $NMSTATECTL => '/usr/bin/nmstatectl';
 Readonly my $NMCFG_DIR => "/etc/nmstate";
 Readonly my $NMCLI_CMD => '/usr/bin/nmcli';
 
@@ -41,17 +43,16 @@ sub is_valid_interface
 
     # Very primitive, based on regex only
     # matchs eth0.yml bond0.yml, or bond0.101.yml
-if (
-
-    $filename =~ m{
-        # Filename is either right at the beginning or following a slash
-        (?: \A | / )
-        # $1 will capture for example:
-        # eth0  bond1  eth0.101  bond0.102
-        ( \w+ \d+ (?: \. \d+ )? )
-        # Suffix (not captured)
-        \. yml \z
-    }x
+    if (
+        $filename =~ m{
+            # Filename is either right at the beginning or following a slash
+            (?: \A | / )
+            # $1 will capture for example:
+            # eth0  bond1  eth0.101  bond0.102
+            ( \w+ \d+ (?: \. \d+ )? )
+            # Suffix (not captured)
+            \. yml \z
+        }x
     ) {
         # name and id for nmstate, this will make connection id and name the same.
         my $name = $1;
@@ -63,7 +64,7 @@ if (
 }
 
 # By default, NetworkManager on Red Hat Enterprise Linux (RHEL) 8+ dynamically updates the /etc/resolv.conf
-# file with the DNS settings from active NetworkManager connection profiles. we manage this using ncm-resolver. 
+# file with the DNS settings from active NetworkManager connection profiles. we manage this using ncm-resolver.
 # so disable this unless nm_manage_dns = true. resolver details can be set using nmstate but not doing this now.
 sub disable_nm_manage_dns
 {
@@ -132,13 +133,13 @@ sub make_nm_ip_route
         } else {
              if ($route->{netmask}){
                  my $dest_addr = NetAddr::IP->new($route->{address}."/".$route->{netmask});
-                 $rt{destination} = $dest_addr->cidr;    
+                 $rt{destination} = $dest_addr->cidr;
              } else {
                 # if no netmask defined for a route, assume its single ip
                 $rt{destination} = $route->{address}."/32";
              }
-        }        
-        $rt{table_id} = "$routing_table_hash->{$route->{table}}" if $route->{table};    
+        }
+        $rt{table_id} = "$routing_table_hash->{$route->{table}}" if $route->{table};
         $rt{next_hop_interface} = $device;
         $rt{next_hop_address} = $route->{gateway} if $route->{gateway};
         push (@rt_entry, \%rt);
@@ -147,22 +148,24 @@ sub make_nm_ip_route
     return \@rt_entry;
 }
 
-# group all eth bound to a bond together in a hashref for to be used as 
+# group all eth bound to a bond together in a hashref for to be used as
 # - port in nmstate config file
-sub get_bonded_eth {
+sub get_bonded_eth
+{
     my ($self, $interfaces) = @_;
     my @data =  ();
     foreach my $name (sort keys %$interfaces) {
         my $iface = $interfaces->{$name};
         if ( $iface->{master} ){
-            push @data, $name; 
+            push @data, $name;
         }
     }
     return \@data;
 }
 
 # wirtes the nmstate yml file, uses nmstate/interface.tt module.
-sub nmstate_file_dump {
+sub nmstate_file_dump
+{
     my ($self, $filename, $ifaceconfig) = @_;
     my $net_module = 'nmstate/interface';
     my $changes = 0;
@@ -174,7 +177,7 @@ sub nmstate_file_dump {
     }
 
     if (!$self->file_exists($filename) || $self->mk_bu($filename, $testcfg)) {
-    
+
     my $trd = EDG::WP4::CCM::TextRender->new($net_module, $ifaceconfig, relpath => 'network');
     if (! defined($trd->get_text())) {
         $self->error ("Unable to generate network config $filename: $trd->{fail}");
@@ -187,10 +190,10 @@ sub nmstate_file_dump {
     if ($fh->close()) {
             if ($self->file_exists($filename)) {
                 $self->info("$func: file $filename has newer version scheduled.");
-                $filestatus = $self->updated_state();
+                $filestatus = $UPDATED;
             } else {
                 $self->info("$func: new file $filename scheduled.");
-                $filestatus = $self->new_state();
+                $filestatus = $NEW;
             }
     } else {
         my $is_active = is_active_interface($self, $ifaceconfig->{name});
@@ -200,9 +203,9 @@ sub nmstate_file_dump {
             # this will allow nm to report issues with config on every run or if someone deletes the conneciton.
             # if no changes to file, then this will never get applied again.
             $self->info("$func: file $filename has no active conneciton, scheduled for update.");
-            $filestatus = $self->updated_state();    
+            $filestatus = $UPDATED;
         } else {
-            $filestatus = $self->nochanges_state();
+            $filestatus = $NOCHANGES;
             # they're equal, remove backup files
             $self->verbose("$func: no changes scheduled for file $filename. Cleaning up.");
             $self->cleanup_backup_test($filename);
@@ -216,7 +219,8 @@ sub nmstate_file_dump {
 
 # generates the hasrefs for interface used by nmstate/interface.tt module.
 # bulk of the config settings needed by the nmstate yml is done here.
-sub generate_nmstate_config {
+sub generate_nmstate_config
+{
     my ($self, $name, $net, $ipv6, $routing_table) = @_;
 
     my $bonded_eth = get_bonded_eth($self, $net->{interfaces});
@@ -228,8 +232,8 @@ sub generate_nmstate_config {
     my $is_vlan_eth = exists $iface->{vlan} ? 1 : 0;
     my $is_bond_eth = exists $iface->{master} ? 1 : 0;
     my $iface_changed = 0;
-    
-    # create hash of interface entries that will be used by nmstate config.   
+
+    # create hash of interface entries that will be used by nmstate config.
     my $ifaceconfig->{name} = $name;
     $ifaceconfig->{device} = $device;
     if ($is_eth) {
@@ -242,7 +246,7 @@ sub generate_nmstate_config {
     } elsif ($is_vlan_eth) {
         my $vlan_id = $name;
         # replace eveytthing upto and include . to get vlan id of the interface.
-        $vlan_id =~ s/^[^.]*.//;;    
+        $vlan_id =~ s/^[^.]*.//;;
         $ifaceconfig->{type} = "vlan";
         $ifaceconfig->{vlan}->{base_iface} = $iface->{physdev};
         $ifaceconfig->{vlan}->{vlan_id} = $vlan_id;
@@ -254,7 +258,7 @@ sub generate_nmstate_config {
             $ifaceconfig->{link_aggregation}->{port} = $bonded_eth;
         }
     }
-    
+
     if ($eth_bootproto eq 'static') {
         $ifaceconfig->{state} = "up";
         if ($is_ip) {
@@ -277,13 +281,13 @@ sub generate_nmstate_config {
                 $self->warn("ipv6 addr found but not supported");
                 # TODO create ipv6.address entries here. i.e
                 #$ifaceconfig->{ipv6}->{address} = [$ipv6_list];
-                #.tt module support is added.                
+                #.tt module support is added.
             } else {
                 $self->verbose("no ipv6 entries");
             }
         }
     } elsif (($eth_bootproto eq "none") && (!$is_bond_eth)) {
-            # no ip on interface and is not a bond eth, assume not managed so disable eth. 
+            # no ip on interface and is not a bond eth, assume not managed so disable eth.
             $ifaceconfig->{enabled} = "false";
             $ifaceconfig->{state} = "down";
     }
@@ -296,7 +300,7 @@ sub generate_nmstate_config {
     }
     # combined default route with any policy routing/rule, if any
     # combination of default route, plus any additional policy routes.
-    # read and set by tt module as 
+    # read and set by tt module as
     # routes:
     #   config:
     #   - desitionation:
@@ -322,7 +326,7 @@ sub generate_nmstate_config {
         $ifaceconfig->{routes}->{config} = $routes;
     }
     #print (YAML::XS::Dump($ifaceconfig));
-    
+
     # TODO: ethtool settings to add in config file? setting via cmd cli working as is.
     # TODO: aliases ip addresses
     # TODO: bridge_options
@@ -334,7 +338,7 @@ sub generate_nmstate_config {
 
 # enable NetworkManager service
 #   without enabled network service, this component is pointless
-#  
+#
 sub enable_network_service
 {
     my ($self) = @_;
@@ -344,7 +348,7 @@ sub enable_network_service
 
 # keep nmstate service disbaled (vendor preset anyway), we will apply config ncm component.
 # nmstate service applies all files found in /etc/nmstate and changes to .applied, which will keep change if component is managing the .yml file.
-#   
+#
 sub disable_nmstate_service
 {
     my ($self) = @_;
@@ -357,7 +361,7 @@ sub disable_nmstate_service
 sub is_active_interface
 {
     my ($self, $ifacename) = @_;
-    my $output = $self->runrun(["$NMCLI_CMD -f name conn show --active"]);
+    my $output = $self->runrun([$NMCLI_CMD, "-f", "name", "conn", "show", "--active"]);
     my @existing_conn = split('\n', $output);
     my %current_conn;
     my $found = 0;
@@ -377,7 +381,7 @@ sub clear_default_nm_connections
     my ($self) = @_;
     # NM creates auto connections with Wired connection x
     # Delete all connections with name 'Wired connection', everything ncm-network creates will have connection name set to interface name.
-    my $output = $self->runrun(["$NMCLI_CMD -f name conn"]);
+    my $output = $self->runrun([$NMCLI_CMD, "-f", "name", "conn"]);
     my @existing_conn = split('\n', $output);
     my %current_conn;
     foreach  my $conn_name  (@existing_conn) {
@@ -386,7 +390,7 @@ sub clear_default_nm_connections
             $self->verbose("Clearing default connections created automatically by NetworkManager [ $conn_name ]");
             $output = $self->runrun([$NMCLI_CMD,"conn", "delete", $conn_name]);
             $self->verbose($output);
-        } 
+        }
     }
 }
 
@@ -395,16 +399,16 @@ sub nmstate_apply
     my ($self, $exifiles, $ifup, $nwsrv) = @_;
 
     my @ifaces = sort keys %$ifup;
-    my $nwstate = $exifiles->{$self->networkcfg()};
-    
+    my $nwstate = $exifiles->{$NETWORKCFG};
+
     my $action;
     my $nmstateclt_cmd = $self->nmstatectl();
     $self->verbose("Apply config using nmstatectl for each interface");
-    if (($nwstate == $self->updated_state()) || ($nwstate == $self->new_state())) {
+    if (($nwstate == $UPDATED) || ($nwstate == $NEW)) {
         # Do not need to start networking in nmstate.
-        #$self->verbose($self->networkcfg(), ($nwstate == $self->new_state() ? 'NEW' : 'UPDATED'), " starting network");
+        #$self->verbose($NETWORKCFG, ($nwstate == $NEW ? 'NEW' : 'UPDATED'), " starting network");
         $action = 1;
-    } 
+    }
     if (@ifaces) {
         $self->info("Applying changes using $nmstateclt_cmd ",join(', ', @ifaces));
         my @cmds;
@@ -414,7 +418,7 @@ sub nmstate_apply
             # apply config using nmstatectl
             my $ymlfile = "$NMCFG_DIR/$iface.yml";
             if ($self->any_exists($ymlfile)){
-                push(@cmds, ["$nmstateclt_cmd apply $ymlfile"]);
+                push(@cmds, [$nmstateclt_cmd, "apply", $ymlfile]);
                 push(@cmds, [qw(sleep 10)]) if ($iface =~ m/bond/);
             } else {
                 # do we down the interface?
@@ -432,19 +436,29 @@ sub nmstate_apply
     return $action;
 }
 
+
+sub get_current_config_post
+{
+    my ($self) = @_;
+
+    # output of nmstate
+    return $self->runrun([$NMSTATECTL, "show"]);
+}
+
+
 sub Configure
 {
     my ($self, $config) = @_;
 
     $self->set_cfg_dir($NMCFG_DIR);
     return if ! defined($self->init_backupdir());
-    
+
     # current setup, will be printed in case of major failure
     my $init_config = $self->get_current_config();
     # The original, assumed to be working resolv.conf
     # Using an FileEditor: it will read the current content, so we can do a close later to save it
     # in case something changed it behind our back.
-    my $resolv_conf_fh = CAF::FileEditor->new($self->resolv_conf(), backup => $self->resolv_suffix(), log => $self);
+    my $resolv_conf_fh = CAF::FileEditor->new($RESOLV_CONF, backup => $RESOLV_SUFFIX, log => $self);
     # Need to reset the original content (otherwise the close will not check the possibly updated content on disk)
     *$resolv_conf_fh->{original_content} = undef;
 
@@ -457,31 +471,31 @@ sub Configure
     return if ! defined($exifiles);
 
     my $comp_tree = $config->getTree($self->prefix());
-    my $nwtree = $config->getTree($self->network_path());
+    my $nwtree = $config->getTree($NETWORK_PATH);
 
     # no backup, restart or anything else required
     $self->routing_table($nwtree->{routing_table});
 
     # main network config
-    return if ! defined($self->mk_bu($self->networkcfg()));
+    return if ! defined($self->mk_bu($NETWORKCFG));
 
     my $hostname = $nwtree->{realhostname} || "$nwtree->{hostname}.$nwtree->{domainname}";
 
-    my $use_hostnamectl = $self->_is_executable($self->hostname_cmd());
+    my $use_hostnamectl = $self->_is_executable($HOSTNAME_CMD);
     # if hostnamectl exists, do not set it via the network config file
     # systemd rpm --script can remove it anyway
     my $nwcfg_hostname = $use_hostnamectl ? undef : $hostname;
 
     my ($text, $ipv6) = $self->make_network_cfg($nwtree, $net, $nwcfg_hostname);
-    $exifiles->{$self->networkcfg()} = $self->file_dump($self->networkcfg(), $text);
+    $exifiles->{$NETWORKCFG} = $self->file_dump($NETWORKCFG, $text);
 
-    if ($exifiles->{$self->networkcfg()} == $self->updated_state() && $use_hostnamectl) {
+    if ($exifiles->{$NETWORKCFG} == $UPDATED && $use_hostnamectl) {
         # Network config was updated, check if it was due to removal of HOSTNAME
         # when hostnamectl is present.
         my ($hntext, $hnipv6) = $self->make_network_cfg($nwtree, $net, $hostname);
-        $self->legacy_keeps_state($self->networkcfg(), $hntext, $exifiles);
+        $self->legacy_keeps_state($NETWORKCFG, $hntext, $exifiles);
     };
-    
+
     foreach my $ifacename (sort keys %$ifaces) {
         my $iface = $ifaces->{$ifacename};
         my $nmstate_cfg = generate_nmstate_config($self, $ifacename, $net, $ipv6, $nwtree->{routing_table});
@@ -492,7 +506,7 @@ sub Configure
         #$self->default_broadcast_keeps_state($file_name, $ifacename, $iface, $exifiles, 0);
         $self->ethtool_opts_keeps_state($file_name, $ifacename, $iface, $exifiles);
 
-        if ($exifiles->{$file_name} == $self->updated_state()) {
+        if ($exifiles->{$file_name} == $UPDATED) {
             # interface configuration was changed
             # check if this was due to addition of resolv_mods / peerdns
             my $no_resolv = $self->make_ifcfg($ifacename, $iface, $ipv6, resolv_mods => 0, peerdns => 0);
@@ -529,7 +543,7 @@ sub Configure
 
     # Record any changes wrt the init config (e.g. due to stopping of NetworkManager)
     $init_config .= "\nPRE APPLY\n";
-   
+
     $init_config .= $self->get_current_config();
 
     # restart network
@@ -540,12 +554,12 @@ sub Configure
     #   2. replace updated/new config; remove REMOVE
     #   3. (re)start things
     my $nwsrv = CAF::Service->new(['NetworkManager'], log => $self);
-    
+
     # NetworkManager manages dns by default, but we manage dns with ncm-resolver, new option to eanble/disable it.
     $self->disable_nm_manage_dns($nwtree->{nm_manage_dns}, $nwsrv);
 
     # nmstate files are applied uinsg nmstate apply via this componant. We don't want nmstate svc to manage it.
-    # If nmstate svc manages the files, it will apply the config for any files found in /etc/nmstate with .yml extension. Once the config is applied, 
+    # If nmstate svc manages the files, it will apply the config for any files found in /etc/nmstate with .yml extension. Once the config is applied,
     # the file name changes to .applied, which won't be ideal if ncm-component is managing .yml files.
     # for this reason we don't really need nmstate service running. It comes disabled by default anyway.
     $self->disable_nmstate_service();
@@ -557,7 +571,7 @@ sub Configure
     # (most likely in this scenario saved by ifup-post)
     # and leave a system without configured DNS (which ncm-network can't recover from,
     # as it does not manage /etc/resolv.conf). Without working DNS, the ccm-fetch network test will probably fail.
-    $self->move($self->resolv_conf_save(), $self->resolv_conf_save().$self->resolv_suffix());
+    $self->move($RESOLV_CONF_SAVE, $RESOLV_CONF_SAVE.$RESOLV_SUFFIX);
 
     # only need to deploy config.
     my $config_changed = $self->deploy_config($exifiles);
@@ -614,8 +628,8 @@ sub Configure
             $self->cleanup_backup_test($file);
         }
 
-        $self->cleanup($self->resolv_conf().$self->resolv_suffix());
-        $self->cleanup($self->resolv_conf_save().$self->resolv_suffix());
+        $self->cleanup($RESOLV_CONF.$RESOLV_SUFFIX);
+        $self->cleanup($RESOLV_CONF_SAVE.$RESOLV_SUFFIX);
     }
 
     # remove all broken links: use file_exists
@@ -632,4 +646,5 @@ sub Configure
 
     return 1;
 }
-1;
\ No newline at end of file
+
+1;

From 4d16d168c7e9a80e10d57e77c0ce7c942f1a5500 Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Tue, 11 Jul 2023 15:49:30 +0200
Subject: [PATCH 4/7] ncm-network: more nmstate cleanup

---
 ncm-network/src/main/perl/network.pm          | 63 ++++++++-------
 ncm-network/src/main/perl/nmstate.pm          | 53 ++++++------
 .../src/main/resources/nmstate/interface.tt   | 80 -------------------
 ncm-network/src/test/perl/nmstate_simple.t    |  1 -
 .../src/test/resources/nmstate-simple.pan     |  6 --
 5 files changed, 62 insertions(+), 141 deletions(-)
 delete mode 100644 ncm-network/src/main/resources/nmstate/interface.tt
 delete mode 100644 ncm-network/src/test/resources/nmstate-simple.pan

diff --git a/ncm-network/src/main/perl/network.pm b/ncm-network/src/main/perl/network.pm
index 97324f3f15..dc3040cdfc 100755
--- a/ncm-network/src/main/perl/network.pm
+++ b/ncm-network/src/main/perl/network.pm
@@ -178,9 +178,6 @@ Readonly my $DEVICE_REGEXP => qr{
     $
 }x;
 
-my $IFCFG_DIR = "/etc/sysconfig/network-scripts";
-my $BACKUP_DIR = "$IFCFG_DIR/.quattorbackup";
-
 Readonly my $NETWORKCFG => "/etc/sysconfig/network";
 
 Readonly my $RESOLV_CONF => '/etc/resolv.conf';
@@ -189,6 +186,8 @@ Readonly my $RESOLV_SUFFIX => '.ncm-network';
 
 Readonly my $FAILED_SUFFIX => '-failed';
 
+Readonly my $BACKUP_DIR_SUFFIX => '.quattorbackup';
+
 Readonly my $REMOVE => -1;
 Readonly my $NOCHANGES => 0;
 Readonly my $UPDATED => 1;
@@ -196,22 +195,23 @@ Readonly my $NEW => 2;
 # changes to file, but same config (eg for new file formats)
 Readonly my $KEEPS_STATE => 3;
 
-
-# automatic exports
+# automatic exports of readonlys
 our @EXPORT = qw($FAILED_SUFFIX
     $REMOVE $NOCHANGES $UPDATED $NEW $KEEPS_STATE
     $RESOLV_CONF $RESOLV_CONF_SAVE $RESOLV_SUFFIX
     $NETWORKCFG $NETWORK_PATH $HOSTNAME_CMD
     );
 
+# list of constants to allow inheritance via $self->CONSTANTNAME
+use constant IFCFG_DIR => "/etc/sysconfig/network-scripts";
 
-sub set_cfg_dir
+sub backup_dir
 {
-    my ($self, $cfg_dir) = @_;
-    $IFCFG_DIR = $cfg_dir;
-    $BACKUP_DIR = "$IFCFG_DIR/.quattorbackup";
+    my ($self) = @_;
+    return $self->IFCFG_DIR . "/$BACKUP_DIR_SUFFIX";
 }
 
+
 # wrapper around -x for easy unittesting
 # is not part of CAF::Path
 sub _is_executable
@@ -283,6 +283,12 @@ sub ethtool_get_current
     return %current;
 }
 
+sub iface_filename
+{
+    my ($self, $iface) = @_;
+    return $self->IFCFG_DIR . "/ifcfg-$iface";
+}
+
 # backup_filename: returns backup filename for given file
 sub backup_filename
 {
@@ -291,7 +297,7 @@ sub backup_filename
     my $back = "$file";
     $back =~ s/\//_/g;
 
-    return "$BACKUP_DIR/$back";
+    return $self->backup_dir() . "/$back";
 }
 
 # Generate the filename to hold the test configuration data
@@ -480,8 +486,8 @@ sub get_current_config
     my $fh = CAF::FileReader->new($NETWORKCFG, log => $self);
     my $output = "$NETWORKCFG\n$fh";
 
-    $output .= "\nls -lrt $IFCFG_DIR\n";
-    $output .= $self->runrun(['ls', '-ltr', $IFCFG_DIR]);
+    $output .= "\nls -lrt " . $self->IFCFG_DIR . "\n";
+    $output .= $self->runrun(['ls', '-ltr', $self->IFCFG_DIR]);
 
     $output .= "\n@$IPADDR\n";
     $output .= $self->runrun($IPADDR);
@@ -895,16 +901,16 @@ sub gather_existing
     my (%exifiles, %exilinks);
 
     # read current config
-    my $files = $self->listdir($IFCFG_DIR, test => sub { return $self->is_valid_interface($_[0]); });
+    my $files = $self->listdir($self->IFCFG_DIR, test => sub { return $self->is_valid_interface($_[0]); });
     foreach my $filename (@$files) {
         if ($filename =~ m/^([:\w.-]+)$/) {
             $filename = $1; # untaint
         } else {
-            $self->warn("Cannot untaint filename $IFCFG_DIR/$filename. Skipping");
+            $self->warn("Cannot untaint filename " . $self->IFCFG_DIR . "/$filename. Skipping");
             next;
         }
 
-        my $file = "$IFCFG_DIR/$filename";
+        my $file = $self->IFCFG_DIR . "/$filename";
 
         my $msg;
         if ($self->is_symlink($file)) {
@@ -1348,8 +1354,7 @@ sub make_ifdown
         my $valid = $self->is_valid_interface($file);
         if ($valid) {
             my ($iface, $ifupdownname) = @$valid;
-            my $cfg_filename = "$IFCFG_DIR/ifcfg-$iface";
-            $cfg_filename = "$IFCFG_DIR/$iface.yml" if $IFCFG_DIR =~ /nmstate/;
+            my $cfg_filename = $self->iface_filename($iface);
             # ifdown: all devices that have files with non-zero status
             if ($value == $NOCHANGES) {
                 $self->verbose("No changes for interface $iface (cfg file $file)");
@@ -1428,8 +1433,8 @@ sub make_ifup
         # and have state other than REMOVE
         # e.g. master with NOCHANGES state can be added here
         # when a slave had modifications
-        my $cfg_filename = "$IFCFG_DIR/ifcfg-$iface";
-        $cfg_filename = "$IFCFG_DIR/$iface.yml" if $IFCFG_DIR =~ /nmstate/;
+        my $cfg_filename = $self->iface_filename($iface);
+
         if (exists($exifiles->{"$cfg_filename"}) &&
             $exifiles->{"$cfg_filename"} == $REMOVE) {
             $self->verbose("Not starting $iface scheduled for removal");
@@ -1627,7 +1632,7 @@ sub recover
 
     $self->error("Network restart failed. Reverting back to original config. ",
                  "Failed modified configfiles can be found in ",
-                 "$BACKUP_DIR with suffix $FAILED_SUFFIX. ",
+                 $self->backup_dir() . " with suffix $FAILED_SUFFIX. ",
                  "(If there aren't any, it means only some devices were removed.)");
 
     # stop/recover/start whole network is the only thing that should always work.
@@ -1717,12 +1722,12 @@ sub init_backupdir
 {
     my $self = shift;
 
-    if (!defined($self->cleanup($BACKUP_DIR, undef, keeps_state => 1))) {
-        $self->error("Failed to cleanup previous backup directory $BACKUP_DIR: $self->{fail}");
+    if (!defined($self->cleanup($self->backup_dir(), undef, keeps_state => 1))) {
+        $self->error("Failed to cleanup previous backup directory " . $self->backup_dir() . ": $self->{fail}");
         return;
     }
-    if (!defined($self->directory($BACKUP_DIR, mode => oct(700), keeps_state => 1))) {
-        $self->error("Failed to create backup directory $BACKUP_DIR: $self->{fail}");
+    if (!defined($self->directory($self->backup_dir(), mode => oct(700), keeps_state => 1))) {
+        $self->error("Failed to create backup directory " . $self->backup_dir() . ": $self->{fail}");
         return;
     }
 
@@ -2036,7 +2041,7 @@ sub Configure
         my $iface = $ifaces->{$ifacename};
         my $text = $self->make_ifcfg($ifacename, $iface, $ipv6);
 
-        my $file_name = "$IFCFG_DIR/ifcfg-$ifacename";
+        my $file_name = $self->iface_filename($ifacename);
         $exifiles->{$file_name} = $self->file_dump($file_name, $text);
 
         $self->default_broadcast_keeps_state($file_name, $ifacename, $iface, $exifiles, 0);
@@ -2057,13 +2062,13 @@ sub Configure
                 # pass device, not system interface name
                 my $text = $self->$method($flavour, $iface->{device} || $ifacename, $iface->{$flavour});
 
-                my $file_name = "$IFCFG_DIR/$flavour-$ifacename";
+                my $file_name = $self->IFCFG_DIR . "/$flavour-$ifacename";
                 $exifiles->{$file_name} = $self->file_dump($file_name, $text);
             }
         }
 
         # legacy IPv4 format
-        $file_name = "$IFCFG_DIR/route-$ifacename";
+        $file_name = $self->IFCFG_DIR . "/route-$ifacename";
         if (exists($exifiles->{$file_name}) && $exifiles->{$file_name} == $UPDATED) {
             # IPv4 route data was modified.
             # Check if it was due to conversion of legacy format or
@@ -2079,7 +2084,7 @@ sub Configure
             my $al_iface = $iface->{aliases}->{$al};
             my $text = $self->make_ifcfg_alias($al_dev, $al_iface);
 
-            my $file_name = "$IFCFG_DIR/ifcfg-$ifacename:$al";
+            my $file_name = $self->IFCFG_DIR . "/ifcfg-$ifacename:$al";
             $exifiles->{$file_name} = $self->file_dump($file_name, $text);
 
             $self->default_broadcast_keeps_state($file_name, $al_dev, $al_iface, $exifiles, 1);
@@ -2092,7 +2097,7 @@ sub Configure
             # Problem is, we want both
             # Adding symlinks however is not the best thing to do.
 
-            my $file_name_sym = "$IFCFG_DIR/ifcfg-$al_dev";
+            my $file_name_sym = $self->IFCFG_DIR . "/ifcfg-$al_dev";
             if ($iface->{vlan} &&
                 $file_name_sym ne $file_name &&
                 ! $self->any_exists($file_name_sym)) { # TODO: should check target with readlink
diff --git a/ncm-network/src/main/perl/nmstate.pm b/ncm-network/src/main/perl/nmstate.pm
index aefcec0069..e00d08960e 100644
--- a/ncm-network/src/main/perl/nmstate.pm
+++ b/ncm-network/src/main/perl/nmstate.pm
@@ -30,9 +30,16 @@ use EDG::WP4::CCM::TextRender;
 use Readonly;
 
 Readonly my $NMSTATECTL => '/usr/bin/nmstatectl';
-Readonly my $NMCFG_DIR => "/etc/nmstate";
 Readonly my $NMCLI_CMD => '/usr/bin/nmcli';
 
+use constant IFCFG_DIR => "/etc/nmstate";
+
+sub iface_filename
+{
+    my ($self, $iface) = @_;
+    return $self->IFCFG_DIR . "/$iface.yml";
+}
+
 # Given the configuration in nmconnection
 # Determine if this is a valid interface for ncm-network to manage,
 # Return arrayref tuple [interface name, ifdown/ifup name] when valid,
@@ -74,7 +81,7 @@ sub disable_nm_manage_dns
     push @data, 'dns=none';
     if ( (defined($manage_dns) && !$manage_dns) || (! defined($manage_dns)) ) {
         $self->verbose("Configuring networkmanager not to manage resolv.conf");
-        my $fh = CAF::FileWriter->new($filename, mode =>0444, log => $self, keeps_state => 1);
+        my $fh = CAF::FileWriter->new($filename, mode => oct(444), log => $self, keeps_state => 1);
         print $fh join("\n", @data, '');
         if ($fh->close())
         {
@@ -83,10 +90,9 @@ sub disable_nm_manage_dns
         };
     } else {
         # cleanup the config if was created previously
-        if (-e $filename)
-        {
+        if (-e $filename) {
             my $msg = "REMOVE config $filename, NOTE: networkmanager will manage resolv.conf";
-            if(unlink($filename)) {
+            if (unlink($filename)) {
                 $self->info($msg);
                 $self->verbose("Reload NetworkManager");
                 $nwsrv->reload();
@@ -104,7 +110,7 @@ sub make_nm_ip_rule
     my ($self, $device, $rules, $routing_table_hash) = @_;
 
     my @text;
-    my $idx=0;
+    my $idx = 0;
     foreach my $rule (@$rules) {
         my $priority = 100;
         $priority = $rule->{priority} if $rule->{priority};
@@ -167,7 +173,6 @@ sub get_bonded_eth
 sub nmstate_file_dump
 {
     my ($self, $filename, $ifaceconfig) = @_;
-    my $net_module = 'nmstate/interface';
     my $changes = 0;
 
     my $func = "nmstate_file_dump";
@@ -178,23 +183,23 @@ sub nmstate_file_dump
 
     if (!$self->file_exists($filename) || $self->mk_bu($filename, $testcfg)) {
 
-    my $trd = EDG::WP4::CCM::TextRender->new($net_module, $ifaceconfig, relpath => 'network');
+    my $trd = EDG::WP4::CCM::TextRender->new('yaml', $ifaceconfig, relpath => 'network');
     if (! defined($trd->get_text())) {
         $self->error ("Unable to generate network config $filename: $trd->{fail}");
         return;
     };
     my $fh = $trd->filewriter($testcfg,
-                            header => "# File generated by " . __PACKAGE__ . ". Do not edit",
-                            log => $self);
+                              header => "# File generated by " . __PACKAGE__ . ". Do not edit",
+                              log => $self);
     my $filestatus;
     if ($fh->close()) {
-            if ($self->file_exists($filename)) {
-                $self->info("$func: file $filename has newer version scheduled.");
-                $filestatus = $UPDATED;
-            } else {
-                $self->info("$func: new file $filename scheduled.");
-                $filestatus = $NEW;
-            }
+        if ($self->file_exists($filename)) {
+            $self->info("$func: file $filename has newer version scheduled.");
+            $filestatus = $UPDATED;
+        } else {
+            $self->info("$func: new file $filename scheduled.");
+            $filestatus = $NEW;
+        }
     } else {
         my $is_active = is_active_interface($self, $ifaceconfig->{name});
         if (( $is_active != 1 ) && ($ifaceconfig->{enabled}) eq "true"){
@@ -402,7 +407,6 @@ sub nmstate_apply
     my $nwstate = $exifiles->{$NETWORKCFG};
 
     my $action;
-    my $nmstateclt_cmd = $self->nmstatectl();
     $self->verbose("Apply config using nmstatectl for each interface");
     if (($nwstate == $UPDATED) || ($nwstate == $NEW)) {
         # Do not need to start networking in nmstate.
@@ -410,15 +414,15 @@ sub nmstate_apply
         $action = 1;
     }
     if (@ifaces) {
-        $self->info("Applying changes using $nmstateclt_cmd ",join(', ', @ifaces));
+        $self->info("Applying changes using $NMSTATECTL ",join(', ', @ifaces));
         my @cmds;
+        # clear any connections created by NM with 'Wired connection x' to start fresh.
+        $self->clear_default_nm_connections();
         foreach my $iface (@ifaces) {
-            # clear any connections created by NM with 'Wired connection x' to start fresh.
-            $self->clear_default_nm_connections();
             # apply config using nmstatectl
-            my $ymlfile = "$NMCFG_DIR/$iface.yml";
+            my $ymlfile = $self->iface_filename($iface);
             if ($self->any_exists($ymlfile)){
-                push(@cmds, [$nmstateclt_cmd, "apply", $ymlfile]);
+                push(@cmds, [$NMSTATECTL, "apply", $ymlfile]);
                 push(@cmds, [qw(sleep 10)]) if ($iface =~ m/bond/);
             } else {
                 # do we down the interface?
@@ -450,7 +454,6 @@ sub Configure
 {
     my ($self, $config) = @_;
 
-    $self->set_cfg_dir($NMCFG_DIR);
     return if ! defined($self->init_backupdir());
 
     # current setup, will be printed in case of major failure
@@ -499,7 +502,7 @@ sub Configure
     foreach my $ifacename (sort keys %$ifaces) {
         my $iface = $ifaces->{$ifacename};
         my $nmstate_cfg = generate_nmstate_config($self, $ifacename, $net, $ipv6, $nwtree->{routing_table});
-        my $file_name = "$NMCFG_DIR/$ifacename.yml";
+        my $file_name = $self->iface_filename($ifacename);
         $exifiles->{$file_name} = $self->nmstate_file_dump($file_name, $nmstate_cfg);
 
         # TODO: not sure about what is going on here, keeping it out for now
diff --git a/ncm-network/src/main/resources/nmstate/interface.tt b/ncm-network/src/main/resources/nmstate/interface.tt
deleted file mode 100644
index 087441c21b..0000000000
--- a/ncm-network/src/main/resources/nmstate/interface.tt
+++ /dev/null
@@ -1,80 +0,0 @@
----
-[%- IF routes.config %]
-routes:
-  config:
-    [%-  FOREACH rt IN routes.config %]
-    - destination: [% rt.destination %]
-      next-hop-address: [% rt.next_hop_address %]
-      next-hop-interface: [% rt.next_hop_interface %]
-      [%- IF rt.table_id %]
-      table-id: [% rt.table_id %]
-      [%- END %]
-    [%- END %]
-[%- END %]
-interfaces:
-- name: [% name %]
-  type: [% type %]
-  profile-name: [% name %]
-  state: [% state %]
-  [%- IF hwaddr %]
-  mac-address: [%hwaddr %]
-  [%- END %]
-  [%- IF mtu %]
-  mtu: [% mtu %]
-  [%- END %]
-  [%- IF ipv4.address %]
-  ipv4:
-    address:
-    [%- FOREACH addr IN ipv4.address %]
-    - ip: [%- addr.ip %]
-      prefix-length: [%- addr.prefix %]
-    [%- END %]      
-    enabled: [% enabled %]
-  [%- ELSE %]
-  ipv4:
-    enabled: [% enabled %]
-  [%- END %]
-  [%- IF ipv6.address %]
-  ipv6:
-    address:
-    [%- FOREACH addr IN ipv6.address %]
-    - ip: [%- addr.ip %]
-      prefix-length: [%- addr.prefix %]
-    [%- END %]      
-    enabled: [% enabled %]
-  [%- ELSE %]
-  ipv6:
-    enabled: false
-  [%- END %]
-  [%- IF type == 'bond' %]
-  [%- IF link_aggregation %]
-  link-aggregation:
-    mode: [% link_aggregation.mode %]
-    [%- IF link_aggregation.options %]
-    options:
-      [%- FOREACH opt IN link_aggregation.options %]
-      [% opt.key %]: [% opt.value %]
-      [%- END %]
-    [%- END %]
-    port:
-    [%- FOREACH port IN link_aggregation.port %]
-    - [%- port %]
-    [%- END %]
-    [%- END %]
-    [%- END %]
-  [%- IF vlan %]
-  vlan:
-    base-iface: [% vlan.base_iface %]
-    id: [% vlan.vlan_id %]
-  [%- END %]
-[%- IF route %]
-route-rules:
-  config:
-    [%-  FOREACH rl IN rule %]
-    [%- IF rl.to %] 
-    - ip-to: [% rl.to %]
-      priority: [% rl.priority %]
-      route-table: [% rl.table_id %]
-    [%- END %]
-    [%- END -%]
-[%- END -%]
diff --git a/ncm-network/src/test/perl/nmstate_simple.t b/ncm-network/src/test/perl/nmstate_simple.t
index b8891252b7..606d4a681f 100644
--- a/ncm-network/src/test/perl/nmstate_simple.t
+++ b/ncm-network/src/test/perl/nmstate_simple.t
@@ -15,7 +15,6 @@ my $mock = Test::MockModule->new('NCM::Component::nmstate');
 my $cfg = get_config_for_profile('nmstate_simple');
 my $cmp = NCM::Component::nmstate->new('network');
 
-
 is($cmp->Configure($cfg), 1, "Component runs correctly with a test profile");
 
 # From here, test custom methods
diff --git a/ncm-network/src/test/resources/nmstate-simple.pan b/ncm-network/src/test/resources/nmstate-simple.pan
deleted file mode 100644
index 1630ac336e..0000000000
--- a/ncm-network/src/test/resources/nmstate-simple.pan
+++ /dev/null
@@ -1,6 +0,0 @@
-object template nmstate_simple;
-
-include 'simple_base_profile';
-# the next include is mainly to the profile, it is not used in the tests
-#   (unless the component gets specific schema things)
-include 'components/network/config-nmstate';

From 243cb1f1b51081972664d17b222c820b5b489490 Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Tue, 11 Jul 2023 19:33:22 +0200
Subject: [PATCH 5/7] ncm-network: exported readonly must be non-local
 variables (ie use our)

---
 ncm-network/src/main/perl/network.pm | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/ncm-network/src/main/perl/network.pm b/ncm-network/src/main/perl/network.pm
index dc3040cdfc..51706023a4 100755
--- a/ncm-network/src/main/perl/network.pm
+++ b/ncm-network/src/main/perl/network.pm
@@ -145,10 +145,10 @@ Readonly my $BRIDGECMD => '/usr/sbin/brctl';
 Readonly my $IPADDR => [qw(ip addr show)];
 Readonly my $IPROUTE => [qw(ip route show)];
 Readonly my $OVS_VCMD => '/usr/bin/ovs-vsctl';
-Readonly my $HOSTNAME_CMD => '/usr/bin/hostnamectl';
+Readonly our $HOSTNAME_CMD => '/usr/bin/hostnamectl';
 Readonly my $ROUTING_TABLE => '/etc/iproute2/rt_tables';
 
-Readonly my $NETWORK_PATH => '/system/network';
+Readonly our $NETWORK_PATH => '/system/network';
 Readonly my $HARDWARE_PATH => '/hardware/cards/nic';
 
 # Regexp for the supported ifcfg-<device> devices.
@@ -178,22 +178,22 @@ Readonly my $DEVICE_REGEXP => qr{
     $
 }x;
 
-Readonly my $NETWORKCFG => "/etc/sysconfig/network";
+Readonly our $NETWORKCFG => "/etc/sysconfig/network";
 
-Readonly my $RESOLV_CONF => '/etc/resolv.conf';
-Readonly my $RESOLV_CONF_SAVE => '/etc/resolv.conf.save';
-Readonly my $RESOLV_SUFFIX => '.ncm-network';
+Readonly our $RESOLV_CONF => '/etc/resolv.conf';
+Readonly our $RESOLV_CONF_SAVE => '/etc/resolv.conf.save';
+Readonly our $RESOLV_SUFFIX => '.ncm-network';
 
-Readonly my $FAILED_SUFFIX => '-failed';
+Readonly our $FAILED_SUFFIX => '-failed';
 
-Readonly my $BACKUP_DIR_SUFFIX => '.quattorbackup';
+Readonly our $BACKUP_DIR_SUFFIX => '.quattorbackup';
 
-Readonly my $REMOVE => -1;
-Readonly my $NOCHANGES => 0;
-Readonly my $UPDATED => 1;
-Readonly my $NEW => 2;
+Readonly our $REMOVE => -1;
+Readonly our $NOCHANGES => 0;
+Readonly our $UPDATED => 1;
+Readonly our $NEW => 2;
 # changes to file, but same config (eg for new file formats)
-Readonly my $KEEPS_STATE => 3;
+Readonly our $KEEPS_STATE => 3;
 
 # automatic exports of readonlys
 our @EXPORT = qw($FAILED_SUFFIX

From 38be4f6706578494029ca0c8114ce76e013d1983 Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Tue, 11 Jul 2023 19:35:33 +0200
Subject: [PATCH 6/7] ncm-network: make some progress with nmstate tests

---
 .../pan/components/network/core-schema.pan    |  1 +
 ncm-network/src/main/perl/nmstate.pm          | 37 ++++++++++---------
 ncm-network/src/test/perl/nmstate_simple.t    | 11 ++++++
 3 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/ncm-network/src/main/pan/components/network/core-schema.pan b/ncm-network/src/main/pan/components/network/core-schema.pan
index 5c92fb59a9..0d3a84c086 100644
--- a/ncm-network/src/main/pan/components/network/core-schema.pan
+++ b/ncm-network/src/main/pan/components/network/core-schema.pan
@@ -418,6 +418,7 @@ type structure_network = {
     "set_hwaddr" ? boolean
     "nmcontrolled" ? boolean
     "allow_nm" ? boolean
+    @{let Networkmanager manage the dns (nmstate). Default is false}
     "nm_manage_dns" ? boolean
     "primary_ip" ? string
     "routers" ? structure_router{}
diff --git a/ncm-network/src/main/perl/nmstate.pm b/ncm-network/src/main/perl/nmstate.pm
index e00d08960e..50b608503d 100644
--- a/ncm-network/src/main/perl/nmstate.pm
+++ b/ncm-network/src/main/perl/nmstate.pm
@@ -79,20 +79,11 @@ sub disable_nm_manage_dns
     my $filename = "/etc/NetworkManager/conf.d/90-quattor-dns-none.conf";
     my @data = ('[main]');
     push @data, 'dns=none';
-    if ( (defined($manage_dns) && !$manage_dns) || (! defined($manage_dns)) ) {
-        $self->verbose("Configuring networkmanager not to manage resolv.conf");
-        my $fh = CAF::FileWriter->new($filename, mode => oct(444), log => $self, keeps_state => 1);
-        print $fh join("\n", @data, '');
-        if ($fh->close())
-        {
-            $self->info("File $filename changed, reload network");
-            $nwsrv->reload();
-        };
-    } else {
+    if ( $manage_dns ) {
         # cleanup the config if was created previously
-        if (-e $filename) {
+        if ($self->file_exists($filename)) {
             my $msg = "REMOVE config $filename, NOTE: networkmanager will manage resolv.conf";
-            if (unlink($filename)) {
+            if ($self->cleanup($filename)) {
                 $self->info($msg);
                 $self->verbose("Reload NetworkManager");
                 $nwsrv->reload();
@@ -100,8 +91,15 @@ sub disable_nm_manage_dns
                 $self->error("$msg failed. ($self->{fail})");
             };
         };
+    } else {
+        $self->verbose("Configuring networkmanager not to manage resolv.conf");
+        my $fh = CAF::FileWriter->new($filename, mode => oct(444), log => $self, keeps_state => 1);
+        print $fh join("\n", @data, '');
+        if ($fh->close()) {
+            $self->info("File $filename changed, reload network");
+            $nwsrv->reload();
+        };
     }
-
 }
 
 # return hasref of policy rule. interface.tt module uses this to create the rule in nmstate file.
@@ -370,7 +368,7 @@ sub is_active_interface
     my @existing_conn = split('\n', $output);
     my %current_conn;
     my $found = 0;
-    foreach  my $conn_name  (@existing_conn) {
+    foreach my $conn_name (@existing_conn) {
         $conn_name =~ s/\s+$//;
         if ($conn_name eq $ifacename){
             $found = 1;
@@ -414,7 +412,7 @@ sub nmstate_apply
         $action = 1;
     }
     if (@ifaces) {
-        $self->info("Applying changes using $NMSTATECTL ",join(', ', @ifaces));
+        $self->info("Applying changes using $NMSTATECTL ", join(', ', @ifaces));
         my @cmds;
         # clear any connections created by NM with 'Wired connection x' to start fresh.
         $self->clear_default_nm_connections();
@@ -480,10 +478,12 @@ sub Configure
     $self->routing_table($nwtree->{routing_table});
 
     # main network config
+    # TODO: aka7, what is the role of /etc/systconfig/network in networkmanager/nmstate managed OS?
     return if ! defined($self->mk_bu($NETWORKCFG));
 
     my $hostname = $nwtree->{realhostname} || "$nwtree->{hostname}.$nwtree->{domainname}";
 
+    # TODO: aka7, targeted OS is EL9, you can assume hostnamectl exists
     my $use_hostnamectl = $self->_is_executable($HOSTNAME_CMD);
     # if hostnamectl exists, do not set it via the network config file
     # systemd rpm --script can remove it anyway
@@ -492,6 +492,7 @@ sub Configure
     my ($text, $ipv6) = $self->make_network_cfg($nwtree, $net, $nwcfg_hostname);
     $exifiles->{$NETWORKCFG} = $self->file_dump($NETWORKCFG, $text);
 
+    # TODO: aka7 this can be removed as well. this is some piece of legacy code you don't need
     if ($exifiles->{$NETWORKCFG} == $UPDATED && $use_hostnamectl) {
         # Network config was updated, check if it was due to removal of HOSTNAME
         # when hostnamectl is present.
@@ -509,6 +510,7 @@ sub Configure
         #$self->default_broadcast_keeps_state($file_name, $ifacename, $iface, $exifiles, 0);
         $self->ethtool_opts_keeps_state($file_name, $ifacename, $iface, $exifiles);
 
+        # TODO: aka7, this is legacy code, it can go away
         if ($exifiles->{$file_name} == $UPDATED) {
             # interface configuration was changed
             # check if this was due to addition of resolv_mods / peerdns
@@ -558,8 +560,8 @@ sub Configure
     #   3. (re)start things
     my $nwsrv = CAF::Service->new(['NetworkManager'], log => $self);
 
-    # NetworkManager manages dns by default, but we manage dns with ncm-resolver, new option to eanble/disable it.
-    $self->disable_nm_manage_dns($nwtree->{nm_manage_dns}, $nwsrv);
+    # NetworkManager manages dns by default, but we manage dns with e.g. ncm-resolver, new option to enable/disable it.
+    $self->disable_nm_manage_dns($nwtree->{nm_manage_dns} || 0, $nwsrv);
 
     # nmstate files are applied uinsg nmstate apply via this componant. We don't want nmstate svc to manage it.
     # If nmstate svc manages the files, it will apply the config for any files found in /etc/nmstate with .yml extension. Once the config is applied,
@@ -580,6 +582,7 @@ sub Configure
     my $config_changed = $self->deploy_config($exifiles);
 
     # Save/Restore last known working (i.e. initial) /etc/resolv.conf
+    # TODO: @aka7, hmmm, if nm is allowed to manage dns, then this should be allowed to have changed
     $resolv_conf_fh->close();
 
     # Since there's per interface reload, interface changes will be applied via nmstatectl.
diff --git a/ncm-network/src/test/perl/nmstate_simple.t b/ncm-network/src/test/perl/nmstate_simple.t
index 606d4a681f..d7cfef512f 100644
--- a/ncm-network/src/test/perl/nmstate_simple.t
+++ b/ncm-network/src/test/perl/nmstate_simple.t
@@ -11,10 +11,21 @@ use Test::MockModule;
 
 use NCM::Component::nmstate;
 my $mock = Test::MockModule->new('NCM::Component::nmstate');
+my %executables;
+$mock->mock('_is_executable', sub {diag "executables $_[1] ",explain \%executables;return $executables{$_[1]};});
 
 my $cfg = get_config_for_profile('nmstate_simple');
 my $cmp = NCM::Component::nmstate->new('network');
 
+# TODO: there should e no reason for this. we can assume it's there in EL9
+$executables{'/usr/bin/hostnamectl'} = 1;
+
+# TODO: why is this still used? can we get rid of doing anything with it?
+set_file_contents("/etc/sysconfig/network", 'x' x 100);
+
+set_file_contents("/etc/resolv.conf", 'managed by something else');
+
+
 is($cmp->Configure($cfg), 1, "Component runs correctly with a test profile");
 
 # From here, test custom methods

From fa49d888f059f6988d2acfbbbbe3139b462c97e0 Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Thu, 13 Jul 2023 13:18:21 +0200
Subject: [PATCH 7/7] ncm-network: nmstate: further testing

---
 ncm-network/src/test/perl/nmstate_simple.t    | 56 +++++++++++++++++--
 .../src/test/resources/nmstate_simple.pan     |  6 ++
 2 files changed, 56 insertions(+), 6 deletions(-)
 create mode 100644 ncm-network/src/test/resources/nmstate_simple.pan

diff --git a/ncm-network/src/test/perl/nmstate_simple.t b/ncm-network/src/test/perl/nmstate_simple.t
index d7cfef512f..00575abcef 100644
--- a/ncm-network/src/test/perl/nmstate_simple.t
+++ b/ncm-network/src/test/perl/nmstate_simple.t
@@ -8,6 +8,7 @@ BEGIN {
 use Test::More;
 use Test::Quattor qw(nmstate_simple);
 use Test::MockModule;
+use Readonly;
 
 use NCM::Component::nmstate;
 my $mock = Test::MockModule->new('NCM::Component::nmstate');
@@ -17,21 +18,64 @@ $mock->mock('_is_executable', sub {diag "executables $_[1] ",explain \%executabl
 my $cfg = get_config_for_profile('nmstate_simple');
 my $cmp = NCM::Component::nmstate->new('network');
 
+Readonly my $NETWORK => <<EOF;
+NETWORKING=yes
+GATEWAY=4.3.2.254
+EOF
+
+Readonly my $RESOLV => <<EOF;
+managed by something else
+EOF
+
+Readonly my $NODNS => <<EOF;
+[main]
+dns=none
+EOF
+
+Readonly my $ETH0_YML => <<EOF;
+# File generated by NCM::Component::nmstate. Do not edit
+---
+device: eth0
+link_aggregation:
+  port: []
+name: eth0
+type: bond
+EOF
+
 # TODO: there should e no reason for this. we can assume it's there in EL9
 $executables{'/usr/bin/hostnamectl'} = 1;
 
 # TODO: why is this still used? can we get rid of doing anything with it?
 set_file_contents("/etc/sysconfig/network", 'x' x 100);
 
-set_file_contents("/etc/resolv.conf", 'managed by something else');
+set_file_contents("/etc/resolv.conf", "$RESOLV");
 
+set_file_contents("/etc/nmstate/toremove.yml", "something");
 
-is($cmp->Configure($cfg), 1, "Component runs correctly with a test profile");
 
-# From here, test custom methods
 command_history_reset();
-$cmp->disable_nmstate(1);
-# check there a executed commands that match NetworkManager
-ok(command_history_ok(undef, ['nmstate']));
+is($cmp->Configure($cfg), 1, "Component runs correctly with a test profile");
+
+my $fh;
+
+$fh = get_file($cmp->testcfg_filename("/etc/sysconfig/network"));
+ok(! defined($fh), "testcfg network file was cleaned up");
+
+# on success, this is hardlink of a cleaned up testcfg; can't use get_file
+is(get_file_contents("/etc/sysconfig/network"), $NETWORK, "Exact network config");
+
+# resolv.conf is unchanged
+is(get_file_contents("/etc/resolv.conf"), $RESOLV, "Exact network config");
+
+# set nm config to disable dns mgmt
+is(get_file_contents("/etc/NetworkManager/conf.d/90-quattor-dns-none.conf"), $NODNS, "disable NM dns mgmt");
+
+# unconfigure nmstate yml is removed
+ok(!$cmp->file_exists("/etc/nmstate/toremove.yml"), "unconfigured yml nmstate is removed");
+my $eth0yml = get_file_contents("/etc/nmstate/eth0.yml");
+is($eth0yml, $ETH0_YML, "Exact eth0 yml config");
+
+
+ok(command_history_ok([], []));
 
 done_testing();
diff --git a/ncm-network/src/test/resources/nmstate_simple.pan b/ncm-network/src/test/resources/nmstate_simple.pan
new file mode 100644
index 0000000000..1630ac336e
--- /dev/null
+++ b/ncm-network/src/test/resources/nmstate_simple.pan
@@ -0,0 +1,6 @@
+object template nmstate_simple;
+
+include 'simple_base_profile';
+# the next include is mainly to the profile, it is not used in the tests
+#   (unless the component gets specific schema things)
+include 'components/network/config-nmstate';