Debugging a problem with netdata-updater.sh

[candlerb]

root@nuc1:~# uname -a
Linux nuc1 5.4.0-162-generic #179-Ubuntu SMP Mon Aug 14 08:51:31 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
root@nuc1:~# /opt/netdata/bin/netdata -v
netdata v1.42.1

root@nuc1:~# /etc/cron.daily/netdata-updater
Thu 14 Dec 15:04:51 GMT 2023 : INFO: netdata-updater:  Checking if a newer version of the updater script is available.
Thu 14 Dec 15:04:52 GMT 2023 : INFO: netdata-updater:  Entering /tmp/netdata-updater-sWkgE493ds
Thu 14 Dec 15:04:53 GMT 2023 : INFO: netdata-updater:  Current Version: 00104200100000
Thu 14 Dec 15:04:53 GMT 2023 : INFO: netdata-updater:  Latest Version: 00104400000045
Thu 14 Dec 15:04:53 GMT 2023 : INFO: netdata-updater:  Update available
Thu 14 Dec 15:04:54 GMT 2023 : FATAL: netdata-updater: FAILED TO UPDATE NETDATA:  Static binary checksum validation failed. Stopping netdata installation and leaving binary in /tmp/netdata-updater-sWkgE493ds
Usually this is a result of an older copy of the file being cached somewhere and can be resolved by simply retrying in an hour.

However, it's been like this for >24 hours.

Problem analysis

Copying the script and tracing it with #!/bin/sh -x, I can see it's downloading these files:

/usr/bin/curl -sSL --connect-timeout 10 --retry 3 https://github.com/netdata/netdata-nightlies/releases/download/v1.44.0-45-nightly/sha256sums.txt
/usr/bin/curl -sSL --connect-timeout 10 --retry 3 https://github.com/netdata/netdata-nightlies/releases/download/v1.44.0-45-nightly/netdata--latest.gz.run

If I try these by hand, the problem is with this one:

root@nuc1:~# /usr/bin/curl -sSL --connect-timeout 10 --retry 3 https://github.com/netdata/netdata-nightlies/releases/download/v1.44.0-45-nightly/netdata--latest.gz.run >netdata--latest.gz.run
root@nuc1:~# echo $?
0
root@nuc1:~# cat netdata--latest.gz.run
Not Foundroot@nuc1:~#

So it's actually generating a checksum of the text "Not Found", which clearly doesn't match :-)

Secondly, the double dash is suspicious. Looking at the shell debug output again:

+ set_tarball_urls nightly yes
+ filename=netdata-latest.tar.gz
+ [ yes = yes ]
+ [ -e /opt/netdata/etc/netdata/.install-type ]
+ . /opt/netdata/etc/netdata/.install-type
+ INSTALL_TYPE=legacy-static
+ filename=netdata--latest.gz.run
+ [ nightly = stable ]

The problem is here:

      # shellcheck disable=SC1091
      . /opt/netdata/etc/netdata/.install-type
      filename="netdata-${PREBUILT_ARCH}-latest.gz.run"

It seems PREBUILT_ARCH is not set, and indeed that's true:

root@nuc1:~# cat /opt/netdata/etc/netdata/.install-type
INSTALL_TYPE='legacy-static'
root@nuc1:~#

Whereas on another system I have:

root@nuc2:~# cat /opt/netdata/etc/netdata/.install-type
INSTALL_TYPE='kickstart-static'
PREBUILT_ARCH='x86_64'
root@nuc2:~#

Clearly I have a solution now, by adding PREBUILT_ARCH to that file.

Suggested improvements

Thiss was rather more debugging than I was expecting to do, and I think there are ways this script could be made more robust. Hence I'm making this a "discussion" of possible changes, rather than a specific bugfix.

Firstly, I think you should add the -f flag to curl. This will make it fail properly:

root@nuc1:~# /usr/bin/curl -fsSL --connect-timeout 10 --retry 3 https://github.com/netdata/netdata-nightlies/releases/download/v1.44.0-45-nightly/netdata--latest.gz.run >netdata--latest.gz.run
curl: (22) The requested URL returned error: 404
root@nuc1:~# echo $?
22

Patch:

@@ -366,7 +366,7 @@
   check_for_curl

   if [ -n "${curl}" ]; then
-    "${curl}" -sSL --connect-timeout 10 --retry 3 "${url}" > "${dest}"
+    "${curl}" -fsSL --connect-timeout 10 --retry 3 "${url}" > "${dest}"
     return $?
   elif command -v wget > /dev/null 2>&1; then
     wget -T 15 -O - "${url}" > "${dest}"

With just this change:

root@nuc1:~# ./netdata-updater.sh
Thu 14 Dec 16:33:03 GMT 2023 : INFO: netdata-updater.sh:  Checking if a newer version of the updater script is available.
Thu 14 Dec 16:33:04 GMT 2023 : INFO: netdata-updater.sh:  Entering /tmp/netdata-updater-TPsObTr2UY
Thu 14 Dec 16:33:05 GMT 2023 : INFO: netdata-updater.sh:  Current Version: 00104200100000
Thu 14 Dec 16:33:05 GMT 2023 : INFO: netdata-updater.sh:  Latest Version: 00104400000045
Thu 14 Dec 16:33:05 GMT 2023 : INFO: netdata-updater.sh:  Update available
curl: (22) The requested URL returned error: 404
Thu 14 Dec 16:33:06 GMT 2023 : FATAL: netdata-updater.sh: FAILED TO UPDATE NETDATA:  Cannot download https://github.com/netdata/netdata-nightlies/releases/download/v1.44.0-45-nightly/netdata--latest.gz.run

That is a more meaningful error than a checksum mismatch.

Secondly, the code could be made more robust against a missing PREBUILT_ARCH. Indeed, I see that lines 683-684 already have a check for this:

    sysarch="${PREBUILT_ARCH}"
    [ -z "$sysarch" ] && sysarch="$(uname -m)"

Unfortunately the corresponding logic is missing from 556. But that's easily added:

@@ -553,6 +553,7 @@
     if [ -e /opt/netdata/etc/netdata/.install-type ]; then
       # shellcheck disable=SC1091
       . /opt/netdata/etc/netdata/.install-type
+      [ -z "${PREBUILT_ARCH:-}" ] && PREBUILT_ARCH="$(uname -m)"
       filename="netdata-${PREBUILT_ARCH}-latest.gz.run"
     else
       filename="netdata-x86_64-latest.gz.run"

Thirdly, scripts can be made more robust in general against unexpected problems with #!/bin/sh -eu, so that (a) the script terminates on uncaught errors, and (b) the script terminates on unset variable expansions. (With bash I also do set -o pipefail so that a | b will fail if a fails, but that's not portable)

However, that means that all variable expansions which are permitted to be unset need changing - e.g. ${1} to ${1:-}. I made some of these (see below) and now it does what I expect:

root@nuc1:~# ./netdata-updater.sh
Thu 14 Dec 16:38:37 GMT 2023 : INFO: netdata-updater.sh:  Checking if a newer version of the updater script is available.
./netdata-updater.sh: 556: PREBUILT_ARCH: parameter not set

I include the full diff below for reference. However, it's quite possible I've not found everything that needs changing - I was looking for instances of -n and -z tests where the variable clearly wasn't set in a previous line. If this is incomplete, it could easily be adding new places for the script to crash. As a result, you may decide it's better not to make this change.

--- /opt/netdata/usr/libexec/netdata/netdata-updater.sh	2023-12-14 16:41:21.250040399 +0000
+++ netdata-updater.sh	2023-12-14 16:56:24.117597834 +0000
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/sh -eu

 # Netdata updater utility
 #
@@ -73,14 +73,14 @@

 error() {
   echo >&3 "$(date) : ERROR: ${script_name}: " "${1}"
-  if [ -n "${NETDATA_SAVE_WARNINGS}" ]; then
+  if [ -n "${NETDATA_SAVE_WARNINGS:-}" ]; then
     NETDATA_WARNINGS="${NETDATA_WARNINGS}\n  - ${1}"
   fi
 }

 fatal() {
   echo >&3 "$(date) : FATAL: ${script_name}: FAILED TO UPDATE NETDATA: " "${1}"
-  if [ -n "${NETDATA_SAVE_WARNINGS}" ]; then
+  if [ -n "${NETDATA_SAVE_WARNINGS:-}" ]; then
     NETDATA_WARNINGS="${NETDATA_WARNINGS}\n  - ${1}"
   fi
   exit_reason "${1}" "${2}"
@@ -88,11 +88,11 @@
 }

 exit_reason() {
-  if [ -n "${NETDATA_SAVE_WARNINGS}" ]; then
+  if [ -n "${NETDATA_SAVE_WARNINGS:-}" ]; then
     EXIT_REASON="${1}"
     EXIT_CODE="${2}"
-    if [ -n "${NETDATA_PROPAGATE_WARNINGS}" ]; then
-      if [ -n "${NETDATA_SCRIPT_STATUS_PATH}" ]; then
+    if [ -n "${NETDATA_PROPAGATE_WARNINGS:-}" ]; then
+      if [ -n "${NETDATA_SCRIPT_STATUS_PATH:-}" ]; then
         {
           echo "EXIT_REASON=\"${EXIT_REASON}\""
           echo "EXIT_CODE=\"${EXIT_CODE}\""
@@ -330,11 +330,11 @@
 }

 create_tmp_directory() {
-  if [ -n "${NETDATA_TMPDIR_PATH}" ]; then
+  if [ -n "${NETDATA_TMPDIR_PATH:-}" ]; then
     echo "${NETDATA_TMPDIR_PATH}"
   else
-    if [ -z "${NETDATA_TMPDIR}" ] || _cannot_use_tmpdir "${NETDATA_TMPDIR}" ; then
-      if [ -z "${TMPDIR}" ] || _cannot_use_tmpdir "${TMPDIR}" ; then
+    if [ -z "${NETDATA_TMPDIR:-}" ] || _cannot_use_tmpdir "${NETDATA_TMPDIR}" ; then
+      if [ -z "${TMPDIR:-}" ] || _cannot_use_tmpdir "${TMPDIR}" ; then
         if _cannot_use_tmpdir /tmp ; then
           if _cannot_use_tmpdir "${PWD}" ; then
             fatal "Unable to find a usable temporary directory. Please set \$TMPDIR to a path that is both writable and allows execution of files and try again." U0003
@@ -354,7 +354,7 @@
 }

 check_for_curl() {
-  if [ -z "${curl}" ]; then
+  if [ -z "${curl:-}" ]; then
     curl="$(PATH="${PATH}:/opt/netdata/bin" command -v curl 2>/dev/null && true)"
   fi
 }
@@ -366,7 +366,7 @@
   check_for_curl

   if [ -n "${curl}" ]; then
-    "${curl}" -sSL --connect-timeout 10 --retry 3 "${url}" > "${dest}"
+    "${curl}" -fsSL --connect-timeout 10 --retry 3 "${url}" > "${dest}"
     return $?
   elif command -v wget > /dev/null 2>&1; then
     wget -T 15 -O - "${url}" > "${dest}"
@@ -445,7 +445,7 @@
 }

 self_update() {
-  if [ -z "${NETDATA_NO_UPDATER_SELF_UPDATE}" ] && newer_commit_date; then
+  if [ -z "${NETDATA_NO_UPDATER_SELF_UPDATE:-}" ] && newer_commit_date; then
     info "Downloading newest version of updater script."

     ndtmpdir=$(create_tmp_directory)
@@ -509,7 +509,7 @@
 }

 update_available() {
-  if [ "$NETDATA_FORCE_UPDATE" = "1" ]; then
+  if [ "${NETDATA_FORCE_UPDATE:-}" = "1" ]; then
      info "Force update requested"
      return 0
   fi
@@ -553,6 +553,7 @@
     if [ -e /opt/netdata/etc/netdata/.install-type ]; then
       # shellcheck disable=SC1091
       . /opt/netdata/etc/netdata/.install-type
+      [ -z "${PREBUILT_ARCH:-}" ] && PREBUILT_ARCH="$(uname -m)"
       filename="netdata-${PREBUILT_ARCH}-latest.gz.run"
     else
       filename="netdata-x86_64-latest.gz.run"
@@ -582,7 +583,7 @@
   if update_available; then
     download "${NETDATA_TARBALL_CHECKSUM_URL}" "${ndtmpdir}/sha256sum.txt" >&3 2>&3
     download "${NETDATA_TARBALL_URL}" "${ndtmpdir}/netdata-latest.tar.gz"
-    if [ -n "${NETDATA_TARBALL_CHECKSUM}" ] &&
+    if [ -n "${NETDATA_TARBALL_CHECKSUM:-}" ] &&
       grep "${NETDATA_TARBALL_CHECKSUM}" sha256sum.txt >&3 2>&3 &&
       [ "$NETDATA_FORCE_UPDATE" != "1" ]; then
       info "Newest version is already installed"
@@ -618,7 +619,7 @@

     env="env TMPDIR=${TMPDIR}"

-    if [ -n "${NETDATA_SELECTED_DASHBOARD}" ]; then
+    if [ -n "${NETDATA_SELECTED_DASHBOARD:-}" ]; then
       env="${env} NETDATA_SELECTED_DASHBOARD=${NETDATA_SELECTED_DASHBOARD}"
     fi

@@ -650,7 +651,7 @@
         . "${NETDATA_SCRIPT_STATUS_PATH}"
         rm -f "${NETDATA_SCRIPT_STATUS_PATH}"
       fi
-      if [ -n "${EXIT_REASON}" ]; then
+      if [ -n "${EXIT_REASON:-}" ]; then
         fatal "Failed to rebuild existing netdata install: ${EXIT_REASON}" "U${EXIT_CODE}"
       else
         fatal "Failed to rebuild existing netdata reinstall." UI0000
@@ -680,7 +681,7 @@
   cd "${ndtmpdir}" || fatal "Failed to change current working directory to ${ndtmpdir}" U0019

   if update_available; then
-    sysarch="${PREBUILT_ARCH}"
+    sysarch="${PREBUILT_ARCH:-}"
     [ -z "$sysarch" ] && sysarch="$(uname -m)"
     download "${NETDATA_TARBALL_CHECKSUM_URL}" "${ndtmpdir}/sha256sum.txt"
     download "${NETDATA_TARBALL_URL}" "${ndtmpdir}/netdata-${sysarch}-latest.gz.run"
@@ -737,7 +738,7 @@
       opensuse-leap|opensuse-tumbleweed)
         DISTRO_COMPAT_NAME="opensuse"
         ;;
-      cloudlinux|almalinux|centos-stream|rocky|rhel)
+      cloudlinux|almalinux|rocky|rhel)
         DISTRO_COMPAT_NAME="centos"
         ;;
       *)
@@ -906,7 +907,7 @@
   . "$(dirname "${ENVIRONMENT_FILE}")/netdata-updater.conf"
 fi

-while [ -n "${1}" ]; do
+while [ -n "${1:-}" ]; do
   case "${1}" in
     --not-running-from-cron) NETDATA_NOT_RUNNING_FROM_CRON=1 ;;
     --no-updater-self-update) NETDATA_NO_UPDATER_SELF_UPDATE=1 ;;
@@ -951,7 +952,7 @@
 export NETDATA_LIB_DIR="${NETDATA_LIB_DIR:-${NETDATA_PREFIX}/var/lib/netdata}"

 # Source the tarball checksum, if not already available from environment (for existing installations with the old logic)
-[ -z "${NETDATA_TARBALL_CHECKSUM}" ] && [ -f "${NETDATA_LIB_DIR}/netdata.tarball.checksum" ] && NETDATA_TARBALL_CHECKSUM="$(cat "${NETDATA_LIB_DIR}/netdata.tarball.checksum")"
+[ -z "${NETDATA_TARBALL_CHECKSUM:-}" ] && [ -f "${NETDATA_LIB_DIR}/netdata.tarball.checksum" ] && NETDATA_TARBALL_CHECKSUM="$(cat "${NETDATA_LIB_DIR}/netdata.tarball.checksum")"

 if echo "$INSTALL_TYPE" | grep -qv ^binpkg && [ "${INSTALL_UID}" != "$(id -u)" ]; then
   fatal "You are running this script as user with uid $(id -u). We recommend to run this script as root (user with uid 0)" U0011
@@ -979,7 +980,7 @@
     custom)
       # At this point, we _should_ have a valid `.environment` file, but it's best to just check.
       # If we do, then behave like the legacy updater.
-      if validate_environment_file && [ -n "${IS_NETDATA_STATIC_BINARY}" ]; then
+      if validate_environment_file && [ -n "${IS_NETDATA_STATIC_BINARY:-}" ]; then
         update_legacy
       else
         fatal "This script does not support updating custom installations without valid environment files." U0012

[Ferroin]

First and foremost, thank you for such a well written analysis on this. It’s truly helpful for us to be able to provide quicker and more specific fixes.

As far as the issue itself, it looks like you stumbled across an edge case we had missed when writing that bit of the updater script, though there’s probably also an issue with the code in the installer that was supposed to properly update the .install-type file for old installs that did not have them (I’ll look into that myself at some point later though).

I agree with your assessments for fixes for the immediate issue as well. If you would be willing to submit a PR with just the change to the curl options and the specific workaround for the missing PREBUILT_ARCH variable (the script can be found in the repo at packaging/installer/netdata-updater.sh), I think we can probably get that merged and approved by next Monday at the latest. If you would prefer, I could put together a PR instead, but I would like to make sure you get the credit you deserve since you’ve essentially done all the work.

The larger-scale fixes are a bit trickier. I entirely agree that we need to move in that direction long-term, but there are a number of cases in our scripts where we’re unfortunately relying on a non-zero exit status from a command being silently ignored, so it’s not quite as simple as adding a few extra shell options and fixing up variable expansions.

[candlerb]

NP. #16613