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

Keep multiple versions of Collector Config #2946

Merged
merged 20 commits into from
May 28, 2024
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
17 changes: 17 additions & 0 deletions .chloggen/matth.versioned_config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# 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. collector, target allocator, auto-instrumentation, opamp, github action)
component: collector

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Keep multiple previous versions of the Collector ConfigMap, configurable via the ConfigVersions field.

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

# (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 change introduces a new field in the Collector ConfigMap, `ConfigVersions`, which allows users to specify the number of previous versions of the Collector ConfigMap to keep. The default value is 1, which means that the current and one previous version of the Collector ConfigMap are kept. By keeping historical versions of the configuration, we ensure that during a config upgrade the previous configuration is still available for running (non-upgraded) pods as well as for rollbacks. If we overwrite the original ConfigMap with the new configuration, any pod which restarts for any reason will get the new configuration, which makes rollouts impossible to control.
6 changes: 6 additions & 0 deletions apis/v1beta1/opentelemetrycollector_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ type OpenTelemetryCollectorSpec struct {
// +required
// +kubebuilder:pruning:PreserveUnknownFields
Config Config `json:"config"`
// ConfigVersions defines the number versions to keep for the collector config. Each config version is stored in a separate ConfigMap.
// Defaults to 3. The minimum value is 1.
// +optional
// +kubebuilder:default:=3
matthagenbuch marked this conversation as resolved.
Show resolved Hide resolved
// +kubebuilder:validation:Minimum:=1
ConfigVersions int `json:"configVersions,omitempty"`
jaronoff97 marked this conversation as resolved.
Show resolved Hide resolved
// Ingress is used to specify how OpenTelemetry Collector is exposed. This
// functionality is only available if one of the valid modes is set.
// Valid modes are: deployment, daemonset and statefulset.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5555,6 +5555,10 @@ spec:
- service
type: object
x-kubernetes-preserve-unknown-fields: true
configVersions:
default: 3
minimum: 1
type: integer
configmaps:
items:
properties:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5541,6 +5541,10 @@ spec:
- service
type: object
x-kubernetes-preserve-unknown-fields: true
configVersions:
default: 3
minimum: 1
type: integer
configmaps:
items:
properties:
Expand Down
29 changes: 19 additions & 10 deletions controllers/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"github.com/open-telemetry/opentelemetry-operator/internal/config"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
)

Expand Down Expand Up @@ -86,6 +87,10 @@ service:
goodConfig := v1beta1.Config{}
err := go_yaml.Unmarshal([]byte(goodConfigYaml), &goodConfig)
require.NoError(t, err)

goodConfigHash, _ := manifestutils.GetConfigMapSHA(goodConfig)
goodConfigHash = goodConfigHash[:8]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. These tests were always a pain to update because of this.


one := int32(1)
type args struct {
instance v1beta1.OpenTelemetryCollector
Expand Down Expand Up @@ -164,7 +169,7 @@ service:
VolumeSource: corev1.VolumeSource{
ConfigMap: &corev1.ConfigMapVolumeSource{
LocalObjectReference: corev1.LocalObjectReference{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
},
Items: []corev1.KeyToPath{
{
Expand Down Expand Up @@ -223,7 +228,7 @@ service:
},
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
Namespace: "test",
Labels: map[string]string{
"app.kubernetes.io/component": "opentelemetry-collector",
Expand Down Expand Up @@ -414,7 +419,7 @@ service:
VolumeSource: corev1.VolumeSource{
ConfigMap: &corev1.ConfigMapVolumeSource{
LocalObjectReference: corev1.LocalObjectReference{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
},
Items: []corev1.KeyToPath{
{
Expand Down Expand Up @@ -473,7 +478,7 @@ service:
},
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
Namespace: "test",
Labels: map[string]string{
"app.kubernetes.io/component": "opentelemetry-collector",
Expand Down Expand Up @@ -700,7 +705,7 @@ service:
VolumeSource: corev1.VolumeSource{
ConfigMap: &corev1.ConfigMapVolumeSource{
LocalObjectReference: corev1.LocalObjectReference{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
},
Items: []corev1.KeyToPath{
{
Expand Down Expand Up @@ -759,7 +764,7 @@ service:
},
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
Namespace: "test",
Labels: map[string]string{
"app.kubernetes.io/component": "opentelemetry-collector",
Expand Down Expand Up @@ -1138,6 +1143,10 @@ service:
goodConfig := v1beta1.Config{}
err := go_yaml.Unmarshal([]byte(goodConfigYaml), &goodConfig)
require.NoError(t, err)

goodConfigHash, _ := manifestutils.GetConfigMapSHA(goodConfig)
goodConfigHash = goodConfigHash[:8]

one := int32(1)
type args struct {
instance v1beta1.OpenTelemetryCollector
Expand Down Expand Up @@ -1225,7 +1234,7 @@ service:
VolumeSource: corev1.VolumeSource{
ConfigMap: &corev1.ConfigMapVolumeSource{
LocalObjectReference: corev1.LocalObjectReference{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
},
Items: []corev1.KeyToPath{
{
Expand Down Expand Up @@ -1284,7 +1293,7 @@ service:
},
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
Namespace: "test",
Labels: map[string]string{
"app.kubernetes.io/component": "opentelemetry-collector",
Expand Down Expand Up @@ -1620,7 +1629,7 @@ prometheus_cr:
VolumeSource: corev1.VolumeSource{
ConfigMap: &corev1.ConfigMapVolumeSource{
LocalObjectReference: corev1.LocalObjectReference{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
},
Items: []corev1.KeyToPath{
{
Expand Down Expand Up @@ -1679,7 +1688,7 @@ prometheus_cr:
},
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector",
Name: "test-collector-" + goodConfigHash,
Namespace: "test",
Labels: map[string]string{
"app.kubernetes.io/component": "opentelemetry-collector",
Expand Down
34 changes: 34 additions & 0 deletions controllers/opentelemetrycollector_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ package controllers

import (
"context"
"fmt"
"sort"

"github.com/go-logr/logr"
routev1 "github.com/openshift/api/route/v1"
Expand Down Expand Up @@ -111,6 +113,17 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont
ownedObjects[uid] = object
}
}

configMapList := &corev1.ConfigMapList{}
err := r.List(ctx, configMapList, listOps)
if err != nil {
return nil, fmt.Errorf("error listing ConfigMaps: %w", err)
}
ownedConfigMaps := r.getConfigMapsToRemove(params.OtelCol.Spec.ConfigVersions, configMapList)
for i := range ownedConfigMaps {
ownedObjects[ownedConfigMaps[i].GetUID()] = &ownedConfigMaps[i]
}

return ownedObjects, nil
}

Expand All @@ -134,6 +147,27 @@ func (r *OpenTelemetryCollectorReconciler) findClusterRoleObjects(ctx context.Co
return ownedObjects, nil
}

// getConfigMapsToRemove returns a list of ConfigMaps to remove based on the number of ConfigMaps to keep.
// It keeps the newest ConfigMap, the `configVersionsToKeep` next newest ConfigMaps, and returns the remainder.
func (r *OpenTelemetryCollectorReconciler) getConfigMapsToRemove(configVersionsToKeep int, configMapList *corev1.ConfigMapList) []corev1.ConfigMap {
configVersionsToKeep = max(1, configVersionsToKeep)
ownedConfigMaps := []corev1.ConfigMap{}
sort.Slice(configMapList.Items, func(i, j int) bool {
iTime := configMapList.Items[i].GetCreationTimestamp().Time
jTime := configMapList.Items[j].GetCreationTimestamp().Time
// sort the ConfigMaps newest to oldest
return iTime.After(jTime)
})

for i := range configMapList.Items {
if i > configVersionsToKeep {
ownedConfigMaps = append(ownedConfigMaps, configMapList.Items[i])
}
}

return ownedConfigMaps
}

func (r *OpenTelemetryCollectorReconciler) getParams(instance v1beta1.OpenTelemetryCollector) (manifests.Params, error) {
p := manifests.Params{
Config: r.config,
Expand Down
12 changes: 9 additions & 3 deletions controllers/reconcile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,9 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
result: controllerruntime.Result{},
checks: []check[v1alpha1.OpenTelemetryCollector]{
func(t *testing.T, params v1alpha1.OpenTelemetryCollector) {
exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.Collector(params.Name), params.Namespace))
configHash, _ := getConfigMapSHAFromString(params.Spec.Config)
configHash = configHash[:8]
exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.ConfigMap(params.Name, configHash), params.Namespace))
assert.NoError(t, err)
assert.True(t, exists)
exists, err = populateObjectIfExists(t, &appsv1.StatefulSet{}, namespacedObjectName(naming.Collector(params.Name), params.Namespace))
Expand All @@ -452,7 +454,9 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
result: controllerruntime.Result{},
checks: []check[v1alpha1.OpenTelemetryCollector]{
func(t *testing.T, params v1alpha1.OpenTelemetryCollector) {
exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.Collector(params.Name), params.Namespace))
configHash, _ := getConfigMapSHAFromString(params.Spec.Config)
configHash = configHash[:8]
exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.ConfigMap(params.Name, configHash), params.Namespace))
assert.NoError(t, err)
assert.True(t, exists)
actual := v1.ConfigMap{}
Expand Down Expand Up @@ -497,7 +501,9 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
result: controllerruntime.Result{},
checks: []check[v1alpha1.OpenTelemetryCollector]{
func(t *testing.T, params v1alpha1.OpenTelemetryCollector) {
exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.Collector(params.Name), params.Namespace))
configHash, _ := getConfigMapSHAFromString(params.Spec.Config)
matthagenbuch marked this conversation as resolved.
Show resolved Hide resolved
configHash = configHash[:8]
exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.ConfigMap(params.Name, configHash), params.Namespace))
assert.NoError(t, err)
assert.True(t, exists)
actual := v1.ConfigMap{}
Expand Down
11 changes: 11 additions & 0 deletions controllers/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import (
logf "sigs.k8s.io/controller-runtime/pkg/log"
metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
"sigs.k8s.io/controller-runtime/pkg/webhook"
"sigs.k8s.io/yaml"

"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
Expand All @@ -60,6 +61,7 @@ import (
"github.com/open-telemetry/opentelemetry-operator/internal/config"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/testdata"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
"github.com/open-telemetry/opentelemetry-operator/internal/rbac"
// +kubebuilder:scaffold:imports
)
Expand Down Expand Up @@ -480,3 +482,12 @@ func populateObjectIfExists(t testing.TB, object client.Object, namespacedName t
}
return true, nil
}

func getConfigMapSHAFromString(configStr string) (string, error) {
var config v1beta1.Config
err := yaml.Unmarshal([]byte(configStr), &config)
if err != nil {
return "", err
}
return manifestutils.GetConfigMapSHA(config)
}
11 changes: 11 additions & 0 deletions docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -29872,6 +29872,17 @@ doing so, you wil accept the risk of it breaking things.<br/>
for the workload.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>configVersions</b></td>
<td>integer</td>
<td>
ConfigVersions defines the number versions to keep for the collector config. Each config version is stored in a separate ConfigMap.
Defaults to 3. The minimum value is 1.<br/>
<br/>
<i>Default</i>: 3<br/>
<i>Minimum</i>: 1<br/>
</td>
<td>false</td>
</tr><tr>
<td><b><a href="#opentelemetrycollectorspecconfigmapsindex-1">configmaps</a></b></td>
<td>[]object</td>
Expand Down
9 changes: 7 additions & 2 deletions internal/manifests/collector/configmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,13 @@ import (
)

func ConfigMap(params manifests.Params) (*corev1.ConfigMap, error) {
name := naming.ConfigMap(params.OtelCol.Name)
labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})
hash, err := manifestutils.GetConfigMapSHA(params.OtelCol.Spec.Config)
if err != nil {
return nil, err
}
name := naming.ConfigMap(params.OtelCol.Name, hash)
collectorName := naming.Collector(params.OtelCol.Name)
labels := manifestutils.Labels(params.OtelCol.ObjectMeta, collectorName, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})

replacedConf, err := ReplaceConfig(params.OtelCol)
if err != nil {
Expand Down
29 changes: 20 additions & 9 deletions internal/manifests/collector/configmap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ import (
"testing"

"github.com/stretchr/testify/assert"

"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
"github.com/open-telemetry/opentelemetry-operator/internal/naming"
matthagenbuch marked this conversation as resolved.
Show resolved Hide resolved
)

func TestDesiredConfigMap(t *testing.T) {
Expand All @@ -29,9 +32,6 @@ func TestDesiredConfigMap(t *testing.T) {
}

t.Run("should return expected collector config map", func(t *testing.T) {
expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector"
expectedLables["app.kubernetes.io/name"] = "test-collector"
expectedLables["app.kubernetes.io/version"] = "0.47.0"

expectedData := map[string]string{
"collector.yaml": `receivers:
Expand All @@ -58,10 +58,17 @@ service:
}

param := deploymentParams()
hash, _ := manifestutils.GetConfigMapSHA(param.OtelCol.Spec.Config)
expectedName := naming.ConfigMap("test", hash)

expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector"
expectedLables["app.kubernetes.io/name"] = "test-collector"
expectedLables["app.kubernetes.io/version"] = "0.47.0"

actual, err := ConfigMap(param)

assert.NoError(t, err)
assert.Equal(t, "test-collector", actual.Name)
assert.Equal(t, expectedName, actual.Name)
assert.Equal(t, expectedLables, actual.Labels)
assert.Equal(t, len(expectedData), len(actual.Data))
for k, expected := range expectedData {
Expand All @@ -70,10 +77,6 @@ service:
})

t.Run("should return expected escaped collector config map with target_allocator config block", func(t *testing.T) {
expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector"
expectedLables["app.kubernetes.io/name"] = "test-collector"
expectedLables["app.kubernetes.io/version"] = "latest"

expectedData := map[string]string{
"collector.yaml": `exporters:
debug:
Expand All @@ -97,11 +100,19 @@ service:

param, err := newParams("test/test-img", "testdata/http_sd_config_servicemonitor_test.yaml")
assert.NoError(t, err)

hash, _ := manifestutils.GetConfigMapSHA(param.OtelCol.Spec.Config)
expectedName := naming.ConfigMap("test", hash)

expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector"
expectedLables["app.kubernetes.io/name"] = "test-collector"
expectedLables["app.kubernetes.io/version"] = "latest"

param.OtelCol.Spec.TargetAllocator.Enabled = true
actual, err := ConfigMap(param)

assert.NoError(t, err)
assert.Equal(t, "test-collector", actual.Name)
assert.Equal(t, expectedName, actual.Name)
assert.Equal(t, expectedLables, actual.Labels)
assert.Equal(t, len(expectedData), len(actual.Data))
for k, expected := range expectedData {
Expand Down
Loading
Loading