Arvados

Javier Bértoli wrote:

Added parameter at 5115ff8a1, branch 17821-provision-dump-config-parameter

Review comments:

+  echo >&2 "  --dump-config <dest_dir>                    Dumps the pillars and states to a directory" 
+  echo >&2 "                                              Defaults to ./local_config_dump" 

The default mentioned seems to be incorrect (the code uses ./salt_config).

+if [ "${DUMP_CONFIG}" != "yes" ]; then
+  apt-get update
+  apt-get install -y curl git jq
+
...
+fi

It seems that this mode affects the functioning of the script. Why is the installation/configuration of the salt minion skipped in "dump config" mode?

Is "dump config" mode intended as a "noop" mode, or as a mode where more of a record of operation is made, but with otherwise the same effect? If the former, that should be made clear in in the help output.

-# git clone --branch "${NGINX_TAG}"       https://github.com/saltstack-formulas/nginx-formula.git
-git clone --branch "${NGINX_TAG}"       https://github.com/netmanagers/nginx-formula.git
+git clone --branch "${NGINX_TAG}"       https://github.com/saltstack-formulas/nginx-formula.git

It seems that this commit also changes the nginx dependency used, which seems to be an unrelated change. Was this an intentional inclusion? In general, please split unrelated changes into separate commits, with their own description.

Finally - how does this dump feature actually work? I don't see an argument passed to `salt_call`; does it rely on environment variables?

Addressed all these comments at aaf3e7305

Ward Vandewege wrote:

The default mentioned seems to be incorrect (the code uses ./salt_config).

Fixed.

It seems that this mode affects the functioning of the script. Why is the installation/configuration of the salt minion skipped in "dump config" mode?

This mode just dumps the set of pillars, states and formulas that will be applied when run without this parameter. The idea is to give you a set of files that you can inspect to make sure all will be deployed as desired or else, use them in a Saltstack master/minion mode, where you won't run the script.

Is "dump config" mode intended as a "noop" mode, or as a mode where more of a record of operation is made, but with otherwise the same effect? If the former, that should be made clear in in the help output.

Please let me know if the modifications I made to the code address this in a satisfactory way.

It seems that this commit also changes the nginx dependency used, which seems to be an unrelated change. Was this an intentional inclusion? In general, please split unrelated changes into separate commits, with their own description.

Reverted. Added #17823 to address this.

Finally - how does this dump feature actually work? I don't see an argument passed to `salt_call`; does it rely on environment variables?

The provision script is mostly a parser to simplify the preparation of pillars and states for the deployment of Arvados using Saltstack. As described in the installation docs using salt, the user creates a local.params file and a local_config_dir to customize as much as required the salt deployment, so no env vars nor parameters are passed in the command line at the moment.

Javier Bértoli wrote:

Addressed all these comments at aaf3e7305

Ward Vandewege wrote:

The default mentioned seems to be incorrect (the code uses ./salt_config).

Fixed.

Okay. I tried to use it, and the option requires an argument, so while the code has a default it is not used, and it is not usable:

 sudo ./provision.sh --dump-config
./provision.sh: option '--dump-config' requires an argument
GNU getopt missing? Use -h for help

Another thing: please remove the '-x' in the first line, the output is incomprehensible as is.

Another thing: that "GNU getopt missing?" line is wonky. Just test for the presence of getopt, and abort with an error saying it is required if it is not there.

I tried this on a fresh VM:

$ sudo ./provision.sh --dump-config ./local_config_dump
xxxx8
Cloning into 'arvados-formula'...
Cloning into 'docker-formula'...
remote: Enumerating objects: 2161, done.
remote: Counting objects: 100% (395/395), done.
remote: Compressing objects: 100% (166/166), done.
remote: Total 2161 (delta 225), reused 378 (delta 222), pack-reused 1766
Receiving objects: 100% (2161/2161), 513.89 KiB | 5.04 MiB/s, done.
Resolving deltas: 100% (1238/1238), done.
Note: checking out 'f89b1895faa836d2aa0965d3fdd27083ff7583b8'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

Cloning into 'locale-formula'...
remote: Enumerating objects: 549, done.
remote: Counting objects: 100% (120/120), done.
remote: Compressing objects: 100% (85/85), done.
remote: Total 549 (delta 55), reused 82 (delta 32), pack-reused 429
Receiving objects: 100% (549/549), 165.62 KiB | 3.18 MiB/s, done.
Resolving deltas: 100% (275/275), done.
Note: checking out 'cdfcf8c6772c72ff9b99fd9450f1d4314c79e671'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

Cloning into 'nginx-formula'...
remote: Enumerating objects: 2329, done.
remote: Counting objects: 100% (372/372), done.
remote: Compressing objects: 100% (193/193), done.
remote: Total 2329 (delta 190), reused 335 (delta 165), pack-reused 1957
Receiving objects: 100% (2329/2329), 565.77 KiB | 5.29 MiB/s, done.
Resolving deltas: 100% (1366/1366), done.
Cloning into 'postgres-formula'...
remote: Enumerating objects: 2559, done.
remote: Counting objects: 100% (166/166), done.
remote: Compressing objects: 100% (118/118), done.
remote: Total 2559 (delta 72), reused 116 (delta 38), pack-reused 2393
Receiving objects: 100% (2559/2559), 621.72 KiB | 6.76 MiB/s, done.
Resolving deltas: 100% (1451/1451), done.
Note: checking out 'ce800b09cd52e17ccea3e7a69dd17c2eb0e6a850'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

Cloning into 'letsencrypt-formula'...
remote: Enumerating objects: 944, done.
remote: Counting objects: 100% (155/155), done.
remote: Compressing objects: 100% (110/110), done.
remote: Total 944 (delta 76), reused 99 (delta 43), pack-reused 789
Receiving objects: 100% (944/944), 223.54 KiB | 3.99 MiB/s, done.
Resolving deltas: 100% (501/501), done.
Note: checking out 'c76514d86f7fe504a6ad9544a0b379e6a70530d6'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 384: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 391: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 401: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 401: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 409: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 417: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 418: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 419: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 422: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 423: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 424: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 425: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 426: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 427: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 428: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 429: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 430: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 431: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 432: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 541: salt-call: command not found
Removing .psql file
Copying the Arvados CA certificate to the installer dir, so you can import it
cp: cannot stat '/etc/ssl/certs/arvados-snakeoil-ca.pem': No such file or directory

Running it again leads to more/different errors:

$ sudo ./provision.sh --dump-config ./local_config_dump
xxxx8
fatal: destination path 'arvados-formula' already exists and is not an empty directory.
fatal: destination path 'docker-formula' already exists and is not an empty directory.
fatal: destination path 'locale-formula' already exists and is not an empty directory.
fatal: destination path 'nginx-formula' already exists and is not an empty directory.
fatal: destination path 'postgres-formula' already exists and is not an empty directory.
fatal: destination path 'letsencrypt-formula' already exists and is not an empty directory.
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 313: "${P_DIR}"/$(basename "${f}"): No such file or directory
./provision.sh: line 384: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 391: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 401: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 401: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 409: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 417: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 418: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 419: ./local_config_dump/salt/top.sls: No such file or directory
./provision.sh: line 422: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 423: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 424: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 425: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 426: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 427: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 428: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 429: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 430: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 431: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 432: ./local_config_dump/pillars/top.sls: No such file or directory
./provision.sh: line 541: salt-call: command not found
Removing .psql file
Copying the Arvados CA certificate to the installer dir, so you can import it
cp: cannot stat '/etc/ssl/certs/arvados-snakeoil-ca.pem': No such file or directory

Can you make it not spit out all that error output and/or fix the errors?

This mode just dumps the set of pillars, states and formulas that will be applied when run without this parameter. The idea is to give you a set of files that you can inspect to make sure all will be deployed as desired or else, use them in a Saltstack master/minion mode, where you won't run the script.

Is "dump config" mode intended as a "noop" mode, or as a mode where more of a record of operation is made, but with otherwise the same effect? If the former, that should be made clear in in the help output.

Please let me know if the modifications I made to the code address this in a satisfactory way.

Yeah, it's better thanks.

It seems that this commit also changes the nginx dependency used, which seems to be an unrelated change. Was this an intentional inclusion? In general, please split unrelated changes into separate commits, with their own description.

Reverted. Added #17823 to address this.

Thanks.

Finally - how does this dump feature actually work? I don't see an argument passed to `salt_call`; does it rely on environment variables?

The provision script is mostly a parser to simplify the preparation of pillars and states for the deployment of Arvados using Saltstack. As described in the installation docs using salt, the user creates a local.params file and a local_config_dir to customize as much as required the salt deployment, so no env vars nor parameters are passed in the command line at the moment.

Right, I was asking about the provision.sh script itself actually, you've now clarified that running it with `--dump-config` will not run salt.

Ward Vandewege wrote:

Okay. I tried to use it, and the option requires an argument, so while the code has a default it is not used, and it is not usable:

Fixed. To keep it consistent with the other parameters, I made specifying an output dir mandatory.

Another thing: please remove the '-x' in the first line, the output is incomprehensible as is.

Done. Now bash is verbose only if run with the --debug parameter.

Another thing: that "GNU getopt missing?" line is wonky. Just test for the presence of getopt, and abort with an error saying it is required if it is not there.

Done.

I tried this on a fresh VM:

[...]

Running it again leads to more/different errors:

[...]

Can you make it not spit out all that error output and/or fix the errors?

Done. Modified the git commands to manage the formulas repositories better.

Please, let me know if a368228fc fixes the issues you found.

Javier Bértoli wrote:

Ward Vandewege wrote:

Okay. I tried to use it, and the option requires an argument, so while the code has a default it is not used, and it is not usable:

Fixed. To keep it consistent with the other parameters, I made specifying an output dir mandatory.

Another thing: please remove the '-x' in the first line, the output is incomprehensible as is.

Done. Now bash is verbose only if run with the --debug parameter.

Another thing: that "GNU getopt missing?" line is wonky. Just test for the presence of getopt, and abort with an error saying it is required if it is not there.

Done.

I tried this on a fresh VM:

[...]

Running it again leads to more/different errors:

[...]

Can you make it not spit out all that error output and/or fix the errors?

Done. Modified the git commands to manage the formulas repositories better.

Please, let me know if a368228fc fixes the issues you found.

That looks much better, thanks. One thing left: you changed ARVADOS_TAG to `2.2.0`, but that's wrong, it needs to stay set to 'main'. It's really the ARVADOS_FORMULA_TAG, actually, so misnamed a bit.

That var should only be set to '2.2.0' on the 2.2-dev branch.

Ward Vandewege wrote:

One thing left: you changed ARVADOS_TAG to `2.2.0`, but that's wrong, it needs to stay set to 'main'. It's really the ARVADOS_FORMULA_TAG, actually, so misnamed a bit.

That var should only be set to '2.2.0' on the 2.2-dev branch.

Changed as suggested in 55f705b34. Added a ticket to rename the variable.

Javier Bértoli wrote:

Ward Vandewege wrote:

One thing left: you changed ARVADOS_TAG to `2.2.0`, but that's wrong, it needs to stay set to 'main'. It's really the ARVADOS_FORMULA_TAG, actually, so misnamed a bit.

That var should only be set to '2.2.0' on the 2.2-dev branch.

Changed as suggested in 55f705b34. Added a ticket to rename the variable.

LGTM, thanks.