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

[receiver/k8scluster] add support for observing resources for a specific namespace #35727

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

[receiver/k8scluster] add support for observing resources for a specific namespace #35727

wants to merge 47 commits into from

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented Oct 10, 2024 •
edited
Loading

Description

This PR extends the k8scluster receiver with an option to limit the observed resources to a specific namespace.

Link to tracking issue

Fixes #9401

Testing

added unit and e2e tests

Documentation

Added section about how to make use of Roles and RoleBindings instead of ClusterRoles and ClusterRoleBindings

@bacherfl
replace deprecated fake client initialisation
f259d1d 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
add e2e test scenario for namespaced k8s cluster receiver
981f1c2 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
remove non-fitting resources from namespaced e2e test
bfc23d8 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
add namespace to role binding
5b9c6ac 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
adapt test expectations
46d11fd 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
revert accidental change
dc3b2f3 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
omit cluster scoped observers if namespace filter is set
1b53fa0 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
[revert when done] - adapt test setup to run locally
e18f4e5 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
Merge branch 'main' into feat/9401/namespaced-cluster-receiver
873b489
@bacherfl
Merge branch 'feat/9401/namespaced-cluster-receiver' of https://githu…
fd62f43 
…b.com/bacherfl/opentelemetry-collector-contrib into feat/9401/namespaced-cluster-receiver
@bacherfl
fix informer setup
a2990f3 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
update expected metrics
c8b1f29 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
do not try to observe cluster resource quotas when namespace limit ha…
f94b075 
…s been specified

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
extend readme and add changelog entry
b0710fb 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
add resourcequota access to role
3fa57f5 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl bacherfl marked this pull request as ready for review October 17, 2024 11:49
@bacherfl bacherfl requested a review from a team as a code owner October 17, 2024 11:49
@bacherfl
Copy link
Contributor Author

bacherfl commented Nov 6, 2024

I see e2e tests are failing on main anyways: https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/11701936581/job/32589392145

I wonder if #36114 introduced some flakiness.

yes i think part of the flakiness might be due to the addition of the cronjob object which causes multiple pods to spin up during the duration of the test, leading to a varying number of metrics received in the sink

@bacherfl
use separate namespace for namespace-scoped test
1968099 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
fix path to testobjects directory
fcbebe3 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
ensure namespace is created before applying other tests
a4f9130 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@ChrsMark
Copy link
Member

ChrsMark commented Nov 6, 2024

yes i think part of the flakiness might be due to the addition of the cronjob object which causes multiple pods to spin up during the duration of the test, leading to a varying number of metrics received in the sink

Sounds plausible, we should restrict the number of active jobs/pods to 1 to avoid having scheduling/timing related flakiness: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/36114/files#diff-c08d5cc648ae8a9f364187a6dbf9a8b1e01070249b7a8669537009ccc56e701fR147.

I will try to send a PR for this.

@bacherfl
fix namespace in confmap
19f947f 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
adapt check for required number of metrics
dca10d1 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@ChrsMark ChrsMark mentioned this pull request Nov 6, 2024
Merged
@bacherfl
fix test expectations
742c663 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
trigger CI
75c8b0e 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
increase timeout and ignore k8s.hpa.current_replicas metric value
bc00b77 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
add debug logs
1defed9 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
reduce job sleep time
4bb2346 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
adapt expections
6315edd 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
adapt expections
14aa5c6 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
remove debug logs that are hopefully not needed anymore
10e9267 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
ChrsMark
ChrsMark reviewed Nov 7, 2024
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Let's wait for #36235 to get merged and re-base this one. Meanwhile I'm a bit worried about the changes to the existing tests.

@bacherfl
Copy link
Contributor Author

bacherfl commented Nov 7, 2024

Let's wait for #36235 to get merged and re-base this one. Meanwhile I'm a bit worried about the changes to the existing tests.

agree, i did some changes in the meantime and got the tests working on this PR, but some changes may have been unneccessary - I will try out everything after rebasing and keep the changes as minimal as possible

TylerHelmuth pushed a commit that referenced this pull request Nov 7, 2024
@ChrsMark
[receiver/k8scluster] Fix e2e tests by limiting jobs/pods of cronjob …
3458d51 
…to 1 (#36235)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Fixes what was described at
#35727 (comment).

After
#36114
the `k8scluster` receiver's e2e tests started showing some flakiness
([example](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/11701936581/job/32589392145)).

With this change we ensure that only 1 active job/pod of the cronjob
will be present for the whole lifetime of the test to avoid hitting
timing/scheduling related flakiness.

@bacherfl could you also take a look here?

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes 

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
pull bot pushed a commit to abaguas/opentelemetry-collector-contrib that referenced this pull request Nov 7, 2024
@ChrsMark @pull
[receiver/k8scluster] Fix e2e tests by limiting jobs/pods of cronjob …
5d238cf 
…to 1 (open-telemetry#36235)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Fixes what was described at
open-telemetry#35727 (comment).

After
open-telemetry#36114
the `k8scluster` receiver's e2e tests started showing some flakiness
([example](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/11701936581/job/32589392145)).

With this change we ensure that only 1 active job/pod of the cronjob
will be present for the whole lifetime of the test to avoid hitting
timing/scheduling related flakiness.

@bacherfl could you also take a look here?

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes 

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@bacherfl
Merge branch 'main' into feat/9401/namespaced-cluster-receiver
ca738c1 
# Conflicts:
#	receiver/k8sclusterreceiver/testdata/e2e/cluster-scoped/expected.yaml
@bacherfl
revert changes to test logic
c1810ec 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
extract common helper functions
4a673dc 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
Copy link
Contributor Author

bacherfl commented Nov 8, 2024

Alright this should now be ready again - @ChrsMark I reverted the changes I made earlier in the waitForData function as it turned out with the recent fix being merged they were not required anymore

ChrsMark
ChrsMark approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark 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. Left a nit comment. Thank's @bacherfl!

(it will need a make generate to make the CI happy)

receiver/k8sclusterreceiver/e2e_test.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@povilasv povilasv povilasv approved these changes

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

@ChrsMark ChrsMark ChrsMark approved these changes

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

Assignees

@TylerHelmuth TylerHelmuth

Labels
receiver/k8scluster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

k8sclusterreceiver: Fetch namespace scoped metrics from the Master API
4 participants
@bacherfl @ChrsMark @TylerHelmuth @povilasv