Merge lp:~fo0bar/turku/turku-agent-gonogo into lp:turku/turku-agent

[Ryan Finnie]

Allow gonogo_program to be stored in config, allow --gonogo-program to be shlex-split

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Tom Haddon (mthaddon)]

Could you explain a little more about what this change is doing, ideally in the help for the command and comments in the code?

It seems a little confusing to have a variable called "gonogo_program" that's actually a list.

And if I'm understanding this right, this option can be set either via the command line option or via a config option on disk? I'm not entirely sure, but it looks like that's the case from the change to turku_agent/utils.py.

Looks like there's a fair amount of techdebt here in terms of lack of lint or tests. I don't have enough context on the change to understand whether it makes sense to block on those or have those happen as a separate change. Could you explain why this is needed and what the impact is?

review: Needs Information

[lp:~fo0bar/turku/turku-agent-gonogo]

52. By Ryan Finnie on 2020-03-23

Allow gonogo_program to be stored in config, allow --gonogo-program to be shlex-split

[Ryan Finnie (fo0bar)]

> Could you explain a little more about what this change is doing, ideally in
> the help for the command and comments in the code?

> Looks like there's a fair amount of techdebt here in terms of lack of lint or
> tests. I don't have enough context on the change to understand whether it
> makes sense to block on those or have those happen as a separate change. Could
> you explain why this is needed and what the impact is?

Change has been updated. Yeah, the codebase could use some large QoL updates, but in a nutshell, the go/no-go program is to prevent pings (and ultimately backups) during sensitive times as determined on the agent machine itself.

> It seems a little confusing to have a variable called "gonogo_program" that's
> actually a list.

Done. The existing command line arg remains --gonogo-program but I've documented it, and kept etc config as gonogo_program to be consistent with the arg, but I've made them loose enough to either be a list (program + args) or string (shlex-split). The post-processed variable has been changed to gonogo_program_and_args to avoid confusion.

> And if I'm understanding this right, this option can be set either via the
> command line option or via a config option on disk? I'm not entirely sure, but
> it looks like that's the case from the change to turku_agent/utils.py.

Yeah. Existing --gonogo-program, but I wanted to add the on-disk etc option so you didn't have to edit a cron file or systemd timer (it was the only thing you couldn't do through etc, and the .cron and .timer files were essentially part of the software package). Added help to --gonogo-program.

This repo could definitely use some documentation, or at least a README.

[Tom Haddon (mthaddon)]

LGTM

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 52

=== modified file 'turku_agent/ping.py'
--- turku_agent/ping.py	2019-04-22 01:39:55 +0000
+++ turku_agent/ping.py	2020-03-23 22:32:41 +0000
@@ -19,6 +19,7 @@
 import os
 import json
 import random
+import shlex
 import subprocess
 import tempfile
 import time
@@ -34,7 +35,8 @@
     parser.add_argument('--wait', '-w', type=float)
     parser.add_argument('--restore', action='store_true')
     parser.add_argument('--restore-storage', type=str, default=None)
-    parser.add_argument('--gonogo-program', type=str, default=None)
+    parser.add_argument('--gonogo-program', type=str, default=None,
+                        help='Go/no-go program run each time to determine whether to ping')
     return parser.parse_args()
 
 
@@ -93,9 +95,21 @@
         return
 
     # If a go/no-go program is defined, run it and only go if it exits 0.
-    if args.gonogo_program is not None:
+    # Example: prevent backups during high-load for sensitive systems:
+    #   ['check_load', '-c', '1,5,15']
+    gonogo_program = args.gonogo_program if args.gonogo_program else config['gonogo_program']
+    if isinstance(gonogo_program, (list, tuple)):
+        # List, program name first, optional arguments after
+        gonogo_program_and_args = list(gonogo_program)
+    elif isinstance(gonogo_program, str):
+        # String, shlex split it
+        gonogo_program_and_args = shlex.split(gonogo_program)
+    else:
+        # None
+        gonogo_program_and_args = []
+    if gonogo_program_and_args:
         try:
-            subprocess.check_call([args.gonogo_program])
+            subprocess.check_call(gonogo_program_and_args)
         except (subprocess.CalledProcessError, OSError):
             return
 
 
=== modified file 'turku_agent/utils.py'
--- turku_agent/utils.py	2019-04-22 01:19:36 +0000
+++ turku_agent/utils.py	2020-03-23 22:32:41 +0000
@@ -160,6 +160,11 @@
     if 'ssh_command' not in config:
         config['ssh_command'] = ['ssh']
 
+    # If a go/no-go program is defined, run it and only go if it exits 0.
+    # Type: String (program with no args) or list (program first, optional arguments after)
+    if 'gonogo_program' not in config:
+        config['gonogo_program'] = None
+
     var_sources_d = os.path.join(config['var_dir'], 'sources.d')
 
     # Validate the unit name