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

*: Refactor to not mutate global objects #90

Merged
merged 2 commits into from
Feb 6, 2020
Merged

*: Refactor to not mutate global objects #90

merged 2 commits into from
Feb 6, 2020

Conversation

brancz
Copy link
Member

@brancz brancz commented Jan 31, 2020

This patch introduces a few changes:

  • Move all manifests to local configurations and non-merged local interactions. Which slightly changes the config paradigm as things have to be more declarative than modifying potentially existing "global" state.
  • Makes the minio dev example work in any namespace
  • Adds more declarative app labels defined by Kubernetes. This may cause users to have to remove and re-create manifests. We should carefully review this and see if we want any other label for this as we shouldn't change this again.
  • Adds optional PodDisruptionBudget for Thanos receive.
  • Moves ServiceMonitors to optional patches under each component .withServiceMonitor.

@metalmatze @squat @bwplotka @kakkoyun

Having reviewed this myself a few times, I recommend reading the generated manifests first, and then reviewing the new jsonnet files in their entity instead of comparing to the previous one.

@brancz brancz force-pushed the refactor branch 4 times, most recently from 82f6abe to 3d5f095 Compare January 31, 2020 16:23
Copy link
Member

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

Skimming over it on mobile made me very excited. I'll give it another look on a computer after FOSDEM. 👍

@brancz
Copy link
Member Author

brancz commented Jan 31, 2020

Sounds good. I think I just had a better idea for persistent volume claim templates anyways :)

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

This looks great.

  1. I think we should fix naming of the components, as it's discussed in Move components names to Ruler, Compactor, Querier (-er) thanos#1871.

receive should be receiver
query should be querier

  1. You also removed sidecar. Which is ok, I think. We didn't have anything significant in that file. However, it would be good to document this. And maybe we can add a helper function to add sidecars to queriers. Just for convenience. This could be handled in another PR, I just wanted to note it.

jsonnet/kube-thanos/thanos.libsonnet Show resolved Hide resolved
jsonnet/kube-thanos/kube-thanos-querier.libsonnet Outdated Show resolved Hide resolved
all.jsonnet Outdated Show resolved Hide resolved
all.jsonnet Outdated Show resolved Hide resolved
example.jsonnet Outdated Show resolved Hide resolved
@kakkoyun
Copy link
Member

kakkoyun commented Feb 3, 2020

@brancz My only concern about naming is inconsistency. I'm totally fine with keeping configuration names in sync with commands. In that case, we should rename querier to query and compactor to compact.

My understanding from that issue (thanos-io/thanos#1871) was, we would eventually move to *tor naming schema, thus my comment.

And it's more clear now. Just to recap we will keep structural things as in commands and only rename documentation, alert message etc.

@brancz
Copy link
Member Author

brancz commented Feb 3, 2020

Looks like we have consensus in thanos-io/thanos#1871, I'll do the changes accordingly here.

@brancz brancz force-pushed the refactor branch 10 times, most recently from 1a1e5e1 to 9c6a9ad Compare February 5, 2020 15:05
README.md Outdated Show resolved Hide resolved
@brancz brancz force-pushed the refactor branch 2 times, most recently from 2b155ff to 3fe7ba5 Compare February 5, 2020 15:38
Signed-off-by: Frederic Branczyk <fbranczyk@gmail.com>
@metalmatze
Copy link
Member

Here's my the progress on moving my personal Thanos cluster to this refactored kube-thanos version.
I also put the diff to my thanos.yaml in there. A few things still missing but overall, this is an amazing improvement!

https://gist.github.com/metalmatze/7280c8101024d0edbccd5fbedb82a2c1

Signed-off-by: Frederic Branczyk <fbranczyk@gmail.com>
Copy link
Member

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

Let's do follow ups, if necessary, in new PRs! :)
Thaaaaaaaaanks soo much for this! 🎉

@metalmatze metalmatze merged commit 7816a2b into master Feb 6, 2020
@metalmatze metalmatze deleted the refactor branch February 6, 2020 18:07
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