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

let rabbitmq operator to control images #1181

Merged

Conversation

farodin91
Copy link
Contributor

Part of #1003

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

Add experimental ENV to let rabbitmq operator update images. No safety checks at the moment.

Additional Context

Local Testing

Please ensure you run the unit, integration and system tests before approving the PR.

To run the unit and integration tests:

$ make unit-tests integration-tests

You will need to target a k8s cluster and have the operator deployed for running the system tests.

For example, for a Kubernetes context named dev-bunny:

$ kubectx dev-bunny
$ make destroy deploy-dev
# wait for operator to be deployed
$ make system-tests

@DanielePalaia
Copy link
Contributor

Hi @farodin91 Can you explain to us more about your use case and how this functionality can help you?
Maybe opening a new issue and linking it to this PR will be nice too.

In general we are not against it but using it improperly can cause some unwanted consequences like a rolling restart of all the rabbitmq clusters whenever an operator upgrade happens.

I think the scope of this new variable should be well documented at least in the code if not maybe even in our doc.

@johanneswuerbach
Copy link

@DanielePalaia we would also be interested in this functionality mainly to reduce operational burden and only keep the operator up-to-date instead of maintaining the operator and cluster(s).

rolling restart of all the rabbitmq clusters whenever an operator upgrade happens

We generally have a 1:1 mapping, so for us that would be fine. An option might be to generally limit how many cluster are rolled in parallel, would this be sufficient?

@farodin91
Copy link
Contributor Author

farodin91 commented Dec 2, 2022

@DanielePalaia I will try to write down a proposal.

This should be just the first step.
Ideas:

  1. adding checks to update only proper version.
  2. a map of support version with the newest patch level to support upgrade of multiple versions
  3. rate limit cluster upgrade speed.

First of all, I will try to add a small documentation.

@DanielePalaia
Copy link
Contributor

Hi @johanneswuerbach @farodin91 yes thank you, I think this should be better documented on the source code as well as maybe describe this future as experimental somewhere in our doc and describe the purpose of it. As first step I suggest anyway to open an issue and link this PR to it so we can discuss it there.

@farodin91 farodin91 force-pushed the let-rabbitmq-operator-to-control-images branch from e4c3ebe to f5cf5c3 Compare December 30, 2022 14:29
Part of rabbitmq#1003 & rabbitmq#1219

Signed-off-by: Jan Jansen <jan.jansen@gdata.de>
@farodin91 farodin91 force-pushed the let-rabbitmq-operator-to-control-images branch from f5cf5c3 to 85eb9c6 Compare December 30, 2022 14:33
@farodin91
Copy link
Contributor Author

@DanielePalaia the update to documentation have to be done in a different repo, i guess?

@DanielePalaia
Copy link
Contributor

Hi @farodin91 we have this page to document our environment variables: https://www.rabbitmq.com/kubernetes/operator/configure-operator-defaults.html#parameters.

But as this is just a first experimental approach just useful for "testing" purpose, maybe we can document it when the functionality will be fully ready (at least to provide a version validity check).

Anyway if you don't have other modifications to be done in this PR I will integrate it.

@farodin91
Copy link
Contributor Author

@DanielePalaia Should I open an PR to the website repository before this get merged?

@DanielePalaia
Copy link
Contributor

Hi @farodin91 No if you are done here I can merge this one now.

@farodin91
Copy link
Contributor Author

We are done here.

@DanielePalaia DanielePalaia merged commit 61e0b56 into rabbitmq:main Jan 10, 2023
@DanielePalaia
Copy link
Contributor

Thanks @farodin91 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants