Add version label to target allocator resources #2455
Conversation
swiatekm
force-pushed
the
chore/targetallocator-labels
branch
3 times, most recently
from
b8bf5b6
to
10352fe
Compare
| if _, ok := base["app.kubernetes.io/name"]; !ok { | ||
| base["app.kubernetes.io/name"] = name | ||
| } | ||
| return manifestutils.Labels(instance.ObjectMeta, name, instance.Spec.TargetAllocator.Image, ComponentOpenTelemetryTargetAllocator, nil) |
| @@ -44,7 +44,7 @@ func Deployment(params manifests.Params) (*appsv1.Deployment, error) { | |||
| Spec: appsv1.DeploymentSpec{ | |||
| Replicas: params.OtelCol.Spec.TargetAllocator.Replicas, | |||
| Selector: &metav1.LabelSelector{ | |||
| MatchLabels: labels, | |||
| MatchLabels: SelectorLabels(params.OtelCol), | |||
There was a problem hiding this comment.
Isn't this immutable field? https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#label-selector-updates
Can you confirm that this will not break during the upgrade?
There was a problem hiding this comment.
I'll test it. If there's a problem with that, then it's probably for the best to leave the selector as is.
There was a problem hiding this comment.
@pavolloffay we do have logic in our reconciler to just delete and recreate when it sees this immutable change, so it should just work (with a small blip for the reconciler logic)
There was a problem hiding this comment.
Deleting and recreating resources is a bit heavyhanded though, and I'm not sure if we want to do it just to have uniform selector labels between the collector and target allocator.
There was a problem hiding this comment.
I brought this label back. I don't think it makes the code substantially messier - check it our for yourselves.
There was a problem hiding this comment.
fair enough! Maybe lets keep it as it is then, and leave a comment explaining the divergence between the two.
swiatekm
force-pushed
the
chore/targetallocator-labels
branch
from
10352fe
to
994e5bf
Compare
* Add version label to target allocator resources * Use manifestutils.SelectorLabels for target allocator resources
Description:
Add
app.kubernetes.io/versionlabel to all target allocator resources. Where the label was present, it now uses the target allocator version instead of the collector version. Usemanifestutils.Labelsas the implementation.I've also used
manifestutils.SelectorLabelswhere it makes sense.Link to tracking Issue: #2454
Testing:
Modified existing tests.