Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new standalone st2-pack-install CLI command for installing a pack #4256

Merged
merged 31 commits into from
Jul 31, 2018

Conversation

Kami
Copy link
Member

@Kami Kami commented Jul 20, 2018

This pull request adds a new independent and standalone st2-pack-install CLI command for installing a pack and setting up pack virtual environment.

End result is the same as running st2 pack install <foo> minus the content registration step.

This command is fully standalone and only requires Python, pip, st2common PyPi package and git binary to be installed on the system where it's used. It doesn't need / require database (MongoDB) and message bus access (RabbitMQ).

This command is designed to be used in environments where all the resources (packs) are baked into the base VM / container image which is then deployed.

Keep in mind that the actual content still needs to be registered with StackStorm at some later point when a cluster is deployed and when MongoDB and RabbitMQ are up (st2ctl reload --register-all which is also an idempotent operation).

NOTE: This change required some long needed refactoring - moved a bunch of code outside of the action which makes it re-usable elsewhere and also easier to test.

Usage:

./st2common/bin/st2-pack-install libcloud
./st2common/bin/st2-pack-install doesnt-exist
./st2common/bin/st2-pack-install libcloud==9.9.9
./st2common/bin/st2-pack-install libcloud --debug
./st2common/bin/st2-pack-install libcloud --debug
./st2common/bin/st2-pack-install libcloud xml=9.9.9 csv
./st2common/bin/st2-pack-install libcloud doesntexist csv xml=9.9.9

Resolves #3912.

Kami added 9 commits July 19, 2018 13:14
pack_management.download module.

This way code is decoupled better and can be re-used in other places.
environment.

This script doesn't depend on RabbitMQ and MongoDB connection and is to
be used and Docker and similar environments where content (packs) are
pre-baked into container / vm image.
functions in st2common.util.pack_management module.
@Kami Kami added this to the 2.9.0 milestone Jul 20, 2018
@arm4b
Copy link
Member

arm4b commented Jul 20, 2018

Can make it to install several packs st2-install-pack github ansible libcloud==9.9.9?
That would be consistent with the st2 pack install input syntax.

@arm4b
Copy link
Member

arm4b commented Jul 20, 2018

And yeah, you mentioned different naming: st2-pack-install vs st2-install-pack.

st2-pack-install looks more inlove with st2 pack install. I'd prefer this naming.

@Kami
Copy link
Member Author

Kami commented Jul 20, 2018

@armab Yeah, I was thinking about it, but I'm not a fan of multi pack install in a single command (never was) so I intentionally didn't add support for that.

The reason for that is that error reporting and handling gets much harder in such scenario since command now has many different failure scenarios.

For example, it can fail half way through - some packs are installed successfully and some are not.

In general in distributed systems and scenarios like that, I believe "one by one" approach is better since it makes error handling easier.

@nmaludy
Copy link
Member

nmaludy commented Jul 20, 2018

I'm in favor of the "multi-install" approach that @armab suggested.

This makes st2-pack-install more consistent with things like yum, apt-get, pip, gem, etc.

@arm4b
Copy link
Member

arm4b commented Jul 20, 2018

It'll make st2-pack-install consistent with st2 pack install, which already supports that kind of approach.

We want it to be as similar as possible in behavior, syntax, input with the original command, just don't interact with the DB/MQ. So from user's side original:
st2 pack install github ansible yolo=1.2.3 https://github.com/some/pack
can be easily transformed to:
st2-pack-install github ansible yolo=1.2.3 https://github.com/some/pack
when only pack download + setup virtualenv is needed.

@Kami
Copy link
Member Author

Kami commented Jul 20, 2018

I'll wait a bit and also let others chime on this one.

If anything, I would prefer to remove installing multiple packs at once support from st2 pack install as well (as mentioned above, I was never a fan of it) :P

@Kami
Copy link
Member Author

Kami commented Jul 20, 2018

@nmaludy The big difference between those tools and StackStorm is that all those tools support and use transactions for those operations which makes error handling much easier and straight forward (either all succeed or all fail, no middle ground).

That's not the case for StackStorm "st2 pack install" command. It doesn't support or use transactions so installing each pack is done in exactly the same manner if user ran the same command each time with a different pack.

The problem is that handling and dealing with such errors is much harder and more complex in multi pack install scenario without transactions - aka how to proceed / rectify if it fails hall way through.

And not to mention that current UX for multi pack install in st2 pack install command is very bad anyway. We don't really handle or report scenarios where installation of a particular pack fails or similar.

See the example below, or this one - https://gist.github.com/Kami/958d45c07e52934ce38fb462b12a8fb5:

2018-07-20 13:39:06,826  WARNING - Auth API server is not available, skipping authentication.

For the "libcloud, xmlx=9.9.9, csv" packs, the following content will be registered:

rules     |  0
sensors   |  0
aliases   |  0
actions   |  32
triggers  |  0

Installation may take a while for packs with many items.

	[ succeeded ] download pack
	[ succeeded ] make a prerun
	[ succeeded ] install pack dependencies
	[ succeeded ] register pack

+----------+-------------------------------------------------+---------+------------------+
| name     | description                                     | version | author           |
+----------+-------------------------------------------------+---------+------------------+
| csv      | st2 content pack containing CSV integrations    | 0.4.2   | StackStorm, Inc. |
| libcloud | st2 content pack containing libcloud            | 0.4.3   | StackStorm, Inc. |
|          | integrations                                    |         |                  |
+----------+-------------------------------------------------+---------+------------------+

So if anything, I'd rather have a fully working "install single pack per invocation" mode than broken (or at the very least kinda working which is not enough and gives user false sense of success) "install multiple packs in a single command invocation" mode.

It boils down to that "st2 pack install" should never support multi pack installation (or do it correctly) and it was probably added and rushed at some point without too much thinking.

And I don't want to repeat same bad behavior in a new command since there is no need to (and as mentioned, doing it correctly is not fully trivial and we would need to decide on the behavior we want).

@Kami
Copy link
Member Author

Kami commented Jul 20, 2018

I discussed this with @armab at length on Slack and we finally came up with a compromise. I will add support for installing multiple packs to this new command, but I won't really document it and installing single pack at once will be the preferred approach.

Either @armab and I will also file an issue for improvements in the existing packs.install action / st2 pack install CLI command so the end user experience and actual behavior on failures will be better and more consistent.

@Kami
Copy link
Member Author

Kami commented Jul 30, 2018

@armab I wasn't able to replicate the pack group issue locally.

It works fine and uses content.pack_group config option (which defaults to st2packs if not explicitly provided in the config).

(virtualenv) vagrant@ubuntu-xenial:/data/stanley$ ./st2common/bin/st2-pack-install bitcoin=0.1.0
2018-07-30 09:03:31,111 INFO [-] Installing pack "bitcoin=0.1.0"
2018-07-30 09:03:32,491 INFO [-] Successfuly installed pack "bitcoin=0.1.0"
2018-07-30 09:03:32,491 INFO [-] Setting up virtualenv for pack "bitcoin"
2018-07-30 09:03:35,689 INFO [-] Successfuly set up virtualenv for pack "bitcoin=0.1.0"
(virtualenv) vagrant@ubuntu-xenial:/data/stanley$ ls -la /opt/stackstorm/packs/
total 32
drwxr-xr-x 8 vagrant vagrant  4096 Jul 30 09:03 .
drwxr-xr-x 5 vagrant vagrant  4096 Jun 22 10:00 ..
drwxrwxr-x 4 vagrant st2packs 4096 Jul 30 09:03 bitcoin

@Kami
Copy link
Member Author

Kami commented Jul 30, 2018

@armab Pushed a fix for st2-pack-install bitcon=0.1.0 (ace321d).

I can't replicate your other two issues though.

  1. Group ownership stuff seems to work fine (see my comment above)
(virtualenv) vagrant@ubuntu-xenial:/data/stanley$ ./st2common/bin/st2-pack-install bitcoin=0.1.0
2018-07-30 09:03:31,111 INFO [-] Installing pack "bitcoin=0.1.0"
2018-07-30 09:03:32,491 INFO [-] Successfuly installed pack "bitcoin=0.1.0"
2018-07-30 09:03:32,491 INFO [-] Setting up virtualenv for pack "bitcoin"
2018-07-30 09:03:35,689 INFO [-] Successfully set up virtualenv for pack "bitcoin=0.1.0"
(virtualenv) vagrant@ubuntu-xenial:/data/stanley$ ls -la /opt/stackstorm/packs/
total 32
drwxr-xr-x 8 vagrant vagrant  4096 Jul 30 09:03 .
drwxr-xr-x 5 vagrant vagrant  4096 Jun 22 10:00 ..
drwxrwxr-x 4 vagrant st2packs 4096 Jul 30 09:03 bitcoin
  1. file://`` syntax also works fine as long as the input directory is a git repository (which is also the case for st2 pack install`
(virtualenv) vagrant@ubuntu-xenial:/data/stanley$ ./st2common/bin/st2-pack-install file:///opt/stackstorm/packs/bitcoin
2018-07-30 09:11:19,309 INFO [-] Installing pack "file:///opt/stackstorm/packs/bitcoin"
2018-07-30 09:11:19,373 INFO [-] Successfuly installed pack "bitcoin"
2018-07-30 09:11:19,373 INFO [-] Setting up virtualenv for pack "bitcoin"
2018-07-30 09:11:21,424 INFO [-] Successful set up virtualenv for pack "bitcoin"

Both of those things should work out of the box since they use exactly the same code path as `st2 pack install.

Kami added 7 commits July 30, 2018 11:15
can be re-used by other code.

Also re-use existing do_register_cli_opts instead of copy and pasting it
over.
Those commands are analogous of packs.download and packs.setup_virtualenv
StackStorm actions.

Running st2-pack-install is the same as running st2-pack-download first
and st2-pack-setup-virtualenv second.
@Kami
Copy link
Member Author

Kami commented Jul 30, 2018

As mentioned / discussed above, I added two new commands:

  1. st2-pack-download
st2-pack-download --help
usage: st2-pack-download [-h] [--config-dir DIR] [--config-file PATH]
                         [--debug] [--force] [--nodebug] [--noforce]
                         [--noprofile] [--nouse-debugger] [--noverbose]
                         [--noverify-ssl] [--profile] [--use-debugger]
                         [--verbose] [--verify-ssl] [--version]
                         [pack [pack ...]]
  1. st2-pack-setup-virtualenv
st2-pack-setup-virtualenv --help
usage: st2-pack-setup-virtualenv [-h] [--config-dir DIR] [--config-file PATH]
                                 [--debug] [--nodebug] [--noprofile]
                                 [--nopython3] [--noupdate] [--nouse-debugger]
                                 [--noverbose] [--profile] [--python3]
                                 [--update] [--use-debugger] [--verbose]
                                 [--version]
                                 [pack [pack ...]]

Those two commands are analogous of packs.download and packs.setup_virtualenv action and use / follow the same code paths.

Running st2-pack-install bitcoin is the same as running st2-pack-download bitcoin ; st2-pack-setup-virtualenv bitcoin.

This gives users more flexibility and I don't think command pollution is an issue (commands are prefixed with st2- and for now, we don't symlink them to /usr/local/bin).

I also prefer this approach over adding CLI flags to st2-pack-install command or similar. And as mentioned above, this way it's also consistent with the existing StackStorm actions.

@Kami
Copy link
Member Author

Kami commented Jul 30, 2018

NOTE: End to end tests will keep failing until I merge this - StackStorm/st2tests#139.

I plan to merge st2tests PR once I get this PR approved and once it's merged into master.

@arm4b
Copy link
Member

arm4b commented Jul 30, 2018

Try installing the packages produced from CircleCI. I guess that brings some diff.
That's what I'm still getting:

sudo /opt/stackstorm/st2/bin/st2-pack-install bitcoin=0.1.0
2018-07-30 14:56:38,490 INFO [-] Installing pack "bitcoin=0.1.0"
2018-07-30 14:56:40,167 INFO [-] Successfuly installed pack "bitcoin=0.1.0"
2018-07-30 14:56:40,167 INFO [-] Setting up virtualenv for pack "bitcoin=0.1.0"
Traceback (most recent call last):
  File "/opt/stackstorm/st2/bin/st2-pack-install", line 42, in <module>
    sys.exit(install_pack.main(sys.argv[1:]))
  File "/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2common/cmd/install_pack.py", line 81, in main
    no_download=True)
  File "/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2common/util/virtualenvs.py", line 70, in setup_pack_virtualenv
    raise ValueError('Invalid pack name "%s"' % (pack_name))
ValueError: Invalid pack name "bitcoin=0.1.0"

from latest package in this PR:

cat package.meta 
[server]
version = 2.9dev
git_sha = ac123a0
circle_build_url = https://circleci.com/gh/StackStorm/st2/8259

@Kami
Copy link
Member Author

Kami commented Jul 30, 2018

@armab Sorry, my bad. I accidentally removed the fix in a subsequent commit.

Fix is now back 48a243d.

@arm4b
Copy link
Member

arm4b commented Jul 30, 2018

Can confirm that pack-install script now works well with custom versions, file://, etc 👍

The group ownership is still an issue.
@Kami Per #4256 (review) comment check the virtualenvs dir, not the packs dir:

ls -la /opt/stackstorm/virtualenvs/
total 28
drwxrwxr-x  7 root st2packs 4096 Jul 30 16:06 .
drwxr-xr-x 11 root root     4096 Jul 17 16:47 ..
drwxr-xr-x  6 root root     4096 Jul 30 15:58 ansible
drwxr-xr-x  6 root root     4096 Jul 30 16:03 bitcoin
drwxr-xr-x  6 root root     4096 Jul 30 16:03 github
drwxrwxr-x  6 root st2packs 4096 Jul 30 16:06 hue
drwxrwxr-x  6 root st2packs 4096 Jul 18 17:45 st2

@Kami
Copy link
Member Author

Kami commented Jul 31, 2018

@armab I assume that's because st2actionrunnerservice which runs packs.setup_virtualenv action runs under root:st2packs (and nothing to do with the actual code).

We don't explicitly change or set permissions for pack virtualenv directory, it just inherits whatever user and group it runs under.

I can make the change and add some code which sets the group based on the pack_group config value inside setup_virtualenv code, but I'm not a fan of because of multiple reasons:

  1. It will require script to run as root / user which runs it will need to have sudo access
  2. There is an edge case and possible inconsistency right now between the group st2actionrunner process runs under and the value of content.pack_group config option.

In short, there really is no need for those commands to require sudo access - they only need it if we mess with the groups.

of the pack virtualenv directory to the value defined in the config
(content.pack_group, defaults to st2packs).

This way the end result is consistent also when running
st2-pack-setup-virtualenv and st2ctl reload --register-setup-virtualenvs
command as a user which is not part of st2packs group.

Keep in mind that this step requires sudo access, but if sudo is not
available, or group ownership change step fails, the command itself
doesn't fail.

This means that sudo access for st2-pack-{download,setup-virtualenv,install}
commands is optional.
@Kami
Copy link
Member Author

Kami commented Jul 31, 2018

@armab I pushed a commit (5513b7d) which I believe is a good compromise and results in the same outcome no matter how you run the command.

Previously, virtualenv directory owner group would only be set correctly when using action runner action (st2 pack install), but not when using st2ctl reload --register-setup-virtualenvs and st2-pack-setup-virtualenv command.

As mentioned above, I'm not a fan of those commands requiring sudo access. That's why the "change directory owner" step doesn't fail if the directory group ownership change fails due to one reason or another (defined group doesn't exist, no sudo access, etc.).

Keep in mind that this "ignore errors on change directory owner" behavior already existed before my changes, but I assume it was not done intentionally, but now that's done intentionally / as part of the design.

So in short, this fixes inconsistencies between the commands (good catch, this issue existed for the virtualenv directory when using st2ctl reload --register-setup-virutalenvs for a long time already) and also also makes sudo optional.

@arm4b
Copy link
Member

arm4b commented Jul 31, 2018

In a normal environment, when the StackStorm is shipped via packages you couldn't run st2-pack-install from normal user, because /opt/stackstorm/packs dir is owned by root:st2packs with no write access for others.

So anyways, it was already a requirement to run st2-pack-install as a privileged user.

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested & looks good 👍
Thanks for adding this feature!

@Kami Kami merged commit eabe1c9 into master Jul 31, 2018
@Kami Kami deleted the pack_install_and_setup_virtualenv_command branch July 31, 2018 13:39
@cognifloyd cognifloyd removed the RFR label Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add utility script which just setup virtualenvs for packs without DB/MQ connection
6 participants