feat(sentrykube): introduce additional possibilities to override config #63
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: libsentrykube/kube.py
Did you find this useful? React with a 👍 or 👎 |
…des' into martinl/additional-config-overrides
for more information, see https://pre-commit.ci
…des' into martinl/additional-config-overrides # Conflicts: # libsentrykube/tests/test_service.py
for more information, see https://pre-commit.ci
…des' into martinl/additional-config-overrides # Conflicts: # libsentrykube/tests/test_service.py
for more information, see https://pre-commit.ci
libsentrykube/kube.py
Outdated
| 5. overridden by creating a hierarchical structure. Adding an intermediate directory | ||
| in 'region_override' with a '_values.yaml' file allows to have a common config | ||
| between a set of regions. This is useful if regions in a service are different, but | ||
| subset of them are similar. | ||
| It works like point 1 and 2 but with an additional layer in between | ||
| 6. overridden by a common '_values.yaml' within a region folder that has a shared config | ||
| between all clusters in the region. Values in point 2 will still override those values |
There was a problem hiding this comment.
I don't think the order is right.
- Cluster file will be deprecated for service specific override, we are moving to region override only. It still exists because we are in the middle of this process. But if a service has region overrides there should be nothing in the cluster file for that service.
- managed override (point 3 above) has to be the last override file (assuming the cluster file goes away).
- It seems that the common
_values.yamlfile within a region (point 6) should be before point 2 (region override). If it is shaerd between clusetrs in the same region, the cluster specific file (these - https://github.com/getsentry/ops/tree/master/k8s/services/relay-pop/region_overrides/us) should be more specific than you point 6. - your point 5 should be before 2 as well.
In summary This should be the order from more generic to more specific
1 The per service value file (this is not region or cluster specific. It is service specific)
5 common override between subset of regions of the same service
6 common override between clusters in the same region (more specific than 5)
3 cluster specific override
4 managed override, which is basically equivalent to 3 but it is a separate file as it is managed by tools and cannot contain comments.
4 the cluster file is still there for historic reasons, but if you made changes to any of the above the cluster file is supposed to be empty for the specific service.
There was a problem hiding this comment.
Thanks for the detailed comment. You're right that the order isn't correct, I misunderstood the existing comment and thought it's more of a "we have this kind of overrides" instead of them being the order from most to least specific. I will update it accordingly
|
|
||
| # Try to load the config only after {cluster_name}.yaml was successfully loaded because the common override | ||
| # only makes sense if there is a cluster config | ||
| common_override_values = get_common_regional_override( | ||
| service_name, region_name, external | ||
| ) | ||
|
|
||
| if common_override_values: | ||
| deep_merge_dict(common_override_values, values) | ||
| return common_override_values | ||
|
|
||
| return values | ||
| except FileNotFoundError: | ||
| values = {} | ||
| return values | ||
| return {} | ||
|
|
There was a problem hiding this comment.
Each of these methods (get_service_values, get_service_value_overrides, etc.) loads a level of overrides only. Then these are layered in the right order in _consolidate_variables. This is intentional so that we separate the logic that loads one level of override from the one that defines the order and make the order apparent.
Please do not merge different levels of overrides in the same method and create a new one instead. The merging should only happen in _consolidate_variables only. If you find a lot of duplication feel free to factor that logic out.
There was a problem hiding this comment.
Thanks for the review, those are valid points. I will split it up and move the logic
| Directory Structure: | ||
| region_overrides/ | ||
| └── common_shared_config/ # Arbitrary name for the shared config group | ||
| ├── _values.yaml # Base values for this group | ||
| └── {region_name}/ # Region-specific overrides | ||
| └── {cluster_name}.yaml # Cluster-specific overrides |
There was a problem hiding this comment.
I am not sure I follow the reasoning here.
- Which scenario do you want to use the hierarchical value overrides in? Is it about single tenant in order to have some common config for all single tenants vs config common to all saas regions? While I hate the idea that ST should be different I guess that's the world we live into. But if this is not targeting that scenario, could you please elaborate ?
- If you want to provide a value file that is common to a group of regions why do you have a cluster file here as well? I think in that case the standard cluster specific file (
/region_override/my_cluster.yaml) should apply rather than having a separate one. In other word I would like this cross regions defaults to apply within the same hierarchy structure we use for all the other services rather than having two parallel hierarchies.
There was a problem hiding this comment.
Which scenario do you want to use the hierarchical value overrides in? Is it about single tenant in order to have some common config for all single tenants vs config common to all saas regions? While I hate the idea that ST should be different I guess that's the world we live into. But if this is not targeting that scenario, could you please elaborate ?
Yes it's for single tenants. I agree that it's not great that the config diverges
If you want to provide a value file that is common to a group of regions why do you have a cluster file here as well? I think in that case the standard cluster specific file (/region_override/my_cluster.yaml) should apply rather than having a separate one. In other word I would like this cross regions defaults to apply within the same hierarchy structure we use for all the other services rather than having two parallel hierarchies.
Consider the following structure:
region_overrides/
├── customer1/
├── customer2/
├── customer3/
├── customer4/
└── customer5/
customer1 and customer2 have similar config and customer3, customer4 and customer5 have also similar config with enough differences that they can't share a common config with customer1 and customer2.
As far as I know there is no support for this type of structure which results in a lot of copied config in either the first two or the last 3 customer configs.
Even if we would use /region_override/my_cluster.yaml, we can still only share common configs for either of those groups and the other one needs to overwrite it per cluster file.
With my changes we could split it into the following structure where each group can have a single file with a common config:
region_overrides/
├── first-group/
│ ├── customer1/
│ └── customer2/
└── second-group/
├── customer3/
├── customer4/
└── customer5/
Additionally to reducing code duplication we would also see differences between customers more easily.
This PR adds two possibilities to share common config between cluster configs.
region_overridesandregion_namewith arbitrary names. This directory can contain a_values.yamlfile with config values where all values fromregion_name/cluster.yamlare merged into._values.yamlfile intoregion_overrides/region_namewhich works conceptually the same as the hierarchical sharing but it makes more sense if many cluster files exist in a region