Skip to content

feat(sentry-kube): support combining files with keyword#114

Merged
victoria-yining-huang merged 2 commits into
mainfrom
vic/combine_all_values_files
May 9, 2025
Merged

feat(sentry-kube): support combining files with keyword#114
victoria-yining-huang merged 2 commits into
mainfrom
vic/combine_all_values_files

Conversation

@victoria-yining-huang

@victoria-yining-huang victoria-yining-huang commented Apr 29, 2025

Copy link
Copy Markdown
Contributor

Context:
streaming team needs github CODEOWNERS to consumer values so they can merge changes without depending on SRE aproval

PR explained:
for every level of overriding logic, find all files that start with the keyword (usually it will be the default value of _values) and combine them together, before proceeding with the original overriding logic

Impacts:
for services that only has one _values.yaml file, this changes nothing

Rollout plan:
after this PR is merged, cut a new release, update sentrykubelib version in ops, then make a new _consumer_values.yaml file in getsentry service, in all overriding levels

@victoria-yining-huang victoria-yining-huang force-pushed the vic/combine_all_values_files branch 5 times, most recently from 76e5eeb to 4c2d2df Compare April 29, 2025 20:50
@victoria-yining-huang victoria-yining-huang marked this pull request as ready for review April 29, 2025 21:10
@victoria-yining-huang victoria-yining-huang requested a review from a team as a code owner April 29, 2025 21:10

@fpacifici fpacifici left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please see my comment inline. There are quite a few more cases to adjust.

Comment thread libsentrykube/service.py Outdated
@victoria-yining-huang victoria-yining-huang force-pushed the vic/combine_all_values_files branch 11 times, most recently from 29a6940 to d55e067 Compare May 5, 2025 16:37
@victoria-yining-huang victoria-yining-huang changed the title ref(sk): combine all _values.yaml files feat(sentry-kube): support combing files with keyword May 5, 2025
@victoria-yining-huang victoria-yining-huang force-pushed the vic/combine_all_values_files branch from 1702795 to 3a7503d Compare May 5, 2025 18:28
@mwarkentin mwarkentin changed the title feat(sentry-kube): support combing files with keyword feat(sentry-kube): support combining files with keyword May 6, 2025
Comment thread libsentrykube/helm.py
@mwarkentin

Copy link
Copy Markdown
Member

Not sure where, but it would be nice to have some documentation about how our values merging works, maybe in https://github.com/getsentry/sentry-infra-tools/tree/main/libsentrykube/README.md?

Do we provide any sort of ordering logic? Is it purely alphabetical? What happens in case of a conflict?

Comment thread libsentrykube/service.py Outdated
Comment thread libsentrykube/helm.py
@rgibert

rgibert commented May 6, 2025

Copy link
Copy Markdown
Member

Not sure where, but it would be nice to have some documentation about how our values merging works, maybe in https://github.com/getsentry/sentry-infra-tools/tree/main/libsentrykube/README.md?

Do we provide any sort of ordering logic? Is it purely alphabetical? What happens in case of a conflict?

merge_values_files_no_conflict() should enforce no conflicts.

@victoria-yining-huang

Copy link
Copy Markdown
Contributor Author

@mwarkentin in terms of documentation, I was pointed to this block of comments

We have multiple levels of overrides for our value files.

Not sure if there exists another doc like this

@mwarkentin

Copy link
Copy Markdown
Member

Ok, at a minimum we should be updating that block with the new functionality, however it does feel like we should have something a little more discoverable / usable for introducing people to how sentry-kube works (values generation being a big part).

I don't have a strong opinion but either in a sentry-infra-tools readme or docs/ dir, or a notion page.

Comment thread libsentrykube/kube.py
Comment thread libsentrykube/kube.py Outdated
1. `k8s/services/getsentry/_values.yaml` content will be combined with `k8s/services/getsentry/_values_consumers.yaml`
3. `k8s/services/getsentry/regional_overrides/s4s/_values.yaml` content will be combined with
`k8s/services/getsentry/regional_overrides/s4s/_values_consumers.yaml`
4. `sk8s/services/seer/region_overrides/de/default.yaml` content will be combined with ``sk8s/services/seer/region_overrides/de/default_2.yaml`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typos

Suggested change
4. `sk8s/services/seer/region_overrides/de/default.yaml` content will be combined with ``sk8s/services/seer/region_overrides/de/default_2.yaml`
4. `k8s/services/seer/region_overrides/de/default.yaml` content will be combined with `k8s/services/seer/region_overrides/de/default_2.yaml`

Is there a reason we use a default_2 example instead of a default_consumers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

just because i don't think the seer service would have a consumers file, i was trying to give real examples. Not sure what example to give here.

Comment thread libsentrykube/kube.py Outdated
Comment thread libsentrykube/service.py
consumer-1:
data
"""
conflict_keys = base.keys() & new.keys()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not enough.
Let's say the issues tame takes over the ingest consumers and the subscription results consumers, it would not be possible to give them access to some specific consumers as we would have to break the value file according to a subkey of consumer_groups.

So you will need to allow some level of overlap. You could allow the same key to be defined in multiple places if it is a collection (like a map) as long as there is no conflict between the subkeys. The result of the merge would be a merge of the collections rather than a substitution.

@victoria-yining-huang victoria-yining-huang May 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's say the issues tame takes over the ingest consumers

is that really happening soon that we should build the support now? I think it's a good idea but I was only under the assumption that streaming team would own all consumers for the next little while

@fpacifici fpacifici May 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is that really happening soon

Yes it is happening as part of the bearhug for celery workers and hopefully for consumers.
Actually this is already supposed to happen now. The streaming platform is not going to take over the metrics indexer (sns) and it is a getsentry consumer.

Comment thread libsentrykube/service.py Outdated
@victoria-yining-huang victoria-yining-huang force-pushed the vic/combine_all_values_files branch 2 times, most recently from 8853b35 to 46ba7df Compare May 7, 2025 18:32
rename

add

added regional override

lint

revert common regional override

regional override fixed

add all tests pass

add new test file

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

add ghost file

revert changes that shouldnt have been added

revert change that shouldnt have been added

fix funciton args

fix arg

every step has combining abilities

sync args

rename arg

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Update libsentrykube/service.py

Co-authored-by: Richard Gibert <richard@gibert.ca>

comments

manged is suffix

get back what i lost
@victoria-yining-huang victoria-yining-huang force-pushed the vic/combine_all_values_files branch from 27f1891 to 01d644f Compare May 8, 2025 16:42
@victoria-yining-huang victoria-yining-huang merged commit f45737b into main May 9, 2025
7 checks passed
@victoria-yining-huang victoria-yining-huang deleted the vic/combine_all_values_files branch May 9, 2025 16:37
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.

4 participants