greenboot: add feature to disable healthchecks #137

djach7 · 2024-04-03T19:25:27Z

Addresses #119. Allows users to specify healthchecks to be skipped via a DISABLED_HEALTHCHECKS variable in greenboot.conf. Skipped healthchecks will be reflected with an appropriate message in the logs.

github-advanced-security

shellcheck found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

etc/greenboot/greenboot.conf

mmartinv · 2024-04-04T08:47:24Z

usr/libexec/greenboot/greenboot

+function exists_in_list {
+    VALUE=$1
+    echo "${DISABLED_HEALTHCHECKS,,}" | tr " " '\n' | grep -F -q -x "$VALUE"
+}


Suggested change

function exists_in_list {

VALUE=$1

echo "${DISABLED_HEALTHCHECKS,,}" | tr " " '\n' | grep -F -q -x "$VALUE"

}

function is_disabled {

HEALTHCHECK=$1

for DISABLED_HEALTHCHECK in "${DISABLED_HEALTHCHECKS[@]}"; do

[ "${HEALTHCHECK}" != "${DISABLED_HEALTHCHECK}" ] || return true

done

return false

}

You don't need to rely on external commands to iterate an array

Kind of covered by my comment above I believe, I used the grep one liner since I'm not using an array. Again, if it's agreed that an array makes more sense I can certainly change it

Using tr and grep for this seems a bit hacky to me

That's fair, I liked the tr and grep solution for its compactness. I agree that the looping solution you provided is easier to follow, though. (The grep solution also made me feel smart for all the fancy flags, but don't tell anyone that.)

Rewriting this to be more like what you have above and I just want to make sure I'm understanding it. In what you have above HEALTHCHECK does not equal DISABLED_HEALTHCHECK returns true, which in the context of the function means "yes it is disabled"? Shouldn't this be reversed, as HEALTHCHECK not equaling DISABLED_HEALTHCHECK would mean that it isn't in the list, and therefore not disabled? I think I am just confusing myself now.

The || operator in bash works this way:

It runs the command on the left side and if the returned code was 0 (the command finished successfully) then it skips the command on the right side and continues the execution (in this case it will continue with the next loop).

In case the return code is different than 0 (the command failed for whatever reason) then it runs the command on the right side.

So as long as HEALTHCHECK is different for the DISABLED_HEALTHCHECK the comparison ([ "${HEALTHCHECK}" != "${DISABLED_HEALTHCHECK}" ]) will be true so the command on the right side will be ignored and the for loop will keep running (we ignore the disabled health checks which are not equal to the healthcheck we are looking for).

On the other hand, if HEALTHCHECK is equal to DISABLED_HEALTHECK (we found that this health check is disabled) then the comparison will be false, the command on the right side is executed and the is_disabled function will return true.

In case the for loop ends, it means we didn't found the HEALTCHECK on the list (so it is not disabled) and the is_disabled function returns false.

Thank you for the explanation!! I have never used that operator in bash before, I thought it was a shorthand if statement (which I guess it sort of is, just not how I was picturing it). I rewrote it to be a one line if statement because that made more sense to me, but if I get a chance I'll try to rework it with the ||. Thanks again!

Addressed in the most recent updates, I was having issues getting the || to work though so I stuck with the if statement. Leaving this open so I remember to keep finagling with the ||

usr/libexec/greenboot/greenboot

mmartinv · 2024-04-05T14:51:29Z

usr/libexec/greenboot/greenboot

+  local required_hc_failed=false
+  echo "$start_msg"
+  for script in $(find "$scripts_dir" -name '*.sh' | sort); do
+    if is_disabled "$(basename "$script")"; then


I think you need to escape the double quotes surrounding $script:

"$(basename \"$script\")"

I just tested with this, it prevented the healthcheck from being skipped even if specified in the config. Going to leave as is for now

I see a lot of executions with "$(basename "$script")" . Why not using a variable to save the value at the begining of the loop and then use that variable?

Even better, why don't print only the script base name in find and use the ${script} variable directly:

for script in $(find "${scripts_dir}" -name '*.sh' -type f -printf '%f '); do if is_disabled "$script"; then echo "'${script}' was skipped, as specified in config" else local rc=0 systemd-cat -t "${script}" bash "${scripts_dir}/${script}" || rc=$? ...

I tried to implement this in the most recent updates but it kept failing to boot with these changes. I'm sure it's something I'm doing wrong, I just haven't figured it out yet

usr/libexec/greenboot/greenboot

miabbott · 2024-04-05T16:29:17Z

I think this change also needs some validation that the script name(s) specified in DISABLED_HEALTHCHECKS actually exist in the different directories that are searched.

Some psuedo-valid bash code:

DISABLED_HEALTHCHECKS=("01_repository_dns_check.sh 02_watchdog.sh 03_this_script_doesnt_exist.sh")

SCRIPTS_DIRS=("/usr/lib/greenboot/check/required.d"
              "/usr/lib/greenboot/check/wanted.d"
              "/usr/lib/greenboot/green.d"
              "/usr/lib/greenboot/red.d"
              "/etc/greenboot/check/required.d"
              "/etc/greenboot/check/wanted.d"
              "/etc/greenboot/green.d"
              "/etc/greenboot/red.d")

for DISABLED_HEALTH_CHECK in "${DISABLED_HEALTH_CHECKS[@]}"; do
    for SCRIPT_DIR in "${SCRIPTS_DIRS[@]}"; do
        if [ -f "${SCRIPT_DIR}/${DISABLED_HEALTH_CHECK}" ]; then
            echo "DEBUG: Found script; will disable it"
            break
        else
            echo "WARNING: Could not find script named ${DISABLED_HEALTH_CHECK} in any expected location"
        fi
    done
    <do other things with disabling the script>
done

mmartinv · 2024-04-05T16:36:18Z

I think this change also needs some validation that the script name(s) specified in DISABLED_HEALTHCHECKS actually exist in the different directories that are searched.

Some psuedo-valid bash code:

DISABLED_HEALTHCHECKS=("01_repository_dns_check.sh 02_watchdog.sh 03_this_script_doesnt_exist.sh")

SCRIPTS_DIRS=("/usr/lib/greenboot/check/required.d"
              "/usr/lib/greenboot/check/wanted.d"
              "/usr/lib/greenboot/green.d"
              "/usr/lib/greenboot/red.d"
              "/etc/greenboot/check/required.d"
              "/etc/greenboot/check/wanted.d"
              "/etc/greenboot/green.d"
              "/etc/greenboot/red.d")

for DISABLED_HEALTH_CHECK in "${DISABLED_HEALTH_CHECKS[@]}"; do
    for SCRIPT_DIR in "${SCRIPTS_DIRS[@]}"; do
        if [ -f "${SCRIPT_DIR}/${DISABLED_HEALTH_CHECK}" ]; then
            echo "DEBUG: Found script; will disable it"
            break
        else
            echo "WARNING: Could not find script named ${DISABLED_HEALTH_CHECK} in any expected location"
        fi
    done
    <do other things with disabling the script>
done

Actually I don't think it's needed because the function is called with existing scripts only

miabbott · 2024-04-05T16:39:28Z

Actually I don't think it's needed because the function is called with existing scripts only

Smart!

miabbott · 2024-04-05T16:43:29Z

Actually I don't think it's needed because the function is called with existing scripts only

I still think there is a chance of a typo when configuring DISABLED_HEALTHCHECKS that we should be able to warn users about, right?

Even if the function is called on an existing script (i.e. 01_repository_dns_check.sh), if I have defined DISABLED_HEALTHCHECKS=("01_repostory_dns_checks.sh"), that script will still run because I used the wrong script name.

etc/greenboot/greenboot.conf

djach7 · 2024-04-09T16:49:45Z

@miabbott I added a line in the config comments about needing exact spelling. If you'd like me to go beyond that and write something more please let me know

usr/libexec/greenboot/greenboot

say-paul · 2024-04-10T08:41:17Z

Apart from the config format, the changes looks good to me.

miabbott · 2024-04-10T11:57:55Z

@miabbott I added a line in the config comments about needing exact spelling. If you'd like me to go beyond that and write something more please let me know

It's a good first step, but I still think we should be able to warn the user if something in DISABLED_HEALTHCHECKS can't be found/doesn't exist. I don't want to block further progress on this implementation, so maybe you can do that kind of warning in a follow-up PR.

Addresses fedora-iot#119. Allows users to specify healthchecks to be skipped via a DISABLED_HEALTHCHECKS variable in greenboot.conf. Skipped healthchecks will be reflected with an appropriate message in the logs. Signed-off-by: djach7 <[email protected]>

github-advanced-security bot found potential problems Apr 3, 2024

View reviewed changes

djach7 linked an issue Apr 3, 2024 that may be closed by this pull request

provide mechanism for disabling certain default health checks #119

Closed

mmartinv requested changes Apr 4, 2024

View reviewed changes

djach7 force-pushed the healthcheck-disable branch 2 times, most recently from 8f50aba to 425c8b3 Compare April 5, 2024 14:43

github-advanced-security bot found potential problems Apr 5, 2024

View reviewed changes

usr/libexec/greenboot/greenboot Fixed Show fixed Hide fixed

mmartinv reviewed Apr 5, 2024

View reviewed changes

usr/libexec/greenboot/greenboot Outdated Show resolved Hide resolved

mmartinv reviewed Apr 5, 2024

View reviewed changes

djach7 force-pushed the healthcheck-disable branch from 425c8b3 to f51b92f Compare April 5, 2024 14:53

mmartinv reviewed Apr 5, 2024

View reviewed changes

usr/libexec/greenboot/greenboot Outdated Show resolved Hide resolved

say-paul reviewed Apr 8, 2024

View reviewed changes

etc/greenboot/greenboot.conf Outdated Show resolved Hide resolved

djach7 force-pushed the healthcheck-disable branch 3 times, most recently from c4ef2af to 6cc439e Compare April 9, 2024 16:47

7flying reviewed Apr 9, 2024

View reviewed changes

usr/libexec/greenboot/greenboot Show resolved Hide resolved

djach7 force-pushed the healthcheck-disable branch from 6cc439e to b57893d Compare April 9, 2024 18:34

github-advanced-security bot found potential problems Apr 9, 2024

View reviewed changes

usr/libexec/greenboot/greenboot Fixed Show fixed Hide fixed

djach7 force-pushed the healthcheck-disable branch from b57893d to f560ceb Compare April 9, 2024 18:38

say-paul approved these changes Apr 10, 2024

View reviewed changes

djach7 force-pushed the healthcheck-disable branch from f560ceb to 3948dbd Compare April 10, 2024 18:27

djach7 force-pushed the healthcheck-disable branch from 3948dbd to 7ee94c8 Compare April 11, 2024 13:45

7flying mentioned this pull request Apr 11, 2024

Warn the user when a disabled health check cannot be found #139

Closed

djach7 merged commit c2f5ff2 into fedora-iot:main Apr 11, 2024
12 checks passed

say-paul mentioned this pull request Sep 23, 2024

change packaging to not require default health check scripts #117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

greenboot: add feature to disable healthchecks #137

greenboot: add feature to disable healthchecks #137

djach7 commented Apr 3, 2024

github-advanced-security bot left a comment

mmartinv Apr 4, 2024 •

edited

Loading

djach7 Apr 4, 2024

mmartinv Apr 5, 2024

djach7 Apr 5, 2024

djach7 Apr 8, 2024

mmartinv Apr 9, 2024

djach7 Apr 9, 2024

djach7 Apr 9, 2024

mmartinv Apr 5, 2024

djach7 Apr 5, 2024

mmartinv Apr 5, 2024 •

edited

Loading

djach7 Apr 9, 2024

miabbott commented Apr 5, 2024

mmartinv commented Apr 5, 2024

miabbott commented Apr 5, 2024

miabbott commented Apr 5, 2024

djach7 commented Apr 9, 2024

say-paul commented Apr 10, 2024

miabbott commented Apr 10, 2024 •

edited

Loading

-function exists_in_list {
-    VALUE=$1
-    echo "${DISABLED_HEALTHCHECKS,,}" | tr " " '\n' | grep -F -q -x "$VALUE"
-}
+function is_disabled {
+    HEALTHCHECK=$1
+    for DISABLED_HEALTHCHECK in "${DISABLED_HEALTHCHECKS[@]}"; do
+        [ "${HEALTHCHECK}" != "${DISABLED_HEALTHCHECK}" ] || return true
+    done
+    return false
+}

greenboot: add feature to disable healthchecks #137

greenboot: add feature to disable healthchecks #137

Conversation

djach7 commented Apr 3, 2024

github-advanced-security bot left a comment

Choose a reason for hiding this comment

mmartinv Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmartinv Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miabbott commented Apr 5, 2024

mmartinv commented Apr 5, 2024

miabbott commented Apr 5, 2024

miabbott commented Apr 5, 2024

djach7 commented Apr 9, 2024

say-paul commented Apr 10, 2024

miabbott commented Apr 10, 2024 • edited Loading

mmartinv Apr 4, 2024 •

edited

Loading

mmartinv Apr 5, 2024 •

edited

Loading

miabbott commented Apr 10, 2024 •

edited

Loading