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

[enterprise-metrics] update ingester podManagementPolicy to Parallel #920

Merged
merged 3 commits into from
Dec 30, 2021

Conversation

replay
Copy link
Contributor

@replay replay commented Dec 27, 2021

This is a request for feedback, I'm curious if others in the team agree that the pros outweigh the cons.

I think we should update the ingester podManagementPolicy to Parallel.

Pros:

Cons:

  • The upgrade procedure from before this change to after this change is rather complicated. It will require that the SFS is deleted with --cascade=false and then recreated. This is risky, because if a customer fails to specify --cascade=false their write path will go down.

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay replay force-pushed the ingester_pod_management_policy_parallel branch from 76d356c to e8504af Compare December 27, 2021 15:10
@replay replay requested review from vlad-diachenko and a team December 27, 2021 15:36
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Change LGTM, but keep in mind you can't change the pod management policy on a statefulset. You have to recreate it, which may be very annoying for customers!

vlad-diachenko
vlad-diachenko previously approved these changes Dec 27, 2021
Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

Lgtm

@replay replay changed the title update ingester podManagementPolicy to Parallel [enterprise-metrics] update ingester podManagementPolicy to Parallel Dec 27, 2021
@replay
Copy link
Contributor Author

replay commented Dec 27, 2021

keep in mind you can't change the pod management policy on a statefulset

Thx for reviewing. Yep, that's the only "con" which I can see with this change, but I guess we'll just have to make sure that our support and solution engineering teams are aware.

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay replay force-pushed the ingester_pod_management_policy_parallel branch from 48b21b2 to 5eff64a Compare December 28, 2021 21:24
@replay
Copy link
Contributor Author

replay commented Dec 28, 2021

I had to force-push because I forgot to DCO sign a commit, apparently this has invalidated the approvals which this PR already had (at least I think this is why they are now not counted anymore).

@replay replay merged commit 168fdb3 into main Dec 30, 2021
@replay replay deleted the ingester_pod_management_policy_parallel branch December 30, 2021 12:46
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

4 participants