m-lab / murakami

Run automated internet measurement tests in a Docker container.
Apache License 2.0
41 stars 11 forks source link

Add balena utilities #94

Closed critzo closed 3 years ago

critzo commented 3 years ago

This PR adds a script to provision a new Murakami device in a Balena Cloud fleet. Empty directories are also added:

Additions to .gitignore ensure downloaded .img files from Balena are not added to a commit, and .json device configuration files in the directories above.

@robertodauria PTAL?


This change is Reviewable

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 15 at r1 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
What happens if the balena client isn't authenticated? Shouldn't this script terminate early?

I'm assuming that the client has already authenticated, but the first prompt could be to check for the installation of balena-cli, and then for authentication. I could see some other interactive things that could be added too.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 36 at r1 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
> ``` > 2.38.3+rev3 > ``` If this is the image's version I would recommend putting it into a variable.

I'd like to pull that from the balena API and store it as a variable, but still looking at the API. Another one that could be made into a variable could be the device type.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 36 at r1 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
I wasn't thinking of getting it dynamically from the API (even if it's a nice long-term solution!), just giving it a name so the next person reading this script can understand what it is more easily. Without prior knowledge, I don't know if that's the device's version, the config's version, or the base Balena image's version.

Ah, I see. Some more informative code comments could clarify what's happening.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 15 at r1 (raw file):

Previously, critzo wrote…
I'm assuming that the client has already authenticated, but the first prompt could be to check for the installation of balena-cli, and then for authentication. I could see some other interactive things that could be added too.

There is now a check for 1) is balena-cli installed and 2) is the user logged in. Script stops if either of these is not the case.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 36 at r1 (raw file):

Previously, critzo wrote…
Ah, I see. Some more informative code comments could clarify what's happening.

I added a code comment that lets users know what this version string means. I hope to auto-detect that in the future. Also I added an API call to get the device type for the fleet automatically, and a prompt to ask the user whether they need to download the latest release image for the fleet or not.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 58 at r2 (raw file):


  availableDevice=$(balena util available-drives)

Also found this handy API call that checks to see if an SD card is inserted in the host system. If it is, then the final instructions suggest it as a target. In the future there could possibly be a user prompt to flash the card as well.

critzo commented 3 years ago

@robertodauria Made your requested changes and added some others.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 13 at r2 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
Could you please add a message for the user to install the Balena CLI in this case?

Added in latest commit.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 18 at r2 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
If there are no fleets this will still succeed since the table's header is printed anyway. I think it's OK as long as commands that rely on a certain fleet existing are checked for errors further down the script. In general, it's better to check for exit codes rather than relying on the output of the CLI to remain stable. e.g. this checks that the exit code of `balena fleets` is non-zero (true if the user hasn't logged in, or in case of network errors, etc.) ``` if ! balena fleets; then exit 1 fi ```

Added in latest commit.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 20 at r2 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
You can remove this `else` and unindent the following lines, since we've already exited in case of errors.

Removed in latest commit.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 27 at r2 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
If this can fail if `$balenaFleet` does not exist, please check for the exit code. You'll need to split the `balena device register` and the `awk` commands though, as `awk` will happily process an empty line end return an exit code = 0. Or, in this case, you can just check that the output is non-empty.

Set script options for error handling and pipefail per your suggestion of team practice for bash.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 44 at r2 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
Should this use the `$balenaFleet` variable as above? Checking for non-emptiness here is probably redundant, but it makes the script more robust in case the part before changes in the future. I'll leave it up to you :)

Pipefail and other error options now address this.

critzo commented 3 years ago

utilities/setup_balena_device.sh, line 50 at r2 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
Please check that this command succeeded before proceeding.

Same comment re: pipefail & script options.

critzo commented 3 years ago

utilities/balenaDeviceConfigs/README.md, line at r2 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…
Nit: the standard way of adding empty folders on Git is to add a file named `.gitkeep`. I assume this README will have some content in the future (e.g. to explain why the folder exists), so I think in this case it's fine. A possible alternative could be creating this folder as part of the setup script since it's only used there (and only to write, so it does not need to exist beforehand).

ack.

critzo commented 3 years ago

@robertodauria Changes committed. PTAL once more?