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

Make clusterName optional on EKS and GKE #1067

Merged
merged 14 commits into from
Jan 2, 2024
Merged

Conversation

crobert-1
Copy link
Contributor

@crobert-1 crobert-1 commented Dec 7, 2023

Description:
Now that the resource detection processor supports detecting the k8s.cluster.name in GKE and EKS environments, the clusterName value can be optional in these environments.

A simplified resource detection processor has been added to more pipelines in this change as well, in every place the clusterName was manually being added.

Testing:
Tested in GKE and kOps. GKE sets cluster name properly without it specified, and overrides the detected k8s.cluster.name when clusterName is set. kOps fails without clusterName with the following message:

crobert$ ~/dev/splunk-otel-collector-chart $ helm upgrade ...
Error: UPGRADE FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
splunk-otel-collector:
- (root): Must validate "then" as "if" was valid
- clusterName: String length must be greater than or equal to 1
- (root): Must validate all the schemas (allOf)

The - (root) lines are a bit confusing, but that's simply saying that the check failed, AFAICT.

Also, removed clusterName option from functional tests and changed expected k8s.cluster.name value in telemetry to be rotel-eks instead of the manually set value.

Documentation:
Updated README and relevant comments in code.

@crobert-1
Copy link
Contributor Author

Note, this is a second attempt of #1056 to enable testing to work properly. I have not yet implemented changes requested in that review, other than README wording.

@crobert-1
Copy link
Contributor Author

I believe I've addressed all comments on the original PR now, so I'm marking as ready for review.

@crobert-1 crobert-1 marked this pull request as ready for review December 8, 2023 22:31
@crobert-1 crobert-1 requested review from a team as code owners December 8, 2023 22:31
Copy link
Contributor

@jvoravong jvoravong left a comment

Choose a reason for hiding this comment

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

One nit. Mostly LGTM. Approving. Please address other review comments before merging.

@jinja2
Copy link
Collaborator

jinja2 commented Dec 14, 2023

@crobert-1 I think it is worth updating the eks functional_tests to test with the resourcedetector added cluster-name. The test currently sets the cluster name in values which if removed, should default to the resourcedetector provided attr. This would need a change to the testdata, since the name of the eks cluster being tested on is rotel-eks and not the provided dev-operator.

@crobert-1 crobert-1 force-pushed the optional_clustername branch 2 times, most recently from 0009ed2 to a597ef7 Compare December 20, 2023 19:31
@crobert-1
Copy link
Contributor Author

I've updated functional tests to account for the EKS optional cluster name. These tests are currently being skipped as the functionality is not complete, but the logic is in place for when they're running. I've moved the kind cluster test data to a sub-directory, along with the eks cluster data to its own sub-directory. The EKS data is not yet being used.

crobert-1 and others added 11 commits January 2, 2024 11:13
- Move duplicated check to single place
- Simplify regex
- Disable all unrelated resource attributes in new resource detection processor
- Remove env detector when unecessary.
Co-authored-by: jvoravong <47871238+jvoravong@users.noreply.github.com>
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
- Only include cluster name detection when required (no clusterName specified)
- Update tests to auto detect cluster name
@jinja2
Copy link
Collaborator

jinja2 commented Jan 2, 2024

hmm, weird that the EKS tests are being skipped after the rebase since the source isn't a fork nvm, the eks tests were skipped this time due to kind tests failing.

@crobert-1
Copy link
Contributor Author

Regarding the failing upgrade test: It's failing because the cluster name is no longer passed in to the install command. Since the test is running a previous version of the chart without this PR's changes, the clusterName is still required, even in EKS environments. I think it's okay to merge this change as once this PR is no longer the latest commit in main, it should pass again.

@atoulme
Copy link
Contributor

atoulme commented Jan 2, 2024

Please go ahead and merge.

@crobert-1 crobert-1 merged commit 7b67ff3 into main Jan 2, 2024
31 of 32 checks passed
@crobert-1 crobert-1 deleted the optional_clustername branch January 2, 2024 23:17
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants