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 custom packs volumes #199

Merged
merged 23 commits into from
Aug 19, 2021
Merged

Add custom packs volumes #199

merged 23 commits into from
Aug 19, 2021

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Jun 17, 2021

New feature: Shared packs volumes st2.packs.volumes. Allow using cluster-specific persistent volumes to store packs, virtualenvs, and (optionally) configs. This enables using st2 pack install. It even works with st2packs images in st2.packs.images.

For a good description of the thinking behind this PR see: #18 (comment) (and the rest of the comments in that issue)

  • add st2.packs.volumes section to values.yaml
  • consolidate packs-volume-mounts into templates
  • add st2.packs.volumes definitions to packs-volumes templates
  • consolidate pack-configs-volume into templates
  • add pack-configs-volume to more pods when st2.packs.volumes.enabled
  • add error messages if st2.packs.volumes config is incomplete
  • When st2.packs.volumes.enabled, make register-content packs-initContainer include system packs
  • disable packs.configs ConfigMap if it is not needed/used
  • add instructions for st2.packs.volumes values
  • add st2.packs.volumes caveats similar to those documented in Allow mounting of NFS volumes instead of using st2packs for pack management #118
  • allow using st2.packs.volumes with st2.packs.images
  • Allow st2.packs.configs to work with st2.packs.volumes.configs
  • add changelog entry

Related #160

Resolves #18
Closes #118

@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Jun 17, 2021
@ericreeves
Copy link
Contributor

This looks pretty slick and flexible! I may start testing it pretty quickly, and will drop a note if I do!

@cognifloyd cognifloyd requested a review from arm4b June 17, 2021 22:53
@cognifloyd
Copy link
Member Author

K. I think this is ready for review :) I look forward to feedback.

@cognifloyd
Copy link
Member Author

cognifloyd commented Jun 18, 2021

I just reviewed #160, so I started puzzling over if we could also have the chart add the actual persistentVolumeClaims. But, once I read all the discussions in there, I think including the PVCs in the stackstorm-ha chart has some gotchas.

Quoting #160 (comment):

Is it possible for you to use StatefulSet with persistentVolumeTemplate instead?
Using what you've suggested deletes pvc when chart is deleted.
I tried using

annotations:
  helm.sh/resource-policy: keep

& it works fine until we reinstall the chart which fails with:

release stackstorm failed: persistentvolumeclaims "st2-packs-pvol-stackstorm" already exists

Or is there any way we can resuse PV with dynamic provisioning.

Maybe this is another reason to keep the PVC or other storage definition and management separate from the StackStorm chart. I wouldn't want any kind of helm maintenance (install, upgrade, delete, reinstall) to touch that storage, at least in the cluster I'm working on.

Right now I'm preparing to upgrade from a modified 1ppc installation, switching over to the stackstorm-ha chart. I will be reusing mongo, rabbitmq, redis, and my rook-ceph (via flexVolume) installations. So, I'll only be replacing the StackStorm deployments, services (plus migrating some configMaps and secrets to the stackstorm-ha format). I'm trying very hard to make this upgrade as seamless as possible.

That said, I'd be okay if others wanted the chart to optionally also create persistentVolumeClaims, even though I don't need or want them. Any thoughts?

@ericreeves
Copy link
Contributor

I agree with you 100%. Storage for me will be managed outside of the stackstorm-ha chart. I'm using NFS (EFS) right now, but I'm considering switching to the aws-efs-csi driver presenting the EFS volume as a PVC which I will simply tell stackstorm-ha to use. This way, my storage and application tiers have a nice bit of separation, which feels like a clean demarcation point for management and provides greater levels of safety and control.

In the interest of simplicity, my vote is that stackstorm-ha should not create the PVC's, it should simply allow you to reference them as a volume source for the pods that require it. Creating the PVC's opens a giant can of worms and potential complexity that doesn't feel like it should be stackstorm-ha's job to deal with.

@cognifloyd
Copy link
Member Author

rebased on master

@cognifloyd
Copy link
Member Author

Thanks @ericreeves for the fixes in cognifloyd#1 and cognifloyd#2 . I've cherry-picked them in.

Copy link
Contributor

@ericreeves ericreeves left a comment

Choose a reason for hiding this comment

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

I have tested this PR in my cluster and it is working great. I think it's the best proposed solution for the shared storage issue so far.

@cognifloyd
Copy link
Member Author

rebased on master + included nindent reformat changes

@cognifloyd cognifloyd force-pushed the packs-volumes branch 2 times, most recently from 586a6ea to 39be0b0 Compare July 6, 2021 16:55
@ericreeves
Copy link
Contributor

Still working great for me with all of the latest master changes merged in!

@cognifloyd
Copy link
Member Author

I just came across an article that touches in how diverse the k8s storage landscape is.
https://blocksandfiles.com/2021/07/08/29444/

Something generic like this PR will be better for stackstorm-ha in the long run.

@ericreeves
Copy link
Contributor

There have been a couple of attempts at resolving the need for shared storage here, and I believe this is by far the least prescriptive and most elegant.

BYOV. (Bring Your Own Volumes!)

@ericreeves
Copy link
Contributor

Hopefully this gets merged, and then I am working on a way to allow this to peacefully coexist with st2.packs.images by only mounting the packs.images on the st2-register-content Job. This way you can still bake pack images and have them dumped on to the shared storage and registered automatically.

I have a POC of this already functioning in my fork, but want to wait until this gets merged to send a PR as not to muddy the goals of this PR.

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.

@cognifloyd Thanks for the Pull Request! 👍

I'm just gathering feedback from a few folks who were using custom volumes already if this can fit and work for everyone.

It looks optimistic so far.
Also thanks @ericreeves for trying this out already and help!

@cognifloyd
Copy link
Member Author

@armab Sweet! I'm glad this is getting more eyes on it! Thanks for socializing this one!

@cognifloyd
Copy link
Member Author

One issue I just ran into, is rebuilding all the virtualenvs after upgrading to 3.4 is more involved.
I ended up using the st2client pod and running this:

cd /opt/stackstorm/packs
/opt/stackstorm/st2/bin/st2-packs-setup-virtualenv *

I couldn't use st2ctl reload --register-recreate-virtualenvs as described in the docs because that doesn't work with the HA install.

I'm not sure if there's an elegant way to add a conditional upgrade job that runs when going from one st2 version to another.
But, that can be addressed in a separate issue/PR.

@cognifloyd
Copy link
Member Author

Also, in our environment, we use the same pack repos across multiple environments (each env has its own set of volumes).
So, the packs do not have any of the rules enabled by default. We always enable the env-specific rules after updating packs with st2 pack install.
However, every helm upgrade runs st2-register-pack-content which disables all the rules again.

Forcing the installation of packs via st2packs images sidesteps this issue. Now that we're enabling packs volumes, more people will run into this.

The postStart hook works well for the system packs which shouldn't be replaced with st2 pack install, it won't however help in this case since packs are installed after the pods are already up and running.

This is the core issue about this: StackStorm/st2#3973

@chris3081
Copy link

Tested on AKS, we are one day in with light testing, using BYO PVCs and its happy so far. The testing continues.

@cognifloyd
Copy link
Member Author

Thanks @chris3081! I'm glad it's working so far.

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.

Got more positive testing feedback about this feature 👍

@cognifloyd Could you please synchronize the https://docs.stackstorm.com/install/k8s_ha.html with the new stackstorm-ha Readme?

@arm4b arm4b merged commit 3ba85e6 into StackStorm:master Aug 19, 2021
@arm4b
Copy link
Member

arm4b commented Aug 19, 2021

Thanks everyone! 🎉

@cognifloyd
Copy link
Member Author

Cool. I'll work on a docs PR this weekend. Thanks!

@cognifloyd
Copy link
Member Author

Doc changes for this PR were just merged: StackStorm/st2docs#1089

@arm4b
Copy link
Member

arm4b commented Sep 12, 2021

Thanks a lot @cognifloyd !

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.

Sharing pack content: Bundle custom st2packs image vs Shared NFS storage?
4 participants