Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add version label to target allocator resources #2455

Merged
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions .chloggen/chore_targetallocator-labels.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
component: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add version label to target allocator resources

# One or more tracking issues related to the change
issues: [2454]

# (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:
9 changes: 9 additions & 0 deletions controllers/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1329,6 +1329,7 @@ service:
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/name": "test-targetallocator",
"app.kubernetes.io/part-of": "opentelemetry",
"app.kubernetes.io/version": "latest",
},
Annotations: nil,
},
Expand All @@ -1344,6 +1345,7 @@ service:
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/name": "test-targetallocator",
"app.kubernetes.io/part-of": "opentelemetry",
"app.kubernetes.io/version": "latest",
},
Annotations: map[string]string{
"opentelemetry-targetallocator-config/hash": "4d1911fd40106e9e2dd3d928f067a6c8c9eab0c569f737ba3701c6f5a9aad6d7",
Expand Down Expand Up @@ -1433,6 +1435,7 @@ service:
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/name": "test-targetallocator",
"app.kubernetes.io/part-of": "opentelemetry",
"app.kubernetes.io/version": "latest",
},
},
},
Expand All @@ -1446,6 +1449,7 @@ service:
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/name": "test-targetallocator",
"app.kubernetes.io/part-of": "opentelemetry",
"app.kubernetes.io/version": "latest",
},
Annotations: nil,
},
Expand Down Expand Up @@ -1685,6 +1689,7 @@ service:
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/name": "test-targetallocator",
"app.kubernetes.io/part-of": "opentelemetry",
"app.kubernetes.io/version": "latest",
},
Annotations: nil,
},
Expand All @@ -1700,6 +1705,7 @@ service:
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/name": "test-targetallocator",
"app.kubernetes.io/part-of": "opentelemetry",
"app.kubernetes.io/version": "latest",
},
Annotations: map[string]string{
"opentelemetry-targetallocator-config/hash": "4d1911fd40106e9e2dd3d928f067a6c8c9eab0c569f737ba3701c6f5a9aad6d7",
Expand Down Expand Up @@ -1789,6 +1795,7 @@ service:
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/name": "test-targetallocator",
"app.kubernetes.io/part-of": "opentelemetry",
"app.kubernetes.io/version": "latest",
},
},
},
Expand All @@ -1802,6 +1809,7 @@ service:
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/name": "test-targetallocator",
"app.kubernetes.io/part-of": "opentelemetry",
"app.kubernetes.io/version": "latest",
},
Annotations: nil,
},
Expand Down Expand Up @@ -1829,6 +1837,7 @@ service:
"app.kubernetes.io/managed-by": "opentelemetry-operator",
"app.kubernetes.io/name": "test-targetallocator",
"app.kubernetes.io/part-of": "opentelemetry",
"app.kubernetes.io/version": "latest",
},
Annotations: nil,
},
Expand Down
8 changes: 0 additions & 8 deletions internal/manifests/targetallocator/configmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
package targetallocator

import (
"strings"

"gopkg.in/yaml.v2"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -35,13 +33,7 @@ const (

func ConfigMap(params manifests.Params) (*corev1.ConfigMap, error) {
name := naming.TAConfigMap(params.OtelCol.Name)
version := strings.Split(params.OtelCol.Spec.Image, ":")
labels := Labels(params.OtelCol, name)
if len(version) > 1 {
labels["app.kubernetes.io/version"] = version[len(version)-1]
} else {
labels["app.kubernetes.io/version"] = "latest"
}

// Collector supports environment variable substitution, but the TA does not.
// TA ConfigMap should have a single "$", as it does not support env var substitution
Expand Down
2 changes: 1 addition & 1 deletion internal/manifests/targetallocator/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test it. If there's a problem with that, then it's probably for the best to leave the selector as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I brought this label back. I don't think it makes the code substantially messier - check it our for yourselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough! Maybe lets keep it as it is then, and leave a comment explaining the divergence between the two.

},
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Expand Down
5 changes: 4 additions & 1 deletion internal/manifests/targetallocator/deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func TestDeploymentNewDefault(t *testing.T) {
assert.Len(t, d.Spec.Template.Annotations, 1)

// the pod selector should match the pod spec's labels
assert.Equal(t, d.Spec.Template.Labels, d.Spec.Selector.MatchLabels)
assert.Subset(t, d.Spec.Template.Labels, d.Spec.Selector.MatchLabels)
}

func TestDeploymentPodAnnotations(t *testing.T) {
Expand Down Expand Up @@ -187,6 +187,9 @@ func collectorInstance() v1alpha1.OpenTelemetryCollector {
Spec: v1alpha1.OpenTelemetryCollectorSpec{
Image: "ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator:0.47.0",
Config: string(configYAML),
TargetAllocator: v1alpha1.OpenTelemetryTargetAllocator{
Image: "ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-targetallocator:0.47.0",
},
},
}
}
Expand Down
27 changes: 10 additions & 17 deletions internal/manifests/targetallocator/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,20 @@ package targetallocator

import (
"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
"github.com/open-telemetry/opentelemetry-operator/internal/naming"
)

// Labels return the common labels to all TargetAllocator objects that are part of a managed OpenTelemetryCollector.
func Labels(instance v1alpha1.OpenTelemetryCollector, name string) map[string]string {
// new map every time, so that we don't touch the instance's label
base := map[string]string{}
if nil != instance.Labels {
for k, v := range instance.Labels {
base[k] = v
}
}

base["app.kubernetes.io/managed-by"] = "opentelemetry-operator"
base["app.kubernetes.io/instance"] = naming.Truncate("%s.%s", 63, instance.Namespace, instance.Name)
base["app.kubernetes.io/part-of"] = "opentelemetry"
base["app.kubernetes.io/component"] = "opentelemetry-targetallocator"

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is awesome

}

return base
// SelectorLabels return the selector labels for Target Allocator Pods.
func SelectorLabels(instance v1alpha1.OpenTelemetryCollector) map[string]string {
selectorLabels := manifestutils.SelectorLabels(instance.ObjectMeta, ComponentOpenTelemetryTargetAllocator)
// TargetAllocator uses the name label as well for selection
// This is inconsistent with the Collector, but changing is a somewhat painful breaking change
selectorLabels["app.kubernetes.io/name"] = naming.TargetAllocator(instance.Name)
return selectorLabels
}
22 changes: 21 additions & 1 deletion internal/manifests/targetallocator/labels_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
"github.com/open-telemetry/opentelemetry-operator/internal/naming"
)

const (
Expand All @@ -43,6 +44,7 @@ func TestLabelsCommonSet(t *testing.T) {
assert.Equal(t, "my-ns.my-instance", labels["app.kubernetes.io/instance"])
assert.Equal(t, "opentelemetry", labels["app.kubernetes.io/part-of"])
assert.Equal(t, "opentelemetry-targetallocator", labels["app.kubernetes.io/component"])
assert.Equal(t, "latest", labels["app.kubernetes.io/version"])
assert.Equal(t, name, labels["app.kubernetes.io/name"])
}

Expand All @@ -61,7 +63,25 @@ func TestLabelsPropagateDown(t *testing.T) {
labels := Labels(otelcol, name)

// verify
assert.Len(t, labels, 6)
assert.Len(t, labels, 7)
assert.Equal(t, "mycomponent", labels["myapp"])
assert.Equal(t, "test", labels["app.kubernetes.io/name"])
}

func TestSelectorLabels(t *testing.T) {
// prepare
otelcol := v1alpha1.OpenTelemetryCollector{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
},
}

// test
labels := SelectorLabels(otelcol)
assert.Equal(t, "opentelemetry-operator", labels["app.kubernetes.io/managed-by"])
assert.Equal(t, "my-ns.my-instance", labels["app.kubernetes.io/instance"])
assert.Equal(t, "opentelemetry", labels["app.kubernetes.io/part-of"])
assert.Equal(t, "opentelemetry-targetallocator", labels["app.kubernetes.io/component"])
assert.Equal(t, naming.TargetAllocator(otelcol.Name), labels["app.kubernetes.io/name"])
}
9 changes: 1 addition & 8 deletions internal/manifests/targetallocator/poddisruptionbudget.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package targetallocator

import (
"fmt"
"strings"

"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
Expand All @@ -42,13 +41,7 @@ func PodDisruptionBudget(params manifests.Params) (*policyV1.PodDisruptionBudget
}

name := naming.TAPodDisruptionBudget(params.OtelCol.Name)
version := strings.Split(params.OtelCol.Spec.Image, ":")
labels := Labels(params.OtelCol, name)
if len(version) > 1 {
labels["app.kubernetes.io/version"] = version[len(version)-1]
} else {
labels["app.kubernetes.io/version"] = "latest"
}

annotations := Annotations(params.OtelCol, nil)

Expand All @@ -65,7 +58,7 @@ func PodDisruptionBudget(params manifests.Params) (*policyV1.PodDisruptionBudget
MinAvailable: params.OtelCol.Spec.TargetAllocator.PodDisruptionBudget.MinAvailable,
MaxUnavailable: params.OtelCol.Spec.TargetAllocator.PodDisruptionBudget.MaxUnavailable,
Selector: &metav1.LabelSelector{
MatchLabels: objectMeta.Labels,
MatchLabels: SelectorLabels(params.OtelCol),
},
},
}, nil
Expand Down
2 changes: 1 addition & 1 deletion internal/manifests/targetallocator/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func Service(params manifests.Params) *corev1.Service {
name := naming.TAService(params.OtelCol.Name)
labels := Labels(params.OtelCol, name)

selector := Labels(params.OtelCol, name)
selector := SelectorLabels(params.OtelCol)
jaronoff97 marked this conversation as resolved.
Show resolved Hide resolved

return &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Expand Down
2 changes: 1 addition & 1 deletion internal/manifests/targetallocator/servicemonitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func ServiceMonitor(params manifests.Params) *monitoringv1.ServiceMonitor {
MatchNames: []string{params.OtelCol.Namespace},
},
Selector: metav1.LabelSelector{
MatchLabels: labels,
MatchLabels: SelectorLabels(params.OtelCol),
},
},
}
Expand Down
4 changes: 4 additions & 0 deletions internal/manifests/targetallocator/targetallocator.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ import (
"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
)

const (
ComponentOpenTelemetryTargetAllocator = "opentelemetry-targetallocator"
)

// Build creates the manifest for the TargetAllocator resource.
func Build(params manifests.Params) ([]client.Object, error) {
var resourceManifests []client.Object
Expand Down