Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@AxeZhan
AxeZhan committed Feb 16, 2024
1 parent f055513 commit eaf7f99
Show file tree
Hide file tree
Showing 5 changed files with 250 additions and 138 deletions.
290 changes: 207 additions & 83 deletions pkg/api/pod/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3474,110 +3474,234 @@ func TestDropClusterTrustBundleProjectedVolumes(t *testing.T) {
}

func TestDropPodLifecycleSleepAction(t *testing.T) {
container := api.Container{
Name: "foo",
makeSleepHandler := func() *api.LifecycleHandler {
return &api.LifecycleHandler{
Sleep: &api.SleepAction{Seconds: 1},
}
}

containerWithHandler := func() api.Container {
return api.Container{
Name: "foo",
Lifecycle: &api.Lifecycle{
PreStop: &api.LifecycleHandler{
Sleep: &api.SleepAction{Seconds: 1},
},
},
makeExecHandler := func() *api.LifecycleHandler {
return &api.LifecycleHandler{
Exec: &api.ExecAction{Command: []string{"foo"}},
}
}

podWithHandler := func() *api.Pod {
return &api.Pod{
Spec: api.PodSpec{
Containers: []api.Container{
containerWithHandler(),
},
},
makeHTTPGetHandler := func() *api.LifecycleHandler {
return &api.LifecycleHandler{
HTTPGet: &api.HTTPGetAction{Host: "foo"},
}
}

podWithoutHandler := func() *api.Pod {
return &api.Pod{
Spec: api.PodSpec{
Containers: []api.Container{
container,
},
},
makeContainer := func(preStop, postStart *api.LifecycleHandler) api.Container {
container := api.Container{Name: "foo"}
if preStop != nil || postStart != nil {
container.Lifecycle = &api.Lifecycle{
PostStart: postStart,
PreStop: preStop,
}
}
return container
}

podInfo := []struct {
description string
hasSleepAction bool
pod func() *api.Pod
makeEphemeralContainer := func(preStop, postStart *api.LifecycleHandler) api.EphemeralContainer {
container := api.EphemeralContainer{
EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "foo"},
}
if preStop != nil || postStart != nil {
container.Lifecycle = &api.Lifecycle{
PostStart: postStart,
PreStop: preStop,
}
}
return container
}

makePod := func(containers []api.Container, initContainers []api.Container, ephemeralContainers []api.EphemeralContainer) *api.PodSpec {
return &api.PodSpec{
Containers: containers,
InitContainers: initContainers,
EphemeralContainers: ephemeralContainers,
}
}

testCases := []struct {
gateEnabled bool
oldLifecycleHandler *api.LifecycleHandler
newLifecycleHandler *api.LifecycleHandler
expectLifecycleHandler *api.LifecycleHandler
}{
// nil -> nil
{
gateEnabled: false,
oldLifecycleHandler: nil,
newLifecycleHandler: nil,
expectLifecycleHandler: nil,
},
{
gateEnabled: true,
oldLifecycleHandler: nil,
newLifecycleHandler: nil,
expectLifecycleHandler: nil,
},
// nil -> exec
{
gateEnabled: false,
oldLifecycleHandler: nil,
newLifecycleHandler: makeExecHandler(),
expectLifecycleHandler: makeExecHandler(),
},
{
gateEnabled: true,
oldLifecycleHandler: nil,
newLifecycleHandler: makeExecHandler(),
expectLifecycleHandler: makeExecHandler(),
},
// nil -> sleep
{
gateEnabled: false,
oldLifecycleHandler: nil,
newLifecycleHandler: makeSleepHandler(),
expectLifecycleHandler: nil,
},
{
gateEnabled: true,
oldLifecycleHandler: nil,
newLifecycleHandler: makeSleepHandler(),
expectLifecycleHandler: makeSleepHandler(),
},
// exec -> exec
{
gateEnabled: false,
oldLifecycleHandler: makeExecHandler(),
newLifecycleHandler: makeExecHandler(),
expectLifecycleHandler: makeExecHandler(),
},
{
gateEnabled: true,
oldLifecycleHandler: makeExecHandler(),
newLifecycleHandler: makeExecHandler(),
expectLifecycleHandler: makeExecHandler(),
},
// exec -> http
{
gateEnabled: false,
oldLifecycleHandler: makeExecHandler(),
newLifecycleHandler: makeHTTPGetHandler(),
expectLifecycleHandler: makeHTTPGetHandler(),
},
{
gateEnabled: true,
oldLifecycleHandler: makeExecHandler(),
newLifecycleHandler: makeHTTPGetHandler(),
expectLifecycleHandler: makeHTTPGetHandler(),
},
// exec -> sleep
{
gateEnabled: false,
oldLifecycleHandler: makeExecHandler(),
newLifecycleHandler: makeSleepHandler(),
expectLifecycleHandler: nil,
},
{
gateEnabled: true,
oldLifecycleHandler: makeExecHandler(),
newLifecycleHandler: makeSleepHandler(),
expectLifecycleHandler: makeSleepHandler(),
},
// sleep -> exec
{
gateEnabled: false,
oldLifecycleHandler: makeSleepHandler(),
newLifecycleHandler: makeExecHandler(),
expectLifecycleHandler: makeExecHandler(),
},
{
description: "has sleep action",
hasSleepAction: true,
pod: podWithHandler,
gateEnabled: true,
oldLifecycleHandler: makeSleepHandler(),
newLifecycleHandler: makeExecHandler(),
expectLifecycleHandler: makeExecHandler(),
},
// sleep -> sleep
{
description: "does not have sleep action",
hasSleepAction: false,
pod: podWithoutHandler,
gateEnabled: false,
oldLifecycleHandler: makeSleepHandler(),
newLifecycleHandler: makeSleepHandler(),
expectLifecycleHandler: makeSleepHandler(),
},
{
description: "is nil",
hasSleepAction: false,
pod: func() *api.Pod { return nil },
gateEnabled: true,
oldLifecycleHandler: makeSleepHandler(),
newLifecycleHandler: makeSleepHandler(),
expectLifecycleHandler: makeSleepHandler(),
},
}

for _, enabled := range []bool{true, false} {
for _, oldPodInfo := range podInfo {
for _, newPodInfo := range podInfo {
oldPodHasSleepAction, oldPod := oldPodInfo.hasSleepAction, oldPodInfo.pod()
newPodHasSleepAction, newPod := newPodInfo.hasSleepAction, newPodInfo.pod()
if newPod == nil {
continue
}
for i, tc := range testCases {
t.Run(fmt.Sprintf("test_%d", i), func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodLifecycleSleepAction, tc.gateEnabled)()

t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodLifecycleSleepAction, enabled)()

var oldPodSpec *api.PodSpec
if oldPod != nil {
oldPodSpec = &oldPod.Spec
}
dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)

// old pod should never be changed
if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
t.Errorf("old pod changed: %v", cmp.Diff(oldPod, oldPodInfo.pod()))
}

switch {
case enabled || oldPodHasSleepAction:
// new pod shouldn't change if feature enabled or if old pod has
// any sleep action
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod()))
}
case newPodHasSleepAction:
// new pod should be changed
if reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod was not changed")
}
// new pod should not have any sleep action
if !reflect.DeepEqual(newPod, podWithoutHandler()) {
t.Errorf("new pod has a sleep action: %v", cmp.Diff(newPod, podWithoutHandler()))
}
default:
// new pod should not need to be changed
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod()))
}
}
})
// preStop
// container
{
oldPod := makePod([]api.Container{makeContainer(tc.oldLifecycleHandler.DeepCopy(), nil)}, nil, nil)
newPod := makePod([]api.Container{makeContainer(tc.newLifecycleHandler.DeepCopy(), nil)}, nil, nil)
expectPod := makePod([]api.Container{makeContainer(tc.expectLifecycleHandler.DeepCopy(), nil)}, nil, nil)
dropDisabledFields(newPod, nil, oldPod, nil)
if diff := cmp.Diff(expectPod, newPod); diff != "" {
t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff)
}
}
}
// InitContainer
{
oldPod := makePod(nil, []api.Container{makeContainer(tc.oldLifecycleHandler.DeepCopy(), nil)}, nil)
newPod := makePod(nil, []api.Container{makeContainer(tc.newLifecycleHandler.DeepCopy(), nil)}, nil)
expectPod := makePod(nil, []api.Container{makeContainer(tc.expectLifecycleHandler.DeepCopy(), nil)}, nil)
dropDisabledFields(newPod, nil, oldPod, nil)
if diff := cmp.Diff(expectPod, newPod); diff != "" {
t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff)
}
}
// EphemeralContainer
{
oldPod := makePod(nil, nil, []api.EphemeralContainer{makeEphemeralContainer(tc.oldLifecycleHandler.DeepCopy(), nil)})
newPod := makePod(nil, nil, []api.EphemeralContainer{makeEphemeralContainer(tc.newLifecycleHandler.DeepCopy(), nil)})
expectPod := makePod(nil, nil, []api.EphemeralContainer{makeEphemeralContainer(tc.expectLifecycleHandler.DeepCopy(), nil)})
dropDisabledFields(newPod, nil, oldPod, nil)
if diff := cmp.Diff(expectPod, newPod); diff != "" {
t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff)
}
}
// postStart
// container
{
oldPod := makePod([]api.Container{makeContainer(nil, tc.oldLifecycleHandler.DeepCopy())}, nil, nil)
newPod := makePod([]api.Container{makeContainer(nil, tc.newLifecycleHandler.DeepCopy())}, nil, nil)
expectPod := makePod([]api.Container{makeContainer(nil, tc.expectLifecycleHandler.DeepCopy())}, nil, nil)
dropDisabledFields(newPod, nil, oldPod, nil)
if diff := cmp.Diff(expectPod, newPod); diff != "" {
t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff)
}
}
// InitContainer
{
oldPod := makePod(nil, []api.Container{makeContainer(nil, tc.oldLifecycleHandler.DeepCopy())}, nil)
newPod := makePod(nil, []api.Container{makeContainer(nil, tc.newLifecycleHandler.DeepCopy())}, nil)
expectPod := makePod(nil, []api.Container{makeContainer(nil, tc.expectLifecycleHandler.DeepCopy())}, nil)
dropDisabledFields(newPod, nil, oldPod, nil)
if diff := cmp.Diff(expectPod, newPod); diff != "" {
t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff)
}
}
// EphemeralContainer
{
oldPod := makePod(nil, nil, []api.EphemeralContainer{makeEphemeralContainer(nil, tc.oldLifecycleHandler.DeepCopy())})
newPod := makePod(nil, nil, []api.EphemeralContainer{makeEphemeralContainer(nil, tc.newLifecycleHandler.DeepCopy())})
expectPod := makePod(nil, nil, []api.EphemeralContainer{makeEphemeralContainer(nil, tc.expectLifecycleHandler.DeepCopy())})
dropDisabledFields(newPod, nil, oldPod, nil)
if diff := cmp.Diff(expectPod, newPod); diff != "" {
t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff)
}
}
})
}
}
5 changes: 1 addition & 4 deletions pkg/kubelet/metrics/metrics.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -951,10 +951,7 @@ func Register(collectors ...metrics.StableCollector) {
}

legacyregistry.MustRegister(LifecycleHandlerHTTPFallbacks)

if utilfeature.DefaultFeatureGate.Enabled(features.PodLifecycleSleepAction) {
legacyregistry.MustRegister(LifecycleHandlerSleepTerminated)
}
legacyregistry.MustRegister(LifecycleHandlerSleepTerminated)
})
}

Expand Down

0 comments on commit eaf7f99

Please sign in to comment.