Use standard collector selectors in target allocator config #2466
Conversation
swiatekm
force-pushed
the
feat/targetallocator-selector
branch
2 times, most recently
from
f50e03e to
6c8c7a7
Compare
| # (Optional) One or more lines of additional information to render under the primary note. | ||
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: This is a breaking change only for users of standalone target allocator. Operator users are unaffected. |
cmd/otel-allocator/config/config.go
Outdated
| KubeConfigFilePath string `yaml:"kube_config_file_path,omitempty"` | ||
| ClusterConfig *rest.Config `yaml:"-"` | ||
| RootLogger logr.Logger `yaml:"-"` | ||
| CollectorSelector metav1.LabelSelector `yaml:"collector_selector,omitempty"` |
There was a problem hiding this comment.
should this be reference? or should we remove omitempty? I'm not sure the TA can work without the collector selector.
There was a problem hiding this comment.
It should be, good catch. The semantics of v1.LabelSelector, as per the documentation, are:
A label selector is a label query over a set of resources. The result of matchLabels and matchExpressions are ANDed. An empty label selector matches all objects. A null label selector matches no objects.
So the target allocator should in fact work without a selector, it'll behave the same way as if there were no collectors to assign targets to.
| taConfig["label_selector"] = manifestutils.SelectorLabels(params.OtelCol.ObjectMeta, collector.ComponentOpenTelemetryCollector) | ||
| taConfig["collector_selector"] = map[string]any{ | ||
| "matchlabels": manifestutils.SelectorLabels(params.OtelCol.ObjectMeta, collector.ComponentOpenTelemetryCollector), | ||
| } |
There was a problem hiding this comment.
one issue that may arise with this change is that users cannot use the newer operator with an older TA version. This could be problematic in this scenario:
- a company runs a mirrored image of the TA due to a security requirement for the image in the collector CRD
- engineer bumps the operator version which no longer writes the former selector
- TA can no longer startup until they re-mirror and bump the TA image
I think for now it may make sense to just mark the label_selector as deprecated in the operator and will be removed entirely in v1alpha2. It is totally fine, however, to just remove it entirely from the TA.
There was a problem hiding this comment.
Do we support running different versions of the operator and targetallocator? I know we don't for the collector, even though things usually work anyway.
How would you mark the label_selector as deprecated? From the operator's perspective, it's just a key in an untyped map.
There was a problem hiding this comment.
I would just mark it in a comment with a TODO linking to an open issue, this could also be a featuregate that starts enabled by default and then we eventually just delete it?
I'm not sure we've discussed what the operators own compat matrix looks like, but the scenario I linked above could break a lot of users because you cannot usually update the TA image and the operator image in the same release. Doing either one without the other would result in a broken state.
There was a problem hiding this comment.
A problem with this is that target allocator currently unmarshals its configuration in strict mode, so the old version can't deal with the new value either way. The way I see it, there's two possible approaches:
- Do an explicit version check and apply the right value.
- Merge a change that makes config unmarshalling non-strict, wait till that's released, and then merge this PR.
I think I like 2 better, WDYT?
There was a problem hiding this comment.
argh 🤦 I think i like 2 more as well. Lets get that merged in before #2482
swiatekm
force-pushed
the
feat/targetallocator-selector
branch
2 times, most recently
from
c8be097 to
a1b0dd9
Compare
swiatekm
force-pushed
the
feat/targetallocator-selector
branch
from
a1b0dd9 to
0db3028
Compare
|
@swiatekm-sumo i think we're good to merge this now that #2488 has been merged right? In the changelog can you call out the potential incompatability? |
This is to keep backwards compatibility with older target allocator versions, which makes upgrades easier.
swiatekm
force-pushed
the
feat/targetallocator-selector
branch
from
0db3028 to
803b7f2
Compare
I made the backwards compatibility guarantee explicit. |
…emetry#2466) * Use standard collector selectors in target allocator config * Use both collector selector formats in ta config This is to keep backwards compatibility with older target allocator versions, which makes upgrades easier.
Description:
Make the label selector for collectors in target allocator configuration use the K8s standard format. We can later explicitly expose this in the TargetAllocator CRD.
One annoying aspect of using the core struct is that it only has tags for json serialization, and the yaml marshaler just lowercases the name. So we end up with
matchlabelsrather thanmatchLabelsormatch_labels. I'm not sure the additional complexity of using a custom struct and then converting is worth it, given that target allocator is almost exclusively used and configured by the operator.Link to tracking Issue: #2422
Resolves #2422
Testing:
Modified existing tests.