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.

Sign up for GitHub

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

Jump to bottom

Add EKS cluster name detection #27408

Closed
wants to merge 30 commits into from
Closed

Add EKS cluster name detection #27408

wants to merge 30 commits into from

Conversation

crobert-1
Copy link
Member

Description:

This enhancement detects the k8s cluster name in EKS. The solution uses EC2 instance tags to determine the cluster name, which means it will only work on EC2 (as noted in documentation updates).

Link to tracking Issue:
Resolves #26794

Testing:
Manual testing in an EKS cluster when running on EC2 instances.

Documentation:
Updated README and relevant metadata

@crobert-1 crobert-1 requested a review from a team October 3, 2023 17:16
@github-actions github-actions bot added cmd/configschema configschema command cmd/otelcontribcol otelcontribcol command exporter/datadog Datadog components processor/resourcedetection Resource detection processor labels Oct 3, 2023
@crobert-1 crobert-1 added the enhancement New feature or request label Oct 3, 2023
dashpole
dashpole reviewed Oct 3, 2023
View reviewed changes
Copy link
Contributor

@dashpole dashpole left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM overall, but someone from AWS should take a look.

processor/resourcedetectionprocessor/internal/aws/eks/detector.go Outdated Show resolved Hide resolved
processor/resourcedetectionprocessor/internal/aws/eks/detector.go Outdated Show resolved Hide resolved
processor/resourcedetectionprocessor/internal/aws/eks/detector.go Outdated Show resolved Hide resolved
processor/resourcedetectionprocessor/internal/aws/eks/detector.go Outdated Show resolved Hide resolved
@jsirianni jsirianni mentioned this pull request Oct 5, 2023
Closed
@crobert-1 crobert-1 force-pushed the eks_cluster_name branch 2 times, most recently from 274008a to 9965c6b Compare October 17, 2023 18:45
@crobert-1 crobert-1 mentioned this pull request Oct 19, 2023
Merged
@jinja2 jinja2 mentioned this pull request Oct 20, 2023
Closed
6 tasks
@crobert-1
Copy link
Member Author

Sorry for all of the updates here, linting is failing with OOM on the GitHub runners. I'm trying to find the best balance here of getting lint to complete successfully in the least amount of time possible.

atoulme
atoulme reviewed Oct 25, 2023
View reviewed changes
.golangci.yml Outdated
@@ -4,7 +4,7 @@ run:
concurrency: 4

# timeout for analysis, e.g. 30s, 5m, default is 1m
timeout: 20m
timeout: 100m
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still necessary?

Copy link
Member Author

@crobert-1 crobert-1 Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My apologies, I should have marked this PR as draft earlier. All of the changes you marked were a part of my testing to determine the failure point in the linting process. I'm going to revert as many of these changes as possible, and mark the review as Ready for Review when it's ready again.

Thanks for calling all of these out!

Copy link
Contributor

Choose a reason for hiding this comment

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

No apologies necessary :) happy to review when it's good to go

crobert-1 and others added 23 commits October 26, 2023 09:15
@crobert-1
Changes requested by dashpole:
8810443 
- Don't log and error, just error
- Return error for empty cluster name
- Return other information for other resources even if cluster name
detection fails.
@crobert-1
Fix CICD failures
73315bc 
- Lint fix
- Test fix: Move accessory methods into utils helper for testability.
@crobert-1
Changes requested by jinja2
91cd314 
- Cleanup config initialization
- Use another possible tag for cluster name
- Only get local EC2 instance tags
@crobert-1
Use proper context, set higher default timeout
5315876
@crobert-1
Remove slices import since it's not introduced until later go versions
95191b2
@crobert-1
Disable k8s cluster name by default
baa172d
@crobert-1
Run make generate
1e1bb17
@crobert-1
Fix test for disabled value
fdad8c6
@crobert-1
TESTING: Check if longer timeout will allow lint to succeed
9fd765b
@crobert-1
TEST: Increase timeout to possibly allow linting pass
5cc324e
@crobert-1
TEST: Increase timeout even more
d12a15f
@crobert-1
TEST: More timeout increases
fa57b20
@crobert-1
Keep increasing timeouts
4c3ffb7
@crobert-1
TEST: Skip failing lint test
836d2cb
@crobert-1
TEST: Skip entire cmd dir for linting
456afe6
@crobert-1
TEST: Increase memory garbage collection frequency
edd762e
@crobert-1
TEST: Increase memory allowance
de0620d
@crobert-1
Test: reduce memory usage
08879a4
@crobert-1
TEST: GC set to 25
f36249e
@crobert-1
TEST: GC to 37
d0c58be
@crobert-1
TEST: GC to 43
7ac141a
@crobert-1
TEST: GC to 45
d0be0b2
@crobert-1
Revert timeout changes back to original values
e6eb190
@dmitryax
Copy link
Member

Looks like your linter config change didn't work out. I see make -C cmd/otelcontribcol lint in the tests. The linter uses cmd/otelcontribcol as root directory

@crobert-1 crobert-1 mentioned this pull request Oct 26, 2023
Merged
@crobert-1
Copy link
Member Author

Superseded by #28649. v1 of the aws-sdk-go library doesn't run into the linting issues, so I've decided to use that instead.

@crobert-1 crobert-1 closed this Oct 26, 2023
@crobert-1 crobert-1 deleted the eks_cluster_name branch October 26, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jinja2 jinja2 jinja2 left review comments

@dashpole dashpole dashpole left review comments

@atoulme atoulme atoulme left review comments

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@mx-psi mx-psi Awaiting requested review from mx-psi mx-psi is a code owner

@songy23 songy23 Awaiting requested review from songy23 songy23 is a code owner

@dmitryax dmitryax Awaiting requested review from dmitryax

@dineshg13 dineshg13 Awaiting requested review from dineshg13

@gbbr gbbr Awaiting requested review from gbbr

@liustanley liustanley Awaiting requested review from liustanley

@mackjmr mackjmr Awaiting requested review from mackjmr

@pmcollins pmcollins Awaiting requested review from pmcollins

Assignees

@atoulme atoulme

Labels
cmd/configschema configschema command cmd/otelcontribcol otelcontribcol command enhancement New feature or request exporter/datadog Datadog components processor/resourcedetection Resource detection processor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[processor/resourcedetection]: Add support for k8s.cluster.name where possible
6 participants
@crobert-1 @dmitryax @atoulme @dashpole @mx-psi @jinja2