KEP 753: SidecarContainers is GA

[SergeyKanzhelev]

• One-line PR description:

SidecarContainer feature is going GA.

• Issue link:

#753

• Other comments:

All bugs listed were reviewed and are not critical for GA promotion.

[mrunalp]

/lgtm

[kannon92]

PRR shadow:

Are there any tests for feature enablement/disablement?
Not yet, but it is planned/required to add them before graduation to Beta.

Can you update this to reflect the reality for GA?

https://github.com/kubernetes/enhancements/blob/ee4c27a15a9f8107deb2831654e4054314e0b7bb/keps/sig-node/753-sidecar-containers/README.md#what-are-other-known-failure-modes

I think the TBD should be filled out here as part of the GA promotion.

[toVersus]

Can you update this to reflect the reality for GA?

It looks like there are no unit tests for enabling/disabling the SidecarContainers feature. Most likely, a test like the following should have been added to pkg/registry/core/pod/strategy_test.go.

TestSidecarContainersEnablement

func TestSidecarContainersEnablement(t *testing.T) {
	containerRestartPolicyAlways := api.ContainerRestartPolicyAlways
	defaultTerminationGracePeriodSeconds := int64(30)

	getInitContainerRestartPolicy := func(pod *api.Pod) *api.ContainerRestartPolicy {
		return pod.Spec.InitContainers[0].RestartPolicy
	}

	podWithInitContainerRestartPolicy := func() *api.Pod {
		return &api.Pod{
			ObjectMeta: metav1.ObjectMeta{
				Namespace:       "default",
				Name:            "foo",
				ResourceVersion: "1",
			},
			Spec: api.PodSpec{
				DNSPolicy:     api.DNSDefault,
				RestartPolicy: api.RestartPolicyAlways,
				InitContainers: []api.Container{
					{
						Name:                     "restartable1",
						Image:                    "image",
						ImagePullPolicy:          "IfNotPresent",
						TerminationMessagePolicy: "File",
						RestartPolicy:            &containerRestartPolicyAlways,
					},
				},
				Containers: []api.Container{
					{
						Name:                     "regular1",
						Image:                    "image",
						ImagePullPolicy:          "IfNotPresent",
						TerminationMessagePolicy: "File",
					},
				},
				TerminationGracePeriodSeconds: &defaultTerminationGracePeriodSeconds,
			},
		}
	}

	podWithoutInitContainerRestartPolicy := func() *api.Pod {
		return &api.Pod{
			ObjectMeta: metav1.ObjectMeta{
				Namespace:       "default",
				Name:            "foo",
				ResourceVersion: "1",
			},
			Spec: api.PodSpec{
				DNSPolicy:     api.DNSDefault,
				RestartPolicy: api.RestartPolicyAlways,
				InitContainers: []api.Container{
					{
						Name:                     "restartable1",
						Image:                    "image",
						ImagePullPolicy:          "IfNotPresent",
						TerminationMessagePolicy: "File",
					},
				},
				Containers: []api.Container{
					{
						Name:                     "regular1",
						Image:                    "image",
						ImagePullPolicy:          "IfNotPresent",
						TerminationMessagePolicy: "File",
					},
				},
				TerminationGracePeriodSeconds: &defaultTerminationGracePeriodSeconds,
			},
		}
	}

	testCases := []struct {
		description string
		gateEnabled bool
		newPod      *api.Pod
		wantPod     *api.Pod
	}{
		{
			description: "gate enabled, creating pods with init container's restart policy",
			gateEnabled: true,
			newPod:      podWithInitContainerRestartPolicy(),
			wantPod:     podWithInitContainerRestartPolicy(),
		},
		{
			description: "gate disabled, creating pods with init container's restart policy",
			gateEnabled: false,
			newPod:      podWithInitContainerRestartPolicy(),
			wantPod:     podWithoutInitContainerRestartPolicy(),
		},
	}

	for _, tc := range testCases {
		t.Run(tc.description, func(t *testing.T) {
			featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, tc.gateEnabled)

			newPod := tc.newPod

			Strategy.PrepareForCreate(genericapirequest.NewContext(), newPod)
			if errs := Strategy.Validate(genericapirequest.NewContext(), newPod); len(errs) != 0 {
				t.Errorf("Unexpected error: %v", errs.ToAggregate())
			}

			if diff := cmp.Diff(getInitContainerRestartPolicy(newPod), getInitContainerRestartPolicy(tc.wantPod)); diff != "" {
				t.Fatalf("Unexpected modification to container restart policy; diff (-got +want)\n%s", diff)
			}
		})
	}
}

However, once the SidecarContainers feature reaches GA and is locked to default in kubernetes/kubernetes#129731, the above test will no longer work and will need to be removed within that PR. I also looked at other KEPs, and in most cases, there are no unit tests for feature enablement/disablement. So, would simply updating the description be sufficient?

Edit: As shown in kubernetes/kubernetes#129731, using featuregatetesting.SetFeatureGateEmulationVersionDuringTest would have allowed us to keep the above test even after it graduated to GA. However, since there is already a test for enabling and disabling the SidecarContainers feature here, the above test case may not be necessary.

[toVersus]

I think the TBD should be filled out here as part of the GA promotion.

Two out of the three are already covered by E2E tests. So, should we just add links to the E2E tests?

• Main containers don't exit within TGPS, leading to sidecars being terminated
  • https://github.com/kubernetes/kubernetes/blob/b4f902f0371485505ff4eda39975e67bfa9b0727/test/e2e_node/container_lifecycle_test.go#L4977-L5077
• Main container or sidecar use a preStop hook consuming TGPS, leading to remaining sidecars being terminated
  • https://github.com/kubernetes/kubernetes/blob/b4f902f0371485505ff4eda39975e67bfa9b0727/test/e2e_node/container_lifecycle_test.go#L4272-L4408

Sidecar container uses a preStop hook that make the container exit during Pod shutdown, sidecar is restarted, leading to a CrashLoopBackOff

The last one is about unintended use case. I think that explaining it in the Sidecar Containers's best practice would make implementing a test in Kubernetes unnecessary. Writing a test for this could be a bit tricky, but if it's absolutely necessary, I can look into whether it can be implemented.

[SergeyKanzhelev]

thanks all, updated!

[jpbetz]

/approve
For PRR

[swatisehgal]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, mrunalp, SergeyKanzhelev, swatisehgal

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:

• keps/prod-readiness/OWNERS [jpbetz]
• keps/sig-node/OWNERS [mrunalp]

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