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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

matthagenbuch
Copy link

Description:

From the issue:

When changing the configuration of a collector, I have no option for a controlled rollout / rollback process in the case of an error. Once the new configuration is applied to my OpenTelemetryCollector resource, the old configuration is overwritten and all running collector pods now see the new configuration.

This makes it difficult to control the rollout process, as any pod that restarts for any reason will load with the new configuration. It also makes it impossible to roll back to the previous configuration without again updating the OpenTelemetryCollector resource.

This PR resolves the issue by keeping 1 or more previous versions of the Collector ConfigMap. The ConfigMap name is suffixed with an 8-character hash of the Config object. The full hash is truncated to 8 characters to leave room in the 63-character name limit for the collector name.

The number of config versions to keep can be specified using the new ConfigVersions field on the CRD. The operator will keep the current version + ConfigVersions previous versions. Cleanup of old versions is handled using the reconciliation code path.

This solution was preferred to support for externally managed ConfigMaps based on conversations at the SIG meeting. External ConfigMaps would break some functionality of the operator which requires the collector config to be known by the operator.

Link to tracking Issue(s):

Testing:
local manual testing, unit tests, e2e tests

Documentation:
New configuration value on OpenTelemetryCollector CRD for setting the number of versions to keep.

@matthagenbuch matthagenbuch requested a review from a team as a code owner May 10, 2024 14:52
Copy link

linux-foundation-easycla bot commented May 10, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

@swiatekm-sumo swiatekm-sumo left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me, and is pleasantly much simpler than I expected. Left some comments mostly around the CRD field.

apis/v1beta1/opentelemetrycollector_types.go Show resolved Hide resolved
apis/v1beta1/opentelemetrycollector_types.go Outdated Show resolved Hide resolved
@@ -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.

controllers/opentelemetrycollector_controller.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

a few minor thoughts, otherwise this looks awesome!

controllers/builder_test.go Outdated Show resolved Hide resolved
controllers/opentelemetrycollector_controller.go Outdated Show resolved Hide resolved
controllers/reconcile_test.go Show resolved Hide resolved
apis/v1beta1/opentelemetrycollector_types.go Show resolved Hide resolved
@matthagenbuch
Copy link
Author

@swiatekm-sumo @pavolloffay any other thoughts on this?

Copy link
Contributor

@swiatekm-sumo swiatekm-sumo left a comment

Choose a reason for hiding this comment

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

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify the collector configuration in an external ConfigMap
4 participants