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

add featuregate for k8s 1.28 native sidecar container #2801

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

frzifus
Copy link
Member

@frzifus frzifus commented Mar 29, 2024

Description:

Link to tracking Issue(s):

Testing:

Documentation:

pkg/sidecar/pod.go Outdated Show resolved Hide resolved
@frzifus frzifus marked this pull request as ready for review April 2, 2024 10:24
@frzifus frzifus requested a review from a team April 2, 2024 10:24
@frzifus frzifus added the help wanted Extra attention is needed label Apr 2, 2024
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.

I think this makes sense... is there a way to write an e2e for this?

@swiatekm
Copy link
Contributor

swiatekm commented Apr 9, 2024

I think this makes sense... is there a way to write an e2e for this?

Sure, you'd need to add a new test suite and run it with the gate enabled. Then checking if the sidecar is an initContainer should be straightforward.

@frzifus
Copy link
Member Author

frzifus commented Jul 16, 2024

@swiatekm whats the best way to configure a k8s featuregate for a specific e2e test suite?

Do we need to create a new e.g. kind-1.30-native-sidecar.yaml and register it here?

kube-version:
- "1.23"
- "1.30"

Do we need then to create a new step like e2e-test-sidecar here too?

@frzifus frzifus requested a review from swiatekm July 16, 2024 17:18
@swiatekm
Copy link
Contributor

@swiatekm whats the best way to configure a k8s featuregate for a specific e2e test suite?

Do we need to create a new e.g. kind-1.30-native-sidecar.yaml and register it here?

kube-version:
- "1.23"
- "1.30"

Do we need then to create a new step like e2e-test-sidecar here too?

You should do the same thing we do for enabling other options in e2e tests. See here.

You do need to add a new entry to the e2e test matrix, because you need a separate operator deployment with the flag enabled. Then you add a make target that adds the command-line option to the operator Deployment (same as we do for other operator flags), and use that make target for setup in the e2e tests.

// It needs to be enabled with +featureGate=SidecarContainers.
// See:
// https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-alpha-or-beta-features
EnableNativeSidecarContainers = featuregate.GlobalRegistry().MustRegister(
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could on k8s 1.28+ always use the native sidecar pattern and keep using the existing approach on older k8s versions.

This way no feature flag is needed and we would always use a recommended approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do that, but we would expect different results in our e2e tests. Which makes it hart to execute the same tests on both platform versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm ok with that as well. I can't think of any issues it might cause.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, then I will enable it for the 1.28+ e2e tests. Would be nice it it makes it into v0.111.0.

@frzifus frzifus removed the help wanted Extra attention is needed label Oct 7, 2024
@frzifus frzifus requested a review from a team as a code owner October 10, 2024 16:32
@frzifus
Copy link
Member Author

frzifus commented Oct 14, 2024

Seems we dont need to enable it in Kind.
https://kubernetes.io/docs/concepts/workloads/pods/sidecar-containers/

Provided that your cluster has the SidecarContainers feature gate enabled (the feature is active by default since Kubernetes v1.29), you can specify a restartPolicy for containers listed in a Pod's initContainers field.

pkg/sidecar/pod.go Outdated Show resolved Hide resolved
pkg/sidecar/pod.go Outdated Show resolved Hide resolved
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
pkg/sidecar/pod.go Outdated Show resolved Hide resolved
pkg/sidecar/pod_test.go Outdated Show resolved Hide resolved
containers:
- name: myapp
initContainers:
- name: otc-container
Copy link
Contributor

Choose a reason for hiding this comment

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

should we verify that this has the correct restart policy?

tests/e2e-native-sidecar/00-install.yaml Outdated Show resolved Hide resolved
@frzifus frzifus requested a review from jaronoff97 October 14, 2024 20:24
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
component: pkg/sidecar
Copy link
Member

Choose a reason for hiding this comment

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

collector

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
Copy link
Member

Choose a reason for hiding this comment

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

Could you please document here the feature gate as a flag e.g. --. Could you please document which k8s version support by default native sidecar?

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
@frzifus
Copy link
Member Author

frzifus commented Oct 15, 2024

Updated the changelog @pavolloffay

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM,

just some docs nits

.github/workflows/e2e.yaml Outdated Show resolved Hide resolved
pkg/featuregate/featuregate.go Outdated Show resolved Hide resolved
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
@frzifus
Copy link
Member Author

frzifus commented Oct 16, 2024

Changed it to k8s 1.29 and we now mention in the fg desc that it should only be enabled on k8s v1.29+.

@frzifus frzifus requested a review from pavolloffay October 16, 2024 07:21
@pavolloffay pavolloffay merged commit 9a1b585 into open-telemetry:main Oct 16, 2024
36 checks passed
@frzifus frzifus deleted the native_sidecar branch October 16, 2024 07:45
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.

Use k8s 1.28 native sidecar for OTEL collector sidecar mode
4 participants