Add support to force recreation of packs virtualenvs #5167

winem · 2021-02-25T21:43:08Z

This PR adds support for --register-setup-virtualenvs-force-recreation to st2ctl. Use-case is, like we have it now with the upgrade to st2 v3.4, an upgrade of the Python version inside the virtual environments for example.

I'll run some tests now but wanted to provide the PR already so that others who ran into this issue can give it a try, too.

arm4b · 2021-02-25T21:55:39Z

st2common/bin/st2ctl

+    echo "  --register-policies                           Register all policies."
+    echo "  --register-configs                            Register all configuration files."
+    echo "  --register-setup-virtualenvs                  Create Python virtual environments for all the registered packs."
+    echo "  --register-setup-virtualenvs-force-recreation (Delete and re-)create Python virtual environments for all the registered packs."


Thinking about the shorter version, how this would sound instead?

- --register-setup-virtualenvs-force-recreation + --register-setup-recreate-virtualenvs

I like it especially because it's more readable. Done & pushed.

I'll have to go to sleep now and will proceed with the testing tomorrow as well as looking into failing tests (if any).

OOH! The bike shed needs another coat of paint

Let's shorten that even farther:

- --register-setup-virtualenvs-force-recreation + --register-recreate-virtualenvs

I'm resisting the recommendation to drop "register", even though re/creating virtualenvs doesn't actually "register" anything. Alas, keeping "register" keeps the args consistent.

Yeah, I like --register-recreate-virtualenvs since it's the shortest :D

IIRC, register prefix was added at some point in the past due to some oslo config limitations or similar, but I forget the details.

Yes, there seems to be a limitation that requires the --register-xxx prefix. I can dig deeper into if needed but will focus on the failing tests and actual issues for now.

…tup-recreate-virtualenvs

m4dcoder · 2021-02-25T22:17:48Z

Can we rename options to --setup-venvs (replaces --register-setup-virtualenvs) and --reset-venvs (replaces --register-setup-virtualenvs-force-recreation)? Does venvs need registering, is that why we have to prefix it with register?

amanda11

Thanks for PR.

The tests are failing, because I think of a missing code change to register the new BoolOpt on the cfg, I've highlighted where I think it needs to go.

I also failed to run the command on upgrade using the artifact from this PR, which I thnk is same problem.

Also we probably should add a new test into ./st2common/tests/integration/test_register_content_script.py for the new parameter, similar to some of the ones already there for the current setup virtualenv.

amanda11 · 2021-02-26T09:54:44Z

st2common/bin/st2ctl


 function register_content() {
-  ALLOWED_REGISTER_FLAGS='--register-all --register-actions --register-aliases --register-runners --register-policies --register-rules --register-sensors --register-triggers --register-configs --register-setup-virtualenvs --register-fail-on-failure --register-no-fail-on-failure --verbose'
+  ALLOWED_REGISTER_FLAGS='--register-all --register-actions --register-aliases --register-runners --register-policies --register-rules --register-sensors --register-triggers --register-configs --register-setup-virtualenvs --register-setup-recreate-virtualenvs --register-fail-on-failure --register-no-fail-on-failure --verbose'


Does the new option need to get added to the sudo flags?

Yeah, it does need to run using sudo.

st2common/st2common/content/bootstrap.py

arm4b · 2021-02-26T16:04:02Z

The context is that a new flag is a nice to have addition that just mimicks the following construction, required for 3.4 upgrade only:

sudo rm -rf /opt/stackstorm/virtualenvs/*
sudo st2ctl reload --register-setup-virtualenvs

@m4dcoder I think at this point refactoring existing params is undesired as a last-minute release enhancement and would be an overkill breaking change.

…tion

winem · 2021-02-26T21:09:41Z

The context is that a new flag is a nice to have addition that just mimicks the following construction, required for 3.4 upgrade only:
sudo rm -rf /opt/stackstorm/virtualenvs/*
sudo st2ctl reload --register-setup-virtualenvs
@m4dcoder I think at this point refactoring existing params is undesired as a last-minute release enhancement and would be an overkill breaking change.

@armab we don't change any defaults so I don't see how this is could be a breaking change. Do I miss something?

cognifloyd · 2021-02-26T21:40:50Z

@winem I think @armab was referring to @m4dcoder's request to rename the options:

Can we rename options to --setup-venvs (replaces --register-setup-virtualenvs) and --reset-venvs (replaces --register-setup-virtualenvs-force-recreation)? Does venvs need registering, is that why we have to prefix it with register?

Renaming the options would be a breaking change. Merely adding a new option as this PR does, is not a breaking change, IMO.

Kami · 2021-02-26T21:53:29Z

Yeah, I agree with @cognifloyd.

In the future, I maybe be OK with renaming it (if we can work around oslo.config limitations), but for this last minute change I'm for --register-recreate-virtualenvs since it's the shortest and consistent with the current naming scheme.

blag · 2021-02-26T22:03:41Z

Try pinning mail-parser to >=3.9.1,<3.10.0 in contrib/core/requirements-tests.txt and see if that fixes it.

That package just had a release like 4 hours ago:
https://pypi.org/project/mail-parser/#history

And they might have broken something we were relying on, or fixed a bug that we were relying on.

winem · 2021-02-26T22:09:17Z

Oh saw that you just pinned it. So let's see if that fixes the issue.

@cognifloyd thanks for clarifying it. Now it makes sense.

amanda11 · 2021-02-26T22:52:46Z

Signed off my review comments, I've not had chance to try on an upgrade to check command works - but didn't want my comments to be a blocker for merging. But if it's been tested on an upgrade, then I'm happy.

blag · 2021-02-26T22:58:48Z

Travis needs payment. I'll be out for an hour and fix it after that.

winem · 2021-02-26T23:18:28Z

@amanda11 your input was actually very helpful and is always appreciated. And I added an integration test. :)

blag · 2021-02-27T01:33:50Z

Paid off Travis-CI, adding a commit to kick CI again.

arm4b · 2021-02-27T11:51:45Z

🎉

Add support to force recreation of packs virtualenvs

70ba173

pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Feb 25, 2021

winem added this to the 3.4.0 milestone Feb 25, 2021

arm4b reviewed Feb 25, 2021

View reviewed changes

winem added 2 commits February 25, 2021 22:59

Update setup_virtualenvs parameter to the new name

b092069

Rename --register-setup-virtualenvs-force-recreation to --register-se…

4f0e3ad

…tup-recreate-virtualenvs

amanda11 requested changes Feb 26, 2021

View reviewed changes

winem added 3 commits February 26, 2021 21:15

Switch to shorter register-recreate-virtualenvs and add the config op…

60d37b5

…tion

shorten comments to be lint compliant

6991175

Add log message and test for register-recreate-virtualenvs

6dae997

Add newline at end of file

e47cb0f

Pin mail-parser tighter - to 3.9.x

36b05fd

amanda11 approved these changes Feb 26, 2021

View reviewed changes

Add st2ctl changes to changelog

d60443d

blag merged commit d2e1860 into StackStorm:master Feb 27, 2021

arm4b added the feature label Feb 27, 2021

amanda11 mentioned this pull request Mar 1, 2021

Update reload virtualenv command on upgrade StackStorm/st2docs#1058

Merged

arm4b mentioned this pull request Mar 3, 2021

Propose adding @winem as a Maintainer #5179

Merged

cognifloyd mentioned this pull request May 7, 2022

Fix upgrade notes for v3.7 StackStorm/st2docs#1120

Merged

Uh oh!

Add support to force recreation of packs virtualenvs #5167

Add support to force recreation of packs virtualenvs #5167

Uh oh!

Conversation

winem commented Feb 25, 2021

Uh oh!

arm4b Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winem Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

cognifloyd Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

cognifloyd Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kami Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

winem Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

m4dcoder commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amanda11 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanda11 Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Kami Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arm4b commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winem commented Feb 26, 2021

Uh oh!

cognifloyd commented Feb 26, 2021

Uh oh!

Kami commented Feb 26, 2021

Uh oh!

blag commented Feb 26, 2021

Uh oh!

winem commented Feb 26, 2021

Uh oh!

amanda11 commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blag commented Feb 26, 2021

Uh oh!

winem commented Feb 26, 2021

Uh oh!

blag commented Feb 27, 2021

Uh oh!

arm4b commented Feb 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

arm4b Feb 25, 2021 •

edited

Loading

cognifloyd Feb 25, 2021 •

edited

Loading

m4dcoder commented Feb 25, 2021 •

edited

Loading

amanda11 left a comment •

edited

Loading

arm4b commented Feb 26, 2021 •

edited

Loading

amanda11 commented Feb 26, 2021 •

edited

Loading