Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions libsentrykube/helm.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ def inner(*args, **kwargs):
return inner


def _consolidate_ctx(region_name, service_name, cluster_name, src_file="_values.yaml"):
def _consolidate_ctx(
region_name, service_name, cluster_name, src_files_prefix="_values"
):
from libsentrykube.service import (
get_service_ctx,
get_service_ctx_overrides,
Expand All @@ -109,33 +111,37 @@ def _consolidate_ctx(region_name, service_name, cluster_name, src_file="_values.
assert_customer_is_defined_at_most_once,
)

if src_file == "_values.yaml":
if src_files_prefix == "_values":
Comment thread
mwarkentin marked this conversation as resolved.
assert_customer_is_defined_at_most_once(
service_name, region_name, namespace="helm"
)

ctx = get_service_ctx(service_name, namespace="helm", src_file=src_file)
ctx = get_service_ctx(service_name, namespace="helm")
ctx_overrides = get_service_ctx_overrides(
service_name,
region_name,
cluster_name,
namespace="helm",
src_file=src_file,
src_files_prefix=src_files_prefix,
cluster_as_folder=True,
)
ctx_common = get_common_regional_override(
service_name, region_name, namespace="helm", src_file=src_file
service_name, region_name, namespace="helm", src_files_prefix=src_files_prefix
)
if ctx_overrides or ctx_common:
deep_merge_dict(ctx, ctx_common)
deep_merge_dict(ctx, ctx_overrides)
else:
ctx_hierarchical = get_hierarchical_value_overrides(
service_name, region_name, cluster_name, namespace="helm", src_file=src_file
service_name,
region_name,
cluster_name,
namespace="helm",
src_files_prefix=src_files_prefix,
)
deep_merge_dict(ctx, ctx_hierarchical)
# get_tools_managed_service_value_overrides?
if src_file == "_values.yaml":
if src_files_prefix == "_values":
ctx_region, _ = get_helm_service_data(region_name, service_name, cluster_name)
deep_merge_dict(ctx, ctx_region)
return ctx
Expand All @@ -144,7 +150,7 @@ def _consolidate_ctx(region_name, service_name, cluster_name, src_file="_values.
def _helm_chart_ctx(region_name, service_name, cluster_name) -> list[HelmRelease]:
rv = []
ctx = _consolidate_ctx(
region_name, service_name, cluster_name, src_file="_helm.yaml"
region_name, service_name, cluster_name, src_files_prefix="_helm"
)
chart_spec = ctx.get("chart", {})
if isinstance(chart_spec, str):
Expand Down
14 changes: 14 additions & 0 deletions libsentrykube/kube.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,20 @@ def _consolidate_variables(
preserve comments.
6. overridden by the cluster file. Which is likely going to be replaced by 2 and 3.

***
For all levels of overrides listed above, in the same directory of file system, we support
merging separete values files together, before proceding with the overriding logic
Comment thread
victoria-yining-huang marked this conversation as resolved.
Most common examples (numbers refer to override level listed above):
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. `k8s/services/seer/region_overrides/de/default.yaml` content will be combined with `k8s/services/seer/region_overrides/de/default_2.yaml`
Files that should be combined have constraints:
- files cannot have conflicting keys. It will fail.
- files must be in the same directory in the file system
- file names must start with `_values` to be combined with `_values.yaml` file, or must start with `<cluster>` to be combined with `<cluster>.yaml`


TODO: write the minimum components of a yaml parser to remove step 3 and
patch the regional override preserving comments.
"""
Expand Down
137 changes: 89 additions & 48 deletions libsentrykube/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,30 +95,60 @@ def get_service_names(namespace: str | None = None) -> List[str]:
return [s for s in _services.get(namespace, {}).keys()]


def merge_values_files_no_conflict(base: dict, new: dict, file_name: str) -> dict:
"""
All values files in each level of overriding should be flatly combined together.
There should be no key conflics
Example (these two files should have no key conflicts):
_values.yaml
workers:
rabbit-worker-1:
data

_values_consumer.yaml
consumer_groups:
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.

if conflict_keys:
raise ValueError(
f"Conflict detected when merging file '{file_name}': duplicate keys {conflict_keys}"
)
base.update(new)
return base


def get_service_ctx(
service_name: str,
external: bool = False,
namespace: str | None = None,
src_file: str = "_values.yaml",
) -> dict:
"""
For the given service, return the values specified in the corresponding {src_file}.
For the given service, return the combined values from all _values*.yaml files in the service directory.

Raises an error if duplicate keys are found across files.

If "external=True" is specified, treat the service name as the full service path.
"""
if external:
service_path = workspace_root() / service_name
service_path_root = workspace_root() / service_name
else:
service_path = get_service_path(service_name, namespace=namespace)
try:
with open(service_path / src_file, "rb") as f:
return yaml.safe_load(f) or {}
except FileNotFoundError:
return {}
service_path_root = get_service_path(service_name, namespace=namespace)

ctx: dict[str, dict[str, Any]] = {}
for file in service_path_root.glob("_values*.yaml"):
try:
with open(file, "rb") as f:
values = yaml.safe_load(f) or {}
ctx = merge_values_files_no_conflict(ctx, values, file.name)
except FileNotFoundError:
continue
return ctx


def get_service_values(service_name: str, external: bool = False) -> dict:
return get_service_ctx(service_name, external=external, src_file="_values.yaml")
return get_service_ctx(service_name, external=external)


def get_service_value_override_path(
Expand Down Expand Up @@ -151,29 +181,34 @@ def get_service_ctx_overrides(
cluster_name: str = "default",
external: bool = False,
namespace: str | None = None,
src_file: str = "_values.yaml",
src_files_prefix: str = "_values",
cluster_as_folder: bool = False,
) -> dict:
"""
For the given service, return the values specified in the corresponding _values.yaml.
If "external=True" is specified, treat the service name as the full service path.
"""
try:
override_path = get_service_value_override_path(
service_name, region_name, external, namespace=namespace
)
service_override_file = (
override_path / cluster_name / src_file
if cluster_as_folder
else override_path / f"{cluster_name}.yaml"
)

with open(service_override_file, "rb") as f:
values = yaml.safe_load(f) or {}

return values
except FileNotFoundError:
return {}
ctx: dict[str, dict[str, Any]] = {}
override_path = get_service_value_override_path(
service_name, region_name, external, namespace=namespace
)
service_override_file_root = (
override_path / cluster_name if cluster_as_folder else override_path
)
keyword_to_merge_files = (
f"{src_files_prefix}*.yaml" if cluster_as_folder else f"{cluster_name}*.yaml"
)
for file in service_override_file_root.glob(keyword_to_merge_files):
try:
if file.name.endswith("managed.yaml"):
continue
with open(file, "rb") as f:
values = yaml.safe_load(f) or {}
ctx = merge_values_files_no_conflict(ctx, values, file.name)
except FileNotFoundError:
raise
return ctx


def get_service_value_overrides(
Expand All @@ -192,26 +227,26 @@ def get_common_regional_override(
region_name: str,
external: bool = False,
namespace: str | None = None,
src_file: str = "_values.yaml",
src_files_prefix: str = "_values",
) -> dict:
"""
Helper function to load common regional configuration values.

Looks for a '_values.yaml' file in the region's override directory that contains
settings shared across all clusters in that region.
"""
try:
common_service_override_file = (
get_service_value_override_path(
service_name, region_name, external, namespace=namespace
)
/ src_file
)

with open(common_service_override_file, "rb") as f:
return yaml.safe_load(f) or {}
except FileNotFoundError:
return {}
common_service_override_file_root = get_service_value_override_path(
service_name, region_name, external, namespace=namespace
)
ctx: dict[str, dict[str, Any]] = {}
for file in common_service_override_file_root.glob(f"{src_files_prefix}*.yaml"):
try:
with open(file, "rb") as f:
values = yaml.safe_load(f) or {}
ctx = merge_values_files_no_conflict(ctx, values, file.name)
except FileNotFoundError:
continue
return ctx


def get_hierarchical_value_overrides(
Expand All @@ -220,7 +255,7 @@ def get_hierarchical_value_overrides(
cluster_name: str = "default",
external: bool = False,
namespace: str | None = None,
src_file: str = "_values.yaml",
src_files_prefix: str = "_values",
) -> dict:
"""
Enables hierarchical configuration overrides with shared base values.
Expand Down Expand Up @@ -255,13 +290,15 @@ def get_hierarchical_value_overrides(
if not override_group.is_dir():
continue

service_override_file_root = service_regions_path / override_group.name
base_values: dict[str, dict[str, Any]] = {}
try:
service_override_file = (
service_regions_path / override_group.name / src_file
)

with open(service_override_file, "rb") as f:
base_values = yaml.safe_load(f) or {}
for file in service_override_file_root.glob(f"{src_files_prefix}*.yaml"):
with open(file, "rb") as f:
values = yaml.safe_load(f) or {}
base_values = merge_values_files_no_conflict(
base_values, values, file.name
)
except FileNotFoundError:
base_values = {}

Expand All @@ -275,12 +312,16 @@ def get_hierarchical_value_overrides(
cluster_name,
external,
namespace=namespace,
src_file=src_file,
src_files_prefix=src_files_prefix,
cluster_as_folder=namespace == "helm",
)

common_service_values = get_common_regional_override(
service_name, region_path, external, namespace=namespace, src_file=src_file
service_name,
region_path,
external,
namespace=namespace,
src_files_prefix=src_files_prefix,
)

# There must be either a cluster specific override file a _values.yaml in the region dir
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
consumer_key1: value1
consumer_key2:
subkey2_1: value2_1
subkey2_2: 2
subkey2_3:
- value2_3_1
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,8 @@ key2:
- value2_4_1_managed_replaced
subkey2_5:
- value2_5_1_managed_replaced
consumer_key2:
subkey2_4:
- value2_4_1_managed_replaced
subkey2_5:
- value2_5_1_managed_replaced
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
consumer_key2:
subkey2_3:
- value2_3_1_replaced
subkey2_4:
- value2_4_1_replaced
12 changes: 12 additions & 0 deletions libsentrykube/tests/test_kube.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@
"value2_5_1_managed_replaced"
], # From the managed file
},
"consumer_key1": "value1",
"consumer_key2": {
"subkey2_1": "value2_1", # From the value file
"subkey2_2": 2, # From the value file
"subkey2_3": ["value2_3_1_replaced"], # From the region override
"subkey2_4": [
"value2_4_1_managed_replaced"
], # From the managed file
"subkey2_5": [
"value2_5_1_managed_replaced"
], # From the managed file
},
},
"service2": {
"key3": "three", # From the cluster file
Expand Down
Loading