cmd/status: Check for proper file for stats from cloud-init-generator

[valentindavid]

cloud-init-generator creates <rundir>/enabled when it enables
cloud-init and removes it when it disables. Nothing creates
<rundir>/disabled. So the status should check for the non existance
of <rundir>/enabled instead of the existance of <rundir>/disabled
to check that cloud-init-generator has disabled it.

[TheRealFalcon]

Hey @valentindavid , thanks for the PR! You're right in that our status code checks for a file that doesn't exist, but I think it'd be better to add a disabled file rather than check for the non-existence of enabled so we can distinguish between cloud-init being disabled via generator vs the generator having not run or failed somehow.

I'm thinking something like this:

diff --git a/systemd/cloud-init-generator.tmpl b/systemd/cloud-init-generator.tmpl
index db964825..6c4a66d1 100644
--- a/systemd/cloud-init-generator.tmpl
+++ b/systemd/cloud-init-generator.tmpl
@@ -9,6 +9,7 @@ LOG_F="/run/cloud-init/cloud-init-generator.log"
 ENABLE="enabled"
 DISABLE="disabled"
 RUN_ENABLED_FILE="$LOG_D/$ENABLE"
+RUN_DISABLED_FILE="$LOG_D/$DISABLE"
 CLOUD_TARGET_NAME="cloud-init.target"
 # lxc sets 'container', but lets make that explicitly a global
 CONTAINER="${container}"
@@ -151,6 +152,10 @@ main() {
                     "ln $CLOUD_SYSTEM_TARGET $link_path"
             fi
         fi
+        if [ -e $RUN_DISABLED_FILE ]; then
+            debug 1 "removing $RUN_DISABLED_FILE and creating $RUN_ENABLED_FILE"
+            rm -f $RUN_DISABLED_FILE
+        fi
         : > "$RUN_ENABLED_FILE"
     elif [ "$result" = "$DISABLE" ]; then
         if [ -f "$link_path" ]; then
@@ -164,8 +169,10 @@ main() {
             debug 1 "already disabled: no change needed [no $link_path]"
         fi
         if [ -e "$RUN_ENABLED_FILE" ]; then
+            debug 1 "removing $RUN_ENABLED_FILE and creating $RUN_DISABLED_FILE"
             rm -f "$RUN_ENABLED_FILE"
         fi
+        : > "$RUN_DISABLED_FILE"
     else
         debug 0 "unexpected result '$result' 'ds=$ds'"
         ret=3

If we go this route, that would also mean adding/updating some tests. If you don't mind doing that, then I'll leave that up to you. Otherwise, I could open a new PR for this.

[valentindavid]

If we go this route, that would also mean adding/updating some tests. If you don't mind doing that, then I'll leave that up to you. Otherwise, I could open a new PR for this.

Go ahead and open a new PR. You will probably be quicker. I do not know the code base. We need it for Ubuntu Core 22, so if you do not have time please just tell me and I will redo the PR.

I had a quick look and I did not find where cloud-init-generator was tested.

[valentindavid]

tests/unittests/cmd/test_status.py creates the disabled file.

[blackboxsw]

Hey @valentindavid , thanks for the PR! You're right in that our status code checks for a file that doesn't exist, but I think it'd be better to add a disabled file rather than check for the non-existence of enabled so we can distinguish between cloud-init being disabled via generator vs the generator having not run or failed somehow.

I'm thinking something like this:

diff --git a/systemd/cloud-init-generator.tmpl b/systemd/cloud-init-generator.tmpl
index db964825..6c4a66d1 100644
--- a/systemd/cloud-init-generator.tmpl
+++ b/systemd/cloud-init-generator.tmpl
@@ -9,6 +9,7 @@ LOG_F="/run/cloud-init/cloud-init-generator.log"
 ENABLE="enabled"
 DISABLE="disabled"
 RUN_ENABLED_FILE="$LOG_D/$ENABLE"
+RUN_DISABLED_FILE="$LOG_D/$DISABLE"
 CLOUD_TARGET_NAME="cloud-init.target"
 # lxc sets 'container', but lets make that explicitly a global
 CONTAINER="${container}"
@@ -151,6 +152,10 @@ main() {
                     "ln $CLOUD_SYSTEM_TARGET $link_path"
             fi
         fi
+        if [ -e $RUN_DISABLED_FILE ]; then
+            debug 1 "removing $RUN_DISABLED_FILE and creating $RUN_ENABLED_FILE"
+            rm -f $RUN_DISABLED_FILE
+        fi
         : > "$RUN_ENABLED_FILE"
     elif [ "$result" = "$DISABLE" ]; then
         if [ -f "$link_path" ]; then
@@ -164,8 +169,10 @@ main() {
             debug 1 "already disabled: no change needed [no $link_path]"
         fi
         if [ -e "$RUN_ENABLED_FILE" ]; then
+            debug 1 "removing $RUN_ENABLED_FILE and creating $RUN_DISABLED_FILE"
             rm -f "$RUN_ENABLED_FILE"
         fi
+        : > "$RUN_DISABLED_FILE"
     else
         debug 0 "unexpected result '$result' 'ds=$ds'"
         ret=3

If we go this route, that would also mean adding/updating some tests. If you don't mind doing that, then I'll leave that up to you. Otherwise, I could open a new PR for this.

I agree with this sentiment that we would want an artifact from the generator that touches /run/cloud-init/disabled. If we can add a unit test exercising this functionality I'm game for a unittest here. It's going to be a bit challenging given that we need to render from template and possibly override the RUN_DISABLED_FILE config. But we might be able to manipulate that value given the unit test will have to render from template anyway.

[TheRealFalcon]

Thanks for opening this @valentindavid . I'm going to close this PR in favor of #1349