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 tests for sidecar containers #1331

Merged
merged 1 commit into from
Dec 5, 2023
Merged

add tests for sidecar containers #1331

merged 1 commit into from
Dec 5, 2023

Conversation

yangjunmyfm192085
Copy link
Contributor

What this PR does / why we need it:
Add tests for new sidecars feature in kubernetes version 1.28
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1329

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 19, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 19, 2023
@yangjunmyfm192085 yangjunmyfm192085 force-pushed the addsidecartest branch 5 times, most recently from 8c36a6f to e17e33c Compare September 19, 2023 08:16
@yangjunmyfm192085
Copy link
Contributor Author

/retest

@yangjunmyfm192085
Copy link
Contributor Author

/cc @serathius @dgrisonnet @SergeyKanzhelev

@serathius
Copy link
Contributor

serathius commented Sep 19, 2023 •
edited
Loading

There are a lot of unrelated changes, can you split changes for:

  • bumping go dependencies
  • bumping kind version
  • testing sidecar containers

I know that there is dependency, however the first two can be merged immediately without much review, while the last one will take some time to review.

@yangjunmyfm192085
Copy link
Contributor Author

I know that there is dependency, however the first two can be merged immediat

ok, Let me split the changes

@yangjunmyfm192085
Copy link
Contributor Author

/hold
wait for #1332 and #1333

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2023
@yangjunmyfm192085
Copy link
Contributor Author

/test pull-metrics-server-test-e2e-ha

@yangjunmyfm192085
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2023
@yangjunmyfm192085
Copy link
Contributor Author

I have splited the changes.
Could you please take a look again? thanks @serathius

@serathius
Copy link
Contributor

I'm busy with other work. @dgrisonnet can you take a look?

@dashpole
Copy link

/assign @serathius @dgrisonnet
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 21, 2023
SergeyKanzhelev
SergeyKanzhelev reviewed Sep 21, 2023
View reviewed changes
test/e2e_test.go Outdated
return nil
}
if podName == initSidecarContainersPodName {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can simply iterate both collections instead of checking for specific pod name. All init containers must be Ready=true if they run to completion yet and if they are sidecars they will follow the same logic as a regular containers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I have updated it.
could you please take a look again?

@yangjunmyfm192085
Copy link
Contributor Author

/retest

dgrisonnet
dgrisonnet reviewed Nov 29, 2023
View reviewed changes
test/e2e_test.go Outdated
@@ -108,6 +120,8 @@ var _ = Describe("MetricsServer", func() {
panic(err)
}

needTestSideCarsContainers = getSidecarFeature(client)
Copy link
Member

Choose a reason for hiding this comment

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

two naming suggestions here:

  • getSidecarFeature should be renamed to hasSidecarFeature or hasSidecarFeatureEnabled since it returns a bool and not the actual feature object
  • needTestSideCarsContainers could be renamed to testSideCarsContainers

@yangjunmyfm192085 yangjunmyfm192085 force-pushed the addsidecartest branch 2 times, most recently from 938e2bb to bea4bd6 Compare November 30, 2023 01:00
@yangjunmyfm192085
Copy link
Contributor Author

/test pull-metrics-server-test-e2e-ha

1 similar comment
@yangjunmyfm192085
Copy link
Contributor Author

/test pull-metrics-server-test-e2e-ha

@yangjunmyfm192085
Copy link
Contributor Author

/retest

1 similar comment
@yangjunmyfm192085
Copy link
Contributor Author

/retest

@yangjunmyfm192085
Copy link
Contributor Author

/test pull-metrics-server-test-e2e-ha

@yangjunmyfm192085
Copy link
Contributor Author

/hold
Analyze test pull-metrics-server-test-e2e-ha failure reasons

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2023
@yangjunmyfm192085 yangjunmyfm192085 force-pushed the addsidecartest branch 4 times, most recently from 131e53f to 3440e36 Compare December 1, 2023 15:29
dgrisonnet
dgrisonnet reviewed Dec 1, 2023
View reviewed changes
test/e2e_test.go Outdated
Comment on lines 123 to 128
msPods := mustGetMetricsServerPods(client)
for _, pod := range msPods {
// access /apis/metrics.k8s.io/v1beta1/nodes for each pod to ensures that every MS instance get an requests so they expose all apiserver metrics.
_, err := proxyRequestToPod(restConfig, pod.Namespace, pod.Name, "https", 10250, "/apis/metrics.k8s.io/v1beta1/nodes")
Expect(err).NotTo(HaveOccurred(), "Failed to get Metrics Server /apis/metrics.k8s.io/v1beta1/nodes endpoint")
}
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this should go in a separate PR since it seems unrelated to the sidecar test

Copy link
Contributor Author

@yangjunmyfm192085 yangjunmyfm192085 Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

sure, let me rollback the change

@yangjunmyfm192085
Copy link
Contributor Author

/cc @dgrisonnet
I will open a separate PR to solve the problem of pull-metrics-server-test-e2e-ha failure

@yangjunmyfm192085
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2023
@yangjunmyfm192085
Copy link
Contributor Author

/cc @serathius @dgrisonnet

@serathius
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: serathius, yangjunmyfm192085

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2023
@k8s-ci-robot k8s-ci-robot merged commit 144f9bc into kubernetes-sigs:master Dec 5, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev left review comments

@s-urbaniak s-urbaniak Awaiting requested review from s-urbaniak

@serathius serathius Awaiting requested review from serathius

@dgrisonnet dgrisonnet Awaiting requested review from dgrisonnet

Assignees

@serathius serathius

@dgrisonnet dgrisonnet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make sure metrics server works with SidecarContainers feature
6 participants
@yangjunmyfm192085 @serathius @dashpole @k8s-ci-robot @SergeyKanzhelev @dgrisonnet