From 3c146019e717c5b57c523f4a7a933d31abb1e0be Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Wed, 10 Jun 2020 21:04:08 +0200
Subject: [PATCH 1/7] ncm-filecopy: fix conditional reporter method call

---
 ncm-filecopy/src/main/perl/filecopy.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ncm-filecopy/src/main/perl/filecopy.pm b/ncm-filecopy/src/main/perl/filecopy.pm
index a0a2a361dc..a84c85324a 100755
--- a/ncm-filecopy/src/main/perl/filecopy.pm
+++ b/ncm-filecopy/src/main/perl/filecopy.pm
@@ -150,7 +150,7 @@ sub Configure
         $cmd->execute();
         if ( $? ) {
             my $method = $confighash->{ignore_restart_failure} ? 'warn' : 'error';
-            $self->{$method}("Command failed. Command output: $cmd_output\n");
+            $self->$method("Command failed. Command output: $cmd_output\n");
         } else {
             $self->debug(1,"Command output: $cmd_output\n");
         }

From 66b66006ab67d6bbc778bbaf4c0140ac509dc8b6 Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Wed, 10 Jun 2020 21:39:15 +0200
Subject: [PATCH 2/7] ncm-metaconfig: support running arbitrary commands in
 various steps

---
 .../main/pan/components/metaconfig/schema.pan |  15 +++
 ncm-metaconfig/src/main/perl/metaconfig.pm    | 117 +++++++++++++++++-
 ncm-metaconfig/src/test/perl/commands.t       |  72 +++++++++++
 ncm-metaconfig/src/test/perl/configure.t      |   6 +-
 .../src/test/resources/commands.pan           |   9 ++
 5 files changed, 212 insertions(+), 7 deletions(-)
 create mode 100644 ncm-metaconfig/src/test/perl/commands.t
 create mode 100644 ncm-metaconfig/src/test/resources/commands.pan

diff --git a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
index c28112a4f9..9566a87493 100644
--- a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
+++ b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
@@ -86,6 +86,18 @@ type ${project.artifactId}_textrender_convert = {
 
 type caf_service_action = string with match(SELF, '^(restart|reload|stop_sleep_start)$');
 
+type ${project.artifactId}_commands = {
+    @{Always run, happens before possible modifications}
+    'pre' ? string
+    @{Always run, happens before possible modifications, the file content is passed om stdin.
+      Runs with 'keeps_state' enabled, so do not modify anything with this command.}
+    'test' ? string
+    @{Only run after file is modified, but after daemon action is added}
+    'changed' ? string
+    @{Always run, happens after possible modifications (and after 'changed').}
+    'post' ? string
+};
+
 type ${project.artifactId}_config =  {
     @{File permissions. Defaults to 0644.}
     'mode' : long = 0644
@@ -118,6 +130,9 @@ type ${project.artifactId}_config =  {
     'contents' : ${project.artifactId}_extension
     @{Predefined conversions from EDG::WP4::CCM::TextRender}
     'convert' ? ${project.artifactId}_textrender_convert
+    @{Commands to run on pre, validation and/or post step.
+      These are independent of daemons and are executed at time of processing each service.}
+    'commands' ? ${project.artifactId}_commands
 } = dict();
 
 type ${project.artifactId}_component = {
diff --git a/ncm-metaconfig/src/main/perl/metaconfig.pm b/ncm-metaconfig/src/main/perl/metaconfig.pm
index c19f711df2..9bac482306 100644
--- a/ncm-metaconfig/src/main/perl/metaconfig.pm
+++ b/ncm-metaconfig/src/main/perl/metaconfig.pm
@@ -160,16 +160,122 @@ our $EC = LC::Exception::Context->new->will_store_all;
 
 our $NoActionSupported = 1;
 
+# Given metaconfigservice C<$srv> for C<$file> and hash-reference C<$actions>,
+# prepare the actions to be taken for this service/file.
+# C<actions> is updated in-place; does not return anything.
+sub prepare_action
+{
+    my ($self, $srv, $file, $actions) = @_;
+
+    # Not using a hash here to detect and support
+    # any overlap with legacy daemon-restart config
+    my @daemon_action;
+
+    my $file_msg = "for file $file";
+
+    foreach my $daemon (sort keys %{$srv->{daemons} || {}}) {
+        push(@daemon_action, $daemon, $srv->{daemons}->{$daemon});
+    }
+
+    if ($srv->{daemon}) {
+        $self->verbose("Deprecated daemon(s) restart via daemon field $file_msg.");
+        foreach my $daemon (@{$srv->{daemon}}) {
+            if ($srv->{daemons}->{$daemon}) {
+                $self->verbose("Daemon $daemon also defined in daemons field $file_msg. Adding restart action anyway.");
+            }
+            push(@daemon_action, $daemon, 'restart');
+        }
+    }
+
+    my @acts;
+    while (my ($daemon,$action) = splice(@daemon_action, 0, 2)) {
+        if (exists($ALLOWED_ACTIONS{$action})) {
+            $actions->{$action} ||= {};
+            $actions->{$action}->{$daemon} = 1;
+            push(@acts, "$daemon:$action");
+        } else {
+            $self->error("Not a CAF::Service allowed action ",
+                         "$action for daemon $daemon $file_msg ",
+                         "in profile (component/schema mismatch?).");
+        }
+    }
+
+    if (@acts) {
+        $self->verbose("Scheduled daemon/action ".join(', ',@acts)." $file_msg.");
+    } else {
+        $self->verbose("No daemon/action scheduled $file_msg.");
+    }
+}
+
+# Take the action for all daemons as defined in hash-reference C<$actions>.
+# Does not return anything.
+sub process_actions
+{
+    my ($self, $actions) = @_;
+    foreach my $action (sort keys %$actions) {
+        my @daemons = sort keys %{$actions->{$action}};
+        $self->info("Executing action $action on services: ", join(',', @daemons));
+        my $srv = CAF::Service->new(\@daemons, log => $self);
+        # CAF::Service does all the logging we need
+        $srv->$action();
+    }
+}
+
+# Run $service shell command of $type (ie a string) (if defined).
+#   $msg is a reporting prefix
+#   When $input is not undef, pass it on stdin
+# Return 1 on success, undef otherwise.
+sub run_shell_command
+{
+    my ($self, $commands, $type, $input) = @_;
+
+    my $command = $commands->{$type};
+    if ($command) {
+        $self->debug(1, "Going to run $type command '$command'");
+
+        my ($err, $out);
+        my %opts = (
+            shell => 1,
+            log => $self,
+            stdout => \$out,
+            stderr => \$err,
+            );
+        if (defined($input)) {
+            $opts{stdin} = "$input";
+        };
+        if ($type eq 'test') {
+            $opts{keeps_state} = 1;
+        };
+
+        CAF::Process->new([$command], %opts)->execute();
+
+        my $ec = $?;
+
+        my $report = $ec ? 'error' : 'verbose';
+        $self->$report("run $type command '$command' ",
+                         ($ec ? 'failed' : 'ok'),
+                         ,": stdout '$out'\n stderr '$err'",
+                         ($input ? "\n stdin '$input'" : ""));
+        return $ec ? undef : 1;
+    } else {
+        $self->debug(5, "No $type command to run");
+        return 1;
+    };
+}
+
 # Generate C<$file>, configuring C<$srv> using CAF::TextRender with
 # contents C<$contents> (if C<$contents>  is not defined,
 # C<$srv->{contents}> is used).
 # Also tracks the actions that need to be taken via the
 # C<$sa> C<CAF::ServiceActions> instance.
-# Returns undef in case of rendering failure, 1 otherwise.
+# Returns undef in case of rendering or other failure, 1 otherwise.
 sub handle_service
 {
     my ($self, $file, $srv, $contents, $sa) = @_;
 
+    my $commands = $srv->{commands} || {};
+    return if ! $self->run_shell_command($commands, 'pre');
+
     $contents = $srv->{contents} if (! defined($contents));
 
     my $trd = EDG::WP4::CCM::TextRender->new($srv->{module},
@@ -202,16 +308,23 @@ sub handle_service
         return;
     }
 
+    if (! $self->run_shell_command($commands, 'test', "$fh")) {
+        $fh->cancel();
+        return;
+    };
+
     if ($fh->close()) {
         $self->info("File $file updated");
         $sa->add($srv->{daemons}, msg => "for file $file");
+        return if ! $self->run_shell_command($commands, 'changed');
     } else {
         $self->verbose("File $file up-to-date");
     };
 
-    return 1;
+    return $self->run_shell_command($commands, 'post');
 }
 
+
 sub _configure_files
 {
     my ($self, $config, $root) = @_;
diff --git a/ncm-metaconfig/src/test/perl/commands.t b/ncm-metaconfig/src/test/perl/commands.t
new file mode 100644
index 0000000000..bcd36455b1
--- /dev/null
+++ b/ncm-metaconfig/src/test/perl/commands.t
@@ -0,0 +1,72 @@
+use strict;
+use warnings;
+use Test::More;
+use Test::Quattor qw(commands);
+use NCM::Component::metaconfig;
+use Test::MockModule;
+
+=pod
+
+=head1 DESCRIPTION
+
+Test the configure() method.
+
+=cut
+
+my $orig = 'X';
+
+sub clean
+{
+    set_file_contents("/foo/bar", "$orig");
+    command_history_reset();
+}
+
+my $cmp = NCM::Component::metaconfig->new('metaconfig');
+my $cfg = get_config_for_profile('commands');
+
+clean();
+
+is($cmp->Configure($cfg), 1, "Configure succeeds");
+my $fh = get_file("/foo/bar");
+ok($orig ne "$fh", "orig content is not same as current $fh");
+
+ok($fh, "A file was actually created");
+isa_ok($fh, "CAF::FileWriter");
+
+# if default sysv init service changes, also modify the aii_command negative test
+ok(command_history_ok(['cmd pre', 'cmd test', 'cmd changed', 'cmd post', 'service foo restart']),
+   "commands run and serivce foo restarted");
+
+clean();
+
+# changed failed, post does not run, daemons does run
+set_command_status('cmd changed', 1);
+$cmp->Configure($cfg);
+ok(command_history_ok(['cmd pre', 'cmd test', 'cmd changed', 'service foo restart'], ['cmd post']),
+   "commands except post run and serivce foo restarted");
+my $fcnt = get_file_contents("/foo/bar");
+ok($orig ne "$fcnt", "orig content is not same as current $fcnt");
+
+clean();
+
+# test failed, changed, post and daemons do not run, content unmodified
+set_command_status('cmd test', 1);
+$cmp->Configure($cfg);
+ok(command_history_ok(['cmd pre', 'cmd test'], ['cmd changed', 'service foo restart', 'cmd post']),
+   "commands run except changed, post and no serivce foo restarted");
+$fcnt = get_file_contents("/foo/bar");
+# unmodified
+is($orig, "$fcnt", "orig content is same as current $fcnt on test failed");
+
+clean();
+# pre failed, test, changed, post and daemons do not run, content unmodified
+set_command_status('cmd pre', 1);
+$cmp->Configure($cfg);
+ok(command_history_ok(['cmd pre'], ['cmd test', 'cmd changed', 'service foo restart', 'cmd post']),
+   "commands run except test, changed, post and no serivce foo restarted");
+$fcnt = get_file_contents("/foo/bar");
+# unmodified
+is($orig, "$fcnt", "orig content is same as current $fcnt on pre failed");
+
+
+done_testing();
diff --git a/ncm-metaconfig/src/test/perl/configure.t b/ncm-metaconfig/src/test/perl/configure.t
index 6b3743ee1e..77877a2fb7 100644
--- a/ncm-metaconfig/src/test/perl/configure.t
+++ b/ncm-metaconfig/src/test/perl/configure.t
@@ -1,12 +1,8 @@
 use strict;
 use warnings;
 use Test::More;
-use Test::Quattor qw(configure);
+use Test::Quattor qw(configure commands);
 use NCM::Component::metaconfig;
-use Test::MockModule;
-use CAF::Object;
-
-use JSON::XS;
 
 =pod
 
diff --git a/ncm-metaconfig/src/test/resources/commands.pan b/ncm-metaconfig/src/test/resources/commands.pan
new file mode 100644
index 0000000000..b6c8f8c4c6
--- /dev/null
+++ b/ncm-metaconfig/src/test/resources/commands.pan
@@ -0,0 +1,9 @@
+object template commands;
+
+include 'simple';
+
+prefix "/software/components/metaconfig/services/{/foo/bar}/commands";
+"pre" = "cmd pre";
+"test" = "cmd test";
+"changed" = "cmd changed";
+"post" = "cmd post";

From 40cd477725142da72815b4fb53dd0ff51b8ef888 Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Thu, 11 Jun 2020 10:14:30 +0200
Subject: [PATCH 3/7] ncm-metaconfig: setup command registry for running
 arbitrary commands

---
 .../main/pan/components/metaconfig/schema.pan |  20 ++-
 ncm-metaconfig/src/main/perl/metaconfig.pm    | 115 +++++++-----------
 ncm-metaconfig/src/test/perl/actions.t        |   1 -
 ncm-metaconfig/src/test/perl/aii_command.t    |   2 +-
 ncm-metaconfig/src/test/resources/aii.pan     |   2 +-
 .../src/test/resources/commands.pan           |   8 +-
 .../src/test/resources/simple_commands.pan    |  15 +++
 7 files changed, 77 insertions(+), 86 deletions(-)
 create mode 100644 ncm-metaconfig/src/test/resources/simple_commands.pan

diff --git a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
index 9566a87493..d82f0e0ff0 100644
--- a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
+++ b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
@@ -86,7 +86,7 @@ type ${project.artifactId}_textrender_convert = {
 
 type caf_service_action = string with match(SELF, '^(restart|reload|stop_sleep_start)$');
 
-type ${project.artifactId}_commands = {
+type ${project.artifactId}_actions = {
     @{Always run, happens before possible modifications}
     'pre' ? string
     @{Always run, happens before possible modifications, the file content is passed om stdin.
@@ -130,12 +130,26 @@ type ${project.artifactId}_config =  {
     'contents' : ${project.artifactId}_extension
     @{Predefined conversions from EDG::WP4::CCM::TextRender}
     'convert' ? ${project.artifactId}_textrender_convert
-    @{Commands to run on pre, validation and/or post step.
+    @{Actions (i.e. named registered commands) to run on pre, validation and/or post step.
       These are independent of daemons and are executed at time of processing each service.}
-    'commands' ? ${project.artifactId}_commands
+    'actions' ? ${project.artifactId}_actions
 } = dict();
 
 type ${project.artifactId}_component = {
     include structure_component
     'services' : ${project.artifactId}_config{} with valid_absolute_file_paths(SELF)
+    @{Command registry for allowed actions, keys should be used as action value}
+    'commands' ? string{}
+} with {
+    foreach (esc_fn; srv; SELF['services']) {
+        if (exists(srv['actions'])) {
+            foreach (action; cmd_ref; srv['actions']) {
+                if (!(exists(SELF['commands']) && exists(SELF['commands'][cmd_ref]))) {
+                    error('Found %s action %s for %s, but no matching command registered',
+                            action, cmd_ref, unescape(esc_fn));
+                };
+            };
+        };
+    };
+    true;
 };
diff --git a/ncm-metaconfig/src/main/perl/metaconfig.pm b/ncm-metaconfig/src/main/perl/metaconfig.pm
index 9bac482306..d4d633d777 100644
--- a/ncm-metaconfig/src/main/perl/metaconfig.pm
+++ b/ncm-metaconfig/src/main/perl/metaconfig.pm
@@ -160,68 +160,8 @@ our $EC = LC::Exception::Context->new->will_store_all;
 
 our $NoActionSupported = 1;
 
-# Given metaconfigservice C<$srv> for C<$file> and hash-reference C<$actions>,
-# prepare the actions to be taken for this service/file.
-# C<actions> is updated in-place; does not return anything.
-sub prepare_action
-{
-    my ($self, $srv, $file, $actions) = @_;
-
-    # Not using a hash here to detect and support
-    # any overlap with legacy daemon-restart config
-    my @daemon_action;
-
-    my $file_msg = "for file $file";
-
-    foreach my $daemon (sort keys %{$srv->{daemons} || {}}) {
-        push(@daemon_action, $daemon, $srv->{daemons}->{$daemon});
-    }
-
-    if ($srv->{daemon}) {
-        $self->verbose("Deprecated daemon(s) restart via daemon field $file_msg.");
-        foreach my $daemon (@{$srv->{daemon}}) {
-            if ($srv->{daemons}->{$daemon}) {
-                $self->verbose("Daemon $daemon also defined in daemons field $file_msg. Adding restart action anyway.");
-            }
-            push(@daemon_action, $daemon, 'restart');
-        }
-    }
-
-    my @acts;
-    while (my ($daemon,$action) = splice(@daemon_action, 0, 2)) {
-        if (exists($ALLOWED_ACTIONS{$action})) {
-            $actions->{$action} ||= {};
-            $actions->{$action}->{$daemon} = 1;
-            push(@acts, "$daemon:$action");
-        } else {
-            $self->error("Not a CAF::Service allowed action ",
-                         "$action for daemon $daemon $file_msg ",
-                         "in profile (component/schema mismatch?).");
-        }
-    }
-
-    if (@acts) {
-        $self->verbose("Scheduled daemon/action ".join(', ',@acts)." $file_msg.");
-    } else {
-        $self->verbose("No daemon/action scheduled $file_msg.");
-    }
-}
-
-# Take the action for all daemons as defined in hash-reference C<$actions>.
-# Does not return anything.
-sub process_actions
-{
-    my ($self, $actions) = @_;
-    foreach my $action (sort keys %$actions) {
-        my @daemons = sort keys %{$actions->{$action}};
-        $self->info("Executing action $action on services: ", join(',', @daemons));
-        my $srv = CAF::Service->new(\@daemons, log => $self);
-        # CAF::Service does all the logging we need
-        $srv->$action();
-    }
-}
-
 # Run $service shell command of $type (ie a string) (if defined).
+#   If $commands is undefined, nothing will be run or logged
 #   $msg is a reporting prefix
 #   When $input is not undef, pass it on stdin
 # Return 1 on success, undef otherwise.
@@ -229,6 +169,8 @@ sub run_shell_command
 {
     my ($self, $commands, $type, $input) = @_;
 
+    return 1 if ! defined($commands);
+
     my $command = $commands->{$type};
     if ($command) {
         $self->debug(1, "Going to run $type command '$command'");
@@ -268,22 +210,24 @@ sub run_shell_command
 # C<$srv->{contents}> is used).
 # Also tracks the actions that need to be taken via the
 # C<$sa> C<CAF::ServiceActions> instance.
+# C<$commands> is a hashref with pre/test/changed/post action commands.
+#   (If it is undefined, nothing will be run or logged, see run_shell_command)
 # Returns undef in case of rendering or other failure, 1 otherwise.
 sub handle_service
 {
-    my ($self, $file, $srv, $contents, $sa) = @_;
+    my ($self, $file, $srv, $contents, $sa, $commands) = @_;
 
-    my $commands = $srv->{commands} || {};
     return if ! $self->run_shell_command($commands, 'pre');
 
     $contents = $srv->{contents} if (! defined($contents));
 
-    my $trd = EDG::WP4::CCM::TextRender->new($srv->{module},
-                                             $contents,
-                                             log => $self,
-                                             eol => 0,
-                                             element => $srv->{convert},
-                                             );
+    my $trd = EDG::WP4::CCM::TextRender->new(
+        $srv->{module},
+        $contents,
+        log => $self,
+        eol => 0,
+        element => $srv->{convert},
+        );
 
     my %opts = (
         log => $self,
@@ -324,10 +268,33 @@ sub handle_service
     return $self->run_shell_command($commands, 'post');
 }
 
+# Lookup actions in command registry, and return hashref with actual commands for each action
+sub resolve_command_actions
+{
+    my ($self, $command_registry, $actions) = @_;
+
+    my $commands = {};  # this will trigger reporting that nothing is configured is this stays empty
+    foreach my $type (sort keys %$actions) {
+        my $action = $actions->{$type};
+        my $command = $command_registry->{$action};
+        if ($command) {
+            $commands->{$type} = $command;
+            $self->verbose("Resolved $type action $action to command '$command'");
+        } else {
+            # Not fatal, should be covered in schema already
+            $self->error("Unable to resovle $type action $action to command");
+        }
+    };
+    return $commands;
+}
+
 
 sub _configure_files
 {
-    my ($self, $config, $root) = @_;
+    my ($self, $config, %opts) = @_;
+
+    my $root = defined($opts{root}) ? $opts{root} : '';
+    my $run_commands = defined($opts{run_commands}) ? $opts{run_commands} : 1;
 
     my $t = $config->getElement($self->prefix)->getTree();
 
@@ -337,7 +304,9 @@ sub _configure_files
         my $srvc = $t->{services}->{$esc_filename};
         my $cont_el = $config->getElement($self->prefix()."/services/$esc_filename/contents");
         my $filename = ($root || '') . unescape($esc_filename);
-        $self->handle_service($filename, $srvc, $cont_el, $sa);
+        # Only when run_commands is false, use undef so nothing is even reported
+        my $commands = $run_commands ? $self->resolve_command_actions($t->{commands}, $srvc->{actions} || {}) : undef;
+        $self->handle_service($filename, $srvc, $cont_el, $sa, $commands);
     }
 
     return $sa;
@@ -356,14 +325,14 @@ sub Configure
 
 # Generate the files relative to metaconfig subdirectory
 # under the configuration cachemanager cache path.
-# No daemons will be restarted.
+# No daemons will be restarted, no commands run.
 sub aii_command
 {
     my ($self, $config) = @_;
 
     my $root = $config->{cache_path};
     if ($root) {
-        $self->_configure_files($config, "$root/metaconfig");
+        $self->_configure_files($config, root => "$root/metaconfig", run_commands => 0);
         return 1;
     } else {
         $self->error("No cache_path found for Configuration instance");
diff --git a/ncm-metaconfig/src/test/perl/actions.t b/ncm-metaconfig/src/test/perl/actions.t
index 5273ba2e02..2fdf8c93ed 100644
--- a/ncm-metaconfig/src/test/perl/actions.t
+++ b/ncm-metaconfig/src/test/perl/actions.t
@@ -32,7 +32,6 @@ Test how the need for restarting a service is handled
 my $actions = {};
 my $cmp = NCM::Component::metaconfig->new('metaconfig');
 
-
 =pod
 
 =head2 Test actions taken via Configure
diff --git a/ncm-metaconfig/src/test/perl/aii_command.t b/ncm-metaconfig/src/test/perl/aii_command.t
index 66b97df8b0..1f9ac3af78 100644
--- a/ncm-metaconfig/src/test/perl/aii_command.t
+++ b/ncm-metaconfig/src/test/perl/aii_command.t
@@ -33,6 +33,6 @@ isa_ok($fh, "CAF::FileWriter");
 $fh = get_file("/foo/bar");
 ok(!defined($fh), "Nothing created at regular file location");
 
-ok(command_history_ok(undef, ['service foo']), "serivce foo not restarted");
+ok(command_history_ok(undef, ['service foo', 'cmd']), "serivce foo not restarted, no cmd run");
 
 done_testing();
diff --git a/ncm-metaconfig/src/test/resources/aii.pan b/ncm-metaconfig/src/test/resources/aii.pan
index b72c1cbec8..9a5dfccef1 100644
--- a/ncm-metaconfig/src/test/resources/aii.pan
+++ b/ncm-metaconfig/src/test/resources/aii.pan
@@ -1,3 +1,3 @@
 object template aii;
 
-include 'simple';
+include 'simple_commands';
diff --git a/ncm-metaconfig/src/test/resources/commands.pan b/ncm-metaconfig/src/test/resources/commands.pan
index b6c8f8c4c6..e1444cb5fc 100644
--- a/ncm-metaconfig/src/test/resources/commands.pan
+++ b/ncm-metaconfig/src/test/resources/commands.pan
@@ -1,9 +1,3 @@
 object template commands;
 
-include 'simple';
-
-prefix "/software/components/metaconfig/services/{/foo/bar}/commands";
-"pre" = "cmd pre";
-"test" = "cmd test";
-"changed" = "cmd changed";
-"post" = "cmd post";
+include 'simple_commands';
diff --git a/ncm-metaconfig/src/test/resources/simple_commands.pan b/ncm-metaconfig/src/test/resources/simple_commands.pan
new file mode 100644
index 0000000000..5a20d95e48
--- /dev/null
+++ b/ncm-metaconfig/src/test/resources/simple_commands.pan
@@ -0,0 +1,15 @@
+unique template simple_commands;
+
+include 'simple';
+
+prefix "/software/components/metaconfig/commands";
+"cmd_pre" = "cmd pre";
+"cmd_test" = "cmd test";
+"cmd_changed" = "cmd changed";
+"cmd_post" = "cmd post";
+
+prefix "/software/components/metaconfig/services/{/foo/bar}/actions";
+"pre" = "cmd_pre";
+"test" = "cmd_test";
+"changed" = "cmd_changed";
+"post" = "cmd_post";

From f29c07efbf378afc006428221f717193ab85dc5b Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Fri, 12 Jun 2020 23:29:13 +0200
Subject: [PATCH 4/7] ncm-metaconfig: switch to no-shell and support "ok to
 fail" commands

---
 .../main/pan/components/metaconfig/schema.pan | 12 ++++++--
 ncm-metaconfig/src/main/perl/metaconfig.pm    | 30 ++++++++++++++-----
 ncm-metaconfig/src/test/perl/commands.t       | 26 +++++++++++-----
 .../src/test/resources/commands_fail_pre.pan  |  8 +++++
 .../src/test/resources/simple_commands.pan    |  8 ++---
 5 files changed, 61 insertions(+), 23 deletions(-)
 create mode 100644 ncm-metaconfig/src/test/resources/commands_fail_pre.pan

diff --git a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
index d82f0e0ff0..4612ae554b 100644
--- a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
+++ b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
@@ -130,16 +130,22 @@ type ${project.artifactId}_config =  {
     'contents' : ${project.artifactId}_extension
     @{Predefined conversions from EDG::WP4::CCM::TextRender}
     'convert' ? ${project.artifactId}_textrender_convert
-    @{Actions (i.e. named registered commands) to run on pre, validation and/or post step.
-      These are independent of daemons and are executed at time of processing each service.}
+    @{Actions (i.e. names found in /software/components/metadata/commands) to run when processing the service.
+      Refer to the metaconfig_actions type definition for the available hooks
+      for when a command may be run.}
     'actions' ? ${project.artifactId}_actions
 } = dict();
 
+@{Command must start with absolute path to executable.
+  If the executable is preceded with a '-', it means that the a command failure
+  treated as success w.r.t. reporting and continuation.}
+type ${project.artifactId}_command = string with match(SELF, '^-?/');
+
 type ${project.artifactId}_component = {
     include structure_component
     'services' : ${project.artifactId}_config{} with valid_absolute_file_paths(SELF)
     @{Command registry for allowed actions, keys should be used as action value}
-    'commands' ? string{}
+    'commands' ? ${project.artifactId}_command{}
 } with {
     foreach (esc_fn; srv; SELF['services']) {
         if (exists(srv['actions'])) {
diff --git a/ncm-metaconfig/src/main/perl/metaconfig.pm b/ncm-metaconfig/src/main/perl/metaconfig.pm
index d4d633d777..73bf03f891 100644
--- a/ncm-metaconfig/src/main/perl/metaconfig.pm
+++ b/ncm-metaconfig/src/main/perl/metaconfig.pm
@@ -154,6 +154,7 @@ use EDG::WP4::CCM::TextRender 18.6.1;
 use CAF::Service;
 use CAF::ServiceActions;
 use EDG::WP4::CCM::Path qw(unescape);
+use Text::ParseWords qw(old_shellwords);
 use Readonly;
 
 our $EC = LC::Exception::Context->new->will_store_all;
@@ -173,11 +174,23 @@ sub run_shell_command
 
     my $command = $commands->{$type};
     if ($command) {
-        $self->debug(1, "Going to run $type command '$command'");
+        my $error_on_fail = 1;
+        if ($command =~ m/^-/) {
+            $command =~ s/^-//;
+            $error_on_fail = 0;
+        }
+        my $cmd_ref = [old_shellwords($command)];
+        if (!$cmd_ref) {
+            $self->error("Failed to split '$command'");
+            return;
+        }
+
+        $self->verbose("Going to run $type command '$command' as ['",
+                       join("','", @$cmd_ref), "']",
+                       $error_on_fail ? "" : " and no error reporting on fail");
 
         my ($err, $out);
         my %opts = (
-            shell => 1,
             log => $self,
             stdout => \$out,
             stderr => \$err,
@@ -189,16 +202,17 @@ sub run_shell_command
             $opts{keeps_state} = 1;
         };
 
-        CAF::Process->new([$command], %opts)->execute();
+        CAF::Process->new($cmd_ref, %opts)->execute();
 
         my $ec = $?;
 
-        my $report = $ec ? 'error' : 'verbose';
+        my $report = $ec && $error_on_fail ? 'error' : 'verbose';
         $self->$report("run $type command '$command' ",
-                         ($ec ? 'failed' : 'ok'),
-                         ,": stdout '$out'\n stderr '$err'",
-                         ($input ? "\n stdin '$input'" : ""));
-        return $ec ? undef : 1;
+                       ($ec ? 'failed' : 'ok'),
+                       ($error_on_fail ? '' : ' (no error on fail set)'),
+                       ,": stdout '$out'\n stderr '$err'",
+                       ($input ? "\n stdin '$input'" : ""));
+        return $ec && $error_on_fail ? undef : 1;
     } else {
         $self->debug(5, "No $type command to run");
         return 1;
diff --git a/ncm-metaconfig/src/test/perl/commands.t b/ncm-metaconfig/src/test/perl/commands.t
index bcd36455b1..fe370ac02c 100644
--- a/ncm-metaconfig/src/test/perl/commands.t
+++ b/ncm-metaconfig/src/test/perl/commands.t
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 use Test::More;
-use Test::Quattor qw(commands);
+use Test::Quattor qw(commands commands_fail_pre);
 use NCM::Component::metaconfig;
 use Test::MockModule;
 
@@ -23,6 +23,7 @@ sub clean
 
 my $cmp = NCM::Component::metaconfig->new('metaconfig');
 my $cfg = get_config_for_profile('commands');
+my $cfg_fail = get_config_for_profile('commands_fail_pre');
 
 clean();
 
@@ -34,15 +35,15 @@ ok($fh, "A file was actually created");
 isa_ok($fh, "CAF::FileWriter");
 
 # if default sysv init service changes, also modify the aii_command negative test
-ok(command_history_ok(['cmd pre', 'cmd test', 'cmd changed', 'cmd post', 'service foo restart']),
+ok(command_history_ok(['/cmd pre', '/cmd test', '/cmd changed', '/cmd post', 'service foo restart']),
    "commands run and serivce foo restarted");
 
 clean();
 
 # changed failed, post does not run, daemons does run
-set_command_status('cmd changed', 1);
+set_command_status('/cmd changed', 1);
 $cmp->Configure($cfg);
-ok(command_history_ok(['cmd pre', 'cmd test', 'cmd changed', 'service foo restart'], ['cmd post']),
+ok(command_history_ok(['/cmd pre', '/cmd test', '/cmd changed', 'service foo restart'], ['/cmd post']),
    "commands except post run and serivce foo restarted");
 my $fcnt = get_file_contents("/foo/bar");
 ok($orig ne "$fcnt", "orig content is not same as current $fcnt");
@@ -50,9 +51,9 @@ ok($orig ne "$fcnt", "orig content is not same as current $fcnt");
 clean();
 
 # test failed, changed, post and daemons do not run, content unmodified
-set_command_status('cmd test', 1);
+set_command_status('/cmd test', 1);
 $cmp->Configure($cfg);
-ok(command_history_ok(['cmd pre', 'cmd test'], ['cmd changed', 'service foo restart', 'cmd post']),
+ok(command_history_ok(['/cmd pre', '/cmd test'], ['/cmd changed', 'service foo restart', '/cmd post']),
    "commands run except changed, post and no serivce foo restarted");
 $fcnt = get_file_contents("/foo/bar");
 # unmodified
@@ -60,13 +61,22 @@ is($orig, "$fcnt", "orig content is same as current $fcnt on test failed");
 
 clean();
 # pre failed, test, changed, post and daemons do not run, content unmodified
-set_command_status('cmd pre', 1);
+set_command_status('/cmd pre', 1);
 $cmp->Configure($cfg);
-ok(command_history_ok(['cmd pre'], ['cmd test', 'cmd changed', 'service foo restart', 'cmd post']),
+ok(command_history_ok(['/cmd pre'], ['/cmd test', '/cmd changed', 'service foo restart', '/cmd post']),
    "commands run except test, changed, post and no serivce foo restarted");
 $fcnt = get_file_contents("/foo/bar");
 # unmodified
 is($orig, "$fcnt", "orig content is same as current $fcnt on pre failed");
 
+clean();
+# Rerun with cmd_pre that can fail. Test still fails as usual
+$cmp->Configure($cfg_fail);
+ok(command_history_ok(['/cmd pre', '/cmd test'], ['/cmd changed', 'service foo restart', '/cmd post']),
+   "commands pre fails, but is ok so still runs test, and no changed, post and no serivce foo restarted");
+$fcnt = get_file_contents("/foo/bar");
+# unmodified
+is($orig, "$fcnt", "orig content is same as current $fcnt on pre failed but ok to fail and test fails");
+
 
 done_testing();
diff --git a/ncm-metaconfig/src/test/resources/commands_fail_pre.pan b/ncm-metaconfig/src/test/resources/commands_fail_pre.pan
new file mode 100644
index 0000000000..b54dcc48ae
--- /dev/null
+++ b/ncm-metaconfig/src/test/resources/commands_fail_pre.pan
@@ -0,0 +1,8 @@
+object template commands_fail_pre;
+
+include 'simple_commands';
+
+"/software/components/metaconfig/commands/cmd_pre_fail" =
+    "-" + value("/software/components/metaconfig/commands/cmd_pre");
+
+"/software/components/metaconfig/services/{/foo/bar}/actions/pre" = "cmd_pre_fail";
diff --git a/ncm-metaconfig/src/test/resources/simple_commands.pan b/ncm-metaconfig/src/test/resources/simple_commands.pan
index 5a20d95e48..d42b6db017 100644
--- a/ncm-metaconfig/src/test/resources/simple_commands.pan
+++ b/ncm-metaconfig/src/test/resources/simple_commands.pan
@@ -3,10 +3,10 @@ unique template simple_commands;
 include 'simple';
 
 prefix "/software/components/metaconfig/commands";
-"cmd_pre" = "cmd pre";
-"cmd_test" = "cmd test";
-"cmd_changed" = "cmd changed";
-"cmd_post" = "cmd post";
+"cmd_pre" = "/cmd pre";
+"cmd_test" = "/cmd test";
+"cmd_changed" = "/cmd changed";
+"cmd_post" = "/cmd post";
 
 prefix "/software/components/metaconfig/services/{/foo/bar}/actions";
 "pre" = "cmd_pre";

From da4aec9cf741482e90338479bcd7b5b6eda16bb4 Mon Sep 17 00:00:00 2001
From: Stijn De Weirdt <stijn.deweirdt@ugent.be>
Date: Wed, 28 Oct 2020 19:32:51 +0100
Subject: [PATCH 5/7] Update
 ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

Co-authored-by: Nathan Dimmock <ned21@toobusyto.org.uk>
---
 ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
index 4612ae554b..9ac8f58bc5 100644
--- a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
+++ b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
@@ -92,7 +92,7 @@ type ${project.artifactId}_actions = {
     @{Always run, happens before possible modifications, the file content is passed om stdin.
       Runs with 'keeps_state' enabled, so do not modify anything with this command.}
     'test' ? string
-    @{Only run after file is modified, but after daemon action is added}
+    @{Only run after file is modified, but before any daemon action is executed. A failure in this command has no effect on whether the daemon action is executed later.}
     'changed' ? string
     @{Always run, happens after possible modifications (and after 'changed').}
     'post' ? string

From 69b4b7b42d84b1b63d61193a10e42af7c050b17b Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Wed, 28 Oct 2020 20:52:35 +0100
Subject: [PATCH 6/7] ncm-metaconfig: improve schema docstrings

---
 .../src/main/pan/components/metaconfig/schema.pan | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
index 9ac8f58bc5..22c58102c5 100644
--- a/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
+++ b/ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan
@@ -87,14 +87,19 @@ type ${project.artifactId}_textrender_convert = {
 type caf_service_action = string with match(SELF, '^(restart|reload|stop_sleep_start)$');
 
 type ${project.artifactId}_actions = {
-    @{Always run, happens before possible modifications}
+    @{Always run, happens before possible modifications.
+      A failure will cancel any file modification, unless the command is prefixed with -.}
     'pre' ? string
-    @{Always run, happens before possible modifications, the file content is passed om stdin.
+    @{Always run before possible modifications with the new (or unchanged) file content is
+      passed on stdin. A failure will cancel any file modification,
+      unless the command is prefixed with -.
       Runs with 'keeps_state' enabled, so do not modify anything with this command.}
     'test' ? string
-    @{Only run after file is modified, but before any daemon action is executed. A failure in this command has no effect on whether the daemon action is executed later.}
+    @{Only run after file is modified, but before any daemon action is executed.
+      A failure in this command has no effect on whether the daemon action is executed later.}
     'changed' ? string
-    @{Always run, happens after possible modifications (and after 'changed').}
+    @{Always run, regardless of whether file was modified or not, and after the 'changed' action
+      but before any daemon action. A failure of this command has no effect on the subsequent daemon action.}
     'post' ? string
 };
 
@@ -137,7 +142,7 @@ type ${project.artifactId}_config =  {
 } = dict();
 
 @{Command must start with absolute path to executable.
-  If the executable is preceded with a '-', it means that the a command failure
+  If the executable is preceded with a '-', it means that a non-zero exit code (i.e. failure) is
   treated as success w.r.t. reporting and continuation.}
 type ${project.artifactId}_command = string with match(SELF, '^-?/');
 

From 1a391bcf8ea68182c98ac09f50e612dec00e61e1 Mon Sep 17 00:00:00 2001
From: stdweird <stijn.deweirdt@ugent.be>
Date: Fri, 30 Oct 2020 19:09:42 +0100
Subject: [PATCH 7/7] ncm-metaconfig: switch to shellwords for commandline
 splitting

---
 ncm-metaconfig/src/main/perl/metaconfig.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ncm-metaconfig/src/main/perl/metaconfig.pm b/ncm-metaconfig/src/main/perl/metaconfig.pm
index 73bf03f891..5d20a81c9e 100644
--- a/ncm-metaconfig/src/main/perl/metaconfig.pm
+++ b/ncm-metaconfig/src/main/perl/metaconfig.pm
@@ -154,7 +154,7 @@ use EDG::WP4::CCM::TextRender 18.6.1;
 use CAF::Service;
 use CAF::ServiceActions;
 use EDG::WP4::CCM::Path qw(unescape);
-use Text::ParseWords qw(old_shellwords);
+use Text::ParseWords qw(shellwords);
 use Readonly;
 
 our $EC = LC::Exception::Context->new->will_store_all;
@@ -179,8 +179,8 @@ sub run_shell_command
             $command =~ s/^-//;
             $error_on_fail = 0;
         }
-        my $cmd_ref = [old_shellwords($command)];
-        if (!$cmd_ref) {
+        my $cmd_ref = [shellwords($command)];
+        if (!@$cmd_ref) {
             $self->error("Failed to split '$command'");
             return;
         }
@@ -295,8 +295,8 @@ sub resolve_command_actions
             $commands->{$type} = $command;
             $self->verbose("Resolved $type action $action to command '$command'");
         } else {
-            # Not fatal, should be covered in schema already
-            $self->error("Unable to resovle $type action $action to command");
+            # Should not happen due to this being validated in the schema
+            $self->error("Unable to resolve $type action $action to command");
         }
     };
     return $commands;