feat(K8s native sidecar): Add support for Kubernetes native Sidecars #8052
Conversation
|
/kind feature |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
fee020c
to
f154502
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
Thanks @kgcarr for working on this. @tektoncd/core-maintainers, this PR is changing the way sidecars are implemented. With this PR, the sidecar containers are added as initcontainers with necessary specifications to align with k8s native sidecars. K8S introduced native sidecars in 1.29. Tekton CI is running on K8S 1.28. What is the best way to test these changes? Should we add an example or e2e test to run on k8s 1.29 for the changes introduced in this PR? or Should be rely on validations of necessary settings of RestartPolicy for now? |
Should we upgrade the dogfooding cluster to 1.29 so that we can have e2e tests for this? |
|
Thanks for the PR @kgcarr With this change, does it mean that we won't have to deal with |
|
@chitrangpatel That's correct, we no longer will need the |
@chitrangpatel Rather than relying on the version of the dogfooding cluster, since we run |
Yes, you're right ofcourse! We have a setup-kind thing that does this. |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
d3dc49b
to
b428541
Compare
pkg/pod/pod.go
Outdated
| sv, err := dc.ServerVersion() | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
I am not very familiar with the DiscoveryInterface(). I tried to find more information about it but could not determine whether it results in an additional API call. Since we have a feature flag for this functionality. I recommend placing it under the if condition that checks the feature flag.
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableKubernetesSidecar {
// Kubernetes Version
sv, err := b.KubeClient.Discovery().ServerVersion()
if err != nil {
return nil, err
}
// Check if current k8s version is less than 1.29
// Since Kubernetes Major version cannot be 0 and if it's 2 then sidecar will be in
// we are only concerned about major version 1 and if the minor is less than 29 then
// we need to do the current logic
svMinorInt, _ := strconv.Atoi(sv.Minor)
svMajorInt, _ := strconv.Atoi(sv.Major)
if svMajorInt >= 1 && svMinorInt >= SidecarK8sMinorVersionCheck {
// Add RestartPolicy and Merge into initContainer
for i := range sidecarContainers {
sc := &sidecarContainers[i]
always := corev1.ContainerRestartPolicyAlways
sc.RestartPolicy = &always
sc.Name = names.SimpleNameGenerator.RestrictLength(fmt.Sprintf("%v%v", sidecarPrefix, sc.Name))
mergedPodInitContainers = append(mergedPodInitContainers, *sc)
}
}
} else {
// Merge sidecar containers with step containers.
for _, sc := range sidecarContainers {
sc.Name = names.SimpleNameGenerator.RestrictLength(fmt.Sprintf("%v%v", sidecarPrefix, sc.Name))
mergedPodContainers = append(mergedPodContainers, sc)
}
}
There was a problem hiding this comment.
I did slightly different logic to handle the case where you set the enable-kubernetes-sidecar to 'true' but you're running on a kubernetes version less than 1.29
pkg/reconciler/taskrun/taskrun.go
Outdated
| svMinorInt, _ := strconv.Atoi(sv.Minor) | ||
| if config.FromContextOrDefaults(ctx).FeatureFlags.EnableKubernetesSidecar && svMajorInt >= 1 && svMinorInt >= 29 { | ||
| logger.Infof("Using Kubernetes Native Sidecars \n") | ||
| } else { |
There was a problem hiding this comment.
A similar recommendation to the one above: https://github.com/tektoncd/pipeline/pull/8052/files#r1676425921
go.mod
Outdated
| @@ -1,6 +1,7 @@ | |||
| module github.com/tektoncd/pipeline | |||
|
|
|||
| go 1.22 | |||
| go 1.22.3 | |||
There was a problem hiding this comment.
NIT: I think we typically avoid changing the Go version in the same PR as introducing a new feature.
|
Thanks, @kgcarr, for this PR. I appreciate the support from @chitrangpatel, @vdemeester, and @afrittoli as well. I have completed my review of this PR. From my understanding, these changes should not affect anyone unless the feature flag is enabled and Kubernetes 1.29 is deployed. Once we implement this feature in our deployment, we will report back on any performance improvements and errors, if they arise. @kgcarr I would like to follow up to ensure we maintain the current code coverage at the very least. See: #8052 (comment) /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pritidesai 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 |
1966f4a
to
55f8361
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
55f8361
to
a8faa17
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/retest |
| @@ -9,6 +9,7 @@ spec: | |||
| image: docker.io/library/ubuntu | |||
| script: | | |||
| echo "hello from sidecar" > /shared/message | |||
| sleep 2 | |||
There was a problem hiding this comment.
🤞 Hope this is not flaky.
| if svMajorInt >= 1 && svMinorInt >= 29 { | ||
| useTektonSidecar = false | ||
| logger.Infof("Using Kubernetes Native Sidecars \n") | ||
| } |
There was a problem hiding this comment.
The if clause needs Test coverage.
| for _, s := range pod.Status.InitContainerStatuses { | ||
| if IsContainerSidecar(s.Name) { | ||
| sidecarStatuses = append(sidecarStatuses, s) | ||
| } |
|
/lgtm |
|
/retest |
|
Integration tests failing with 😿 : |
|
/retest |
1 similar comment
|
/retest |
| @@ -2317,7 +2317,7 @@ status: | |||
| // Check actions | |||
| actions := clients.Kube.Actions() | |||
| if len(actions) != 2 || !actions[0].Matches("list", "configmaps") || !actions[1].Matches("watch", "configmaps") { | |||
| t.Errorf("expected 2 actions (list configmaps, and watch configmaps) created by the reconciler,"+ | |||
| t.Errorf("expected 3 actions (list configmaps, and watch configmaps) created by the reconciler,"+ | |||
There was a problem hiding this comment.
Need to fix this...adding it to #8128
This PR is for #7617
Changes
Kubernetes 1.29 introduced native sidecar support
To implement the native sidecar support, we added an optional
RestartPolicyto the sidecar struct. TheRestartPolicywill get set if we are using native Kubernetes sidecars and the sidecar container will be added to the initcontainer list instead of the container list.Need to document that a
startupProbeon the sidecar needs to be used for the containers to wait for the sidecar to be readySubmitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes