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

Issue #306 : Adding the ability to use overrides #307

Merged
merged 61 commits into from
May 17, 2022

Conversation

cwilson21
Copy link
Contributor

@cwilson21 cwilson21 commented May 13, 2022

After working to get this implemented and tested. It appears that the changes I have added are working.

This creates the /opt/stackstorm/overrides/ mountpoint in the st2client pod so that if reloads happen in the pod it will keep the settings provided by users in the overrides. This also registers overrides when the st2 cluster is being built via the register-content job.

THis is just extending the now existing override functionality that exists in Stackstorm 3.7.0 into the HA version of Stackstorm that is now operating on version 3.7.0

2022-05-13 22:08:52,283 INFO [-] Connecting to database "st2" @ "mongodb-local-headless:27017" as user "st2-admin".
2022-05-13 22:08:52,294 INFO [-] Successfully connected to database "st2" @ "mongodb-local-headless:27017" as user "st2-admin".
2022-05-13 22:08:52,734 INFO [-] =========================================================
2022-05-13 22:08:52,734 INFO [-] ############## Registering triggers #####################
2022-05-13 22:08:52,734 INFO [-] =========================================================
2022-05-13 22:08:53,152 INFO [-] Registered 1 triggers.
2022-05-13 22:08:53,152 INFO [-] =========================================================
2022-05-13 22:08:53,152 INFO [-] ############## Registering sensors ######################
2022-05-13 22:08:53,153 INFO [-] =========================================================
2022-05-13 22:08:54,528 INFO [-] Registered 40 sensors.
2022-05-13 22:08:54,529 INFO [-] 0 sensors had their metadata overridden.
2022-05-13 22:08:54,529 INFO [-] =========================================================
2022-05-13 22:08:54,529 INFO [-] ############## Registering runners ######################
2022-05-13 22:08:54,529 INFO [-] =========================================================
2022-05-13 22:08:55,912 INFO [-] Registered 14 runners.
2022-05-13 22:08:55,912 INFO [-] =========================================================
2022-05-13 22:08:55,912 INFO [-] ############## Registering actions ######################
2022-05-13 22:08:55,913 INFO [-] =========================================================
2022-05-13 22:10:24,631 INFO [-] Registered 4473 actions.
2022-05-13 22:10:24,631 INFO [-] 0 actions had their metadata overridden.
2022-05-13 22:10:24,631 INFO [-] =========================================================
2022-05-13 22:10:24,631 INFO [-] ############## Registering rules ########################
2022-05-13 22:10:24,631 INFO [-] =========================================================
2022-05-13 22:10:24,805 INFO [-] Registered 5 rules.
2022-05-13 22:10:24,805 INFO [-] 5 rules had their metadata overridden.
2022-05-13 22:10:24,805 INFO [-] =========================================================
2022-05-13 22:10:24,805 INFO [-] ############## Registering aliases ######################
2022-05-13 22:10:24,805 INFO [-] =========================================================
2022-05-13 22:10:25,081 INFO [-] Registered 15 aliases.
2022-05-13 22:10:25,081 INFO [-] 0 aliases had their metadata overridden.
2022-05-13 22:10:25,082 INFO [-] =========================================================
2022-05-13 22:10:25,082 INFO [-] ############## Registering policy types #################
2022-05-13 22:10:25,082 INFO [-] =========================================================
2022-05-13 22:10:25,112 INFO [-] Registered 3 policy types.
2022-05-13 22:10:25,112 INFO [-] =========================================================
2022-05-13 22:10:25,112 INFO [-] ############## Registering policies #####################
2022-05-13 22:10:25,112 INFO [-] =========================================================
2022-05-13 22:10:25,117 INFO [-] Registered 0 policies.
2022-05-13 22:10:25,117 INFO [-] =========================================================
2022-05-13 22:10:25,117 INFO [-] ############## Registering configs ######################
2022-05-13 22:10:25,118 INFO [-] =========================================================
2022-05-13 22:10:25,158 INFO [-] Registered 4 configs.```

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label May 13, 2022
@CLAassistant
Copy link

CLAassistant commented May 13, 2022

CLA assistant check
All committers have signed the CLA.

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.

Thanks for the contribution! A really clean implementation 👍

Left several comments to address and discussion around the expected interface.

values.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
templates/configmaps_overrides.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
@arm4b arm4b added the feature label May 14, 2022
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

You've only mounted the overrides under the st2client deployment. So we need this inner any other deployments like st2api? Or st2actionrunner?

And I agree that the register-content job is the only default job that needs the overrides. But, could we also add the overrides mount to the extra_hooks jobs?

Plus a few other comments.
Thanks again for working on this

templates/deployments.yaml Show resolved Hide resolved
templates/jobs.yaml Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
templates/_helpers.tpl Outdated Show resolved Hide resolved
templates/_helpers.tpl Outdated Show resolved Hide resolved
templates/deployments.yaml Outdated Show resolved Hide resolved
templates/deployments.yaml Outdated Show resolved Hide resolved
@cwilson21
Copy link
Contributor Author

I don't think that this needs to be in any other deployment likes st2api or st2actionrunner. I say that based on I am only adding this to client so that if a st2ctl reload is ran it does not nuke the overrides that were put in when register-content happened. As far as I see from my testing being in just st2-client is fine, I have a job in my local I was working on previously for our implementation for backing up mongo to gcs and that job has not ran since I enacted the global override on rules to set them to enabled: false . I appreciate being able to work on this, I am fairly new to working with k8s so this was a fun adventure and I will read through your suggestions and correct what was missed and see about implementing the changes to use my helper template.

cwilson21 and others added 3 commits May 14, 2022 22:47
Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
Updating things from override to overrides
@cwilson21
Copy link
Contributor Author

Ah I think I missed a few places I will address them shortly

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Excellent. Thanks for dealing with all my nitpicks.

tests/unit/overrides_test.yaml Outdated Show resolved Hide resolved
tests/unit/overrides_test.yaml Outdated Show resolved Hide resolved
tests/unit/overrides_test.yaml Outdated Show resolved Hide resolved
@cognifloyd cognifloyd enabled auto-merge May 17, 2022 19:20
@cognifloyd
Copy link
Member

Oh! We also need a changelog entry for this. Please and thank you!

@cwilson21
Copy link
Contributor Author

@cognifloyd what version would we call this? 0.100.0 or is this going to be apart of 0.90.0?

@cwilson21
Copy link
Contributor Author

nvm. Just throwing it in the In Development section

auto-merge was automatically disabled May 17, 2022 21:04

Head branch was pushed to by a user without write access

@cognifloyd cognifloyd requested a review from arm4b May 17, 2022 21:12
@cognifloyd
Copy link
Member

@armab wdyt? I think this is good to merge.

@cwilson21
Copy link
Contributor Author

Missed one change request for the trailing slash on the mountpoint and got that updated :)

values.yaml Outdated
# rules:
# defaults:
# enabled: true
# packA.yaml: |
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a wrong indentation here for packA.yaml, compared to _global.yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The indentation should be fixed now.

values.yaml Outdated Show resolved Hide resolved
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.

Left a couple small comments,

besides that LGTM 👍

Updating values.yaml to have the exmaple more consistent iwth others …
@cognifloyd cognifloyd merged commit dceceed into StackStorm:master May 17, 2022
@arm4b
Copy link
Member

arm4b commented May 18, 2022

Nice!
That's a high-quality contribution Thanks, everyone 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants