Add GracefulNodeShutdown e2e test #98658

wzshiming · 2021-02-01T10:12:55Z

What type of PR is this?
/kind testing

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

k8s-ci-robot · 2021-02-01T10:12:56Z

@wzshiming: The label(s) kind/testing cannot be applied, because the repository doesn't have them

In response to this:

What type of PR is this?
/kind testing

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
NONE
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

wzshiming · 2021-02-01T10:27:58Z

/assign @bobbypage

bobbypage · 2021-02-03T01:40:07Z

test/e2e_node/node_shutdown_linux_test.go

+	"k8s.io/kubernetes/test/e2e/framework"
+)
+
+var _ = framework.KubeDescribe("[NodeFeature:GracefulNodeShutdown]", func() {


this might need some other tags, e.g. [Serial] since it modifies node status?

xref: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-testing/e2e-tests.md#kinds-of-tests

bobbypage · 2021-02-03T01:42:28Z

test/e2e_node/node_shutdown_linux_test.go

+	"k8s.io/kubernetes/test/e2e/framework"
+)
+
+var _ = framework.KubeDescribe("[NodeFeature:GracefulNodeShutdown]", func() {


the test naming convention appears to be

Feature [Label] [Label] [Label]

I believe the labels are used to filter which test suite the test will run in.

e.g. https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/eviction_test.go#L69

let's follow the convention: so maybe something like GracefulNodeShutdown [Serial] [NodeFeature:GracefulNodeShutdown

bobbypage · 2021-02-03T01:45:51Z

test/e2e_node/node_shutdown_linux_test.go

+			initialConfig.ShutdownGracePeriodCriticalPods = metav1.Duration{Duration: 10 * time.Second}
+		})
+
+		ginkgo.It("Normal shutdown", func() {


I think ginkgo is suppose to follow BDD style, so maybe something like:

.It("should be able to gracefully shutdown pods with various grace periods")

bobbypage · 2021-02-03T01:47:23Z

test/e2e_node/node_shutdown_linux_test.go

+
+var _ = framework.KubeDescribe("[NodeFeature:GracefulNodeShutdown]", func() {
+	f := framework.NewDefaultFramework("graceful-node-shutdown")
+	ginkgo.Context("Graceful node shutdown", func() {


Following BDD style: when gracefully shutting down

bobbypage · 2021-02-03T01:52:49Z

test/e2e_node/node_shutdown_linux_test.go

+				getGracePeriodOverrideTestPod("period-critical-120", nodeName, 120, true),
+				getGracePeriodOverrideTestPod("period-critical-5", nodeName, 5, true),
+			}
+


let's add some verbose logging of the various main steps the test does to make debugging failure easier in future. The pattern seems to be to use ginkgo.By, e.g. https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/docker_test.go#L60

I added some suggestions of places to log some of the important steps in the test.

bobbypage · 2021-02-03T02:05:20Z

test/e2e_node/node_shutdown_linux_test.go

+	}
+	if critical {
+		pod.ObjectMeta.Annotations = map[string]string{
+			kubelettypes.ConfigSourceAnnotationKey: kubelettypes.FileSource,


curious what this is / why is it needed?

kubernetes/test/e2e_node/critical_pod_test.go

Lines 152 to 162 in 7655bad

if critical {

pod.ObjectMeta.Namespace = kubeapi.NamespaceSystem

pod.ObjectMeta.Annotations = map[string]string{

kubelettypes.ConfigSourceAnnotationKey: kubelettypes.FileSource,

}

pod.Spec.PriorityClassName = scheduling.SystemNodeCritical

framework.ExpectEqual(kubelettypes.IsCriticalPod(pod), true, "pod should be a critical pod")

} else {

framework.ExpectEqual(kubelettypes.IsCriticalPod(pod), false, "pod should not be a critical pod")

}

kubelettypes.IsCriticalPod judged based on this key to determine whether this pod is critical

bobbypage · 2021-02-03T02:07:34Z

test/e2e_node/node_shutdown_linux_test.go

+	sleep 300
+}
+trap _term SIGTERM 
+sleep 300


pod will exist if test somehow gets stuck and takes a while (> than 300s). maybe bump up from 300s to say 10mins or something like that? not sure what's reasonable time for test to execute :)

bobbypage · 2021-02-03T02:11:06Z

test/e2e_node/node_shutdown_linux_test.go

+	return err
+}
+
+func getNodeReadyStatus(f *framework.Framework) bool {


you should be able to use getNode which already does this:

https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/e2e_node_suite_test.go#L314

I think all you need is something like this:
https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/e2e_node_suite_test.go#L273-L280

(or maybe above when you check the node status you can use waitForNodeReady directly and remove this function?

getNode may not be used because it is not compatible with framework.Framework

waitForNodeReady is also not needed. this case not only requires waiting for node ready but also waiting for node not ready.

bobbypage · 2021-02-03T02:12:14Z

test/e2e_node/node_shutdown_linux_test.go

+}
+
+func emitSignalPrepareForShutdown(b bool) error {
+	cmd := "gdbus emit --system --object-path /org/freedesktop/login1 --signal org.freedesktop.login1.Manager.PrepareForShutdown " + strconv.FormatBool(b)


let's leave a comment describing what this does:

// Emits a fake PrepareForShutdown dbus message on system dbus. Will cause kubelet to react to an active shutdown event.

bobbypage · 2021-02-03T02:18:01Z

test/e2e_node/node_shutdown_linux_test.go

+						"node did not become shutdown as expected",
+					)
+				} else {
+					time.Sleep(time.Second)


let's get rid of the sleep and rely on gomega.Eventually

bobbypage · 2021-02-03T02:35:09Z

Thanks so much for starting on the tests! I left some comments above.

Another thing that would be great to add is if we could also test somehow that the pod itself actually received the proper grace period when it was shutdown. I'm not sure the best way to test that, maybe something like the pod prints a timestamp every second upon getting SIGTERM and then getting the delta between first and last timestamp? Maybe there's a better way to check that... We can do that as followup, I think most important is to get basic test in place first and then we can evolve it :)

bobbypage · 2021-02-03T06:48:12Z

Thanks for quick updates. Were you able to test this locally? I ran the test via e2e remote:

make test-e2e-node FOCUS="GracefulNodeShutdown" REMOTE=true DELETE_INSTANCES=false PARALLELISM=1 SKIP="" RUNTIME=remote CLEANUP=false

and it failed with a timeout:

• Failure [23.170 seconds]
[k8s.io] GracefulNodeShutdown [Serial] [NodeFeature:GracefulNodeShutdown]
/home/porterdavid_google_com/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/framework.go:635
  when gracefully shutting down
  _output/local/go/src/k8s.io/kubernetes/test/e2e_node/node_shutdown_linux_test.go:42
    should be able to handle a cancelled shutdown [It]
    _output/local/go/src/k8s.io/kubernetes/test/e2e_node/node_shutdown_linux_test.go:124

    Timed out after 3.000s.
    Expected
        <*errors.errorString | 0xc001744bf0>: {
            s: "node did not become shutdown as expected",
        }
    to be nil

    _output/local/go/src/k8s.io/kubernetes/test/e2e_node/node_shutdown_linux_test.go:134

oomichi

/cc @oomichi

oomichi · 2021-02-01T20:26:16Z

test/e2e_node/node_shutdown_linux_test.go

+					pod.Status.Phase == v1.PodRunning,
+					true,
+					"pod is not ready",
+				)


nit: framework.ExpectEqual(pod.Status.Phase, v1.PodRunning, "pod is not ready")

oomichi · 2021-02-01T20:28:27Z

test/e2e_node/node_shutdown_linux_test.go

+						pod.Status.Phase == v1.PodRunning,
+						true,
+						"critical pod should not be shutdown",
+					)


nit: framework.ExpectEqual(pod.Status.Phase, v1.PodRunning, "critical pod should not be shutdown")

oomichi · 2021-02-16T21:05:10Z

test/e2e_node/node_shutdown_linux_test.go

+				for _, pod := range list.Items {
+					if kubelettypes.IsCriticalPod(&pod) {
+						if pod.Status.Phase != v1.PodRunning {
+							return fmt.Errorf("critical pod should not be shutdown")


It is better to output actual pod.Status.Phase in error message for debugging.

oomichi · 2021-02-16T21:05:21Z

test/e2e_node/node_shutdown_linux_test.go

+						}
+					} else {
+						if pod.Status.Phase != v1.PodFailed || pod.Status.Reason != "Shutdown" {
+							return fmt.Errorf("pod should be shutdown")


ditto: pod.Status.Phase

oomichi · 2021-02-16T21:06:20Z

test/e2e_node/node_shutdown_linux_test.go

+
+				for _, pod := range list.Items {
+					if pod.Status.Phase != v1.PodFailed || pod.Status.Reason != "Shutdown" {
+						return fmt.Errorf("pod should be shutdown")


ditto: pod.Status.Phase

wzshiming · 2021-02-18T06:47:43Z

@oomichi Thanks, updated

wzshiming · 2021-02-18T17:40:01Z

/retest

bobbypage · 2021-02-19T00:03:16Z

/lgtm
/assign @mrunalp

SergeyKanzhelev · 2021-02-20T05:14:50Z

/triage accepted
/priority important-soon
(since the KEP is marked to go to beta in 1.21)

SergeyKanzhelev · 2021-02-20T05:15:02Z

KEP: kubernetes/enhancements#2000

wzshiming · 2021-02-20T05:23:23Z

/retest

oomichi · 2021-02-21T03:06:04Z

/approve

k8s-ci-robot · 2021-02-21T03:06:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oomichi, wzshiming

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:

~~test/e2e_node/OWNERS~~ [oomichi]

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

pacoxu · 2021-02-21T05:44:05Z

/retest

wzshiming · 2021-02-21T09:17:51Z

/retest

pacoxu · 2021-02-21T11:01:41Z

/retest
for flake 😓

fejta-bot · 2021-02-21T14:23:10Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

SergeyKanzhelev · 2021-02-22T05:44:58Z

You'd need to update the tests definition to include this test into regular runs.

ci-kubernetes-node-kubelet-alpha repels Serial, ci-kubernetes-node-kubelet-serial doesn't include Alpha features.

We may start with the new tab specifically for this test. Or remove the Alpha repellant from Serial tab - it doesn't seem there are more Alpha features to exclude.

k8s-ci-robot requested review from bart0sh and mrunalp February 1, 2021 10:14

wzshiming force-pushed the add-e2e-for-graceful-node-shutdown branch 2 times, most recently from b175de6 to 309281e Compare February 1, 2021 10:25

k8s-ci-robot assigned bobbypage Feb 1, 2021

wzshiming force-pushed the add-e2e-for-graceful-node-shutdown branch from 309281e to a501393 Compare February 1, 2021 10:39

wzshiming changed the title ~~[WIP] Add GracefulNodeShutdown e2e test~~ Add GracefulNodeShutdown e2e test Feb 1, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2021

wzshiming force-pushed the add-e2e-for-graceful-node-shutdown branch from a501393 to 1ce42f8 Compare February 1, 2021 11:34

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board Feb 1, 2021

SergeyKanzhelev moved this from Triage to PRs - Review in progress in SIG Node CI/Test Board Feb 1, 2021

bobbypage reviewed Feb 3, 2021

View reviewed changes

wzshiming force-pushed the add-e2e-for-graceful-node-shutdown branch 2 times, most recently from e9afe1d to 4cae803 Compare February 3, 2021 03:44

k8s-ci-robot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Feb 14, 2021

oomichi reviewed Feb 16, 2021

View reviewed changes

k8s-ci-robot requested a review from oomichi February 16, 2021 21:06

Add GracefulNodeShutdown e2e test

e367d2f

wzshiming force-pushed the add-e2e-for-graceful-node-shutdown branch from 59237c8 to e367d2f Compare February 18, 2021 06:47

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2021

k8s-ci-robot merged commit 89b9cf7 into kubernetes:master Feb 21, 2021

SIG Node CI/Test Board automation moved this from PRs - Review in progress to Done Feb 21, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 21, 2021

wzshiming deleted the add-e2e-for-graceful-node-shutdown branch February 22, 2021 06:49

wzshiming mentioned this pull request Feb 22, 2021

Provide jobs for e2e-node graceful node shutdown tests kubernetes/test-infra#20937

Merged

arunmk mentioned this pull request Mar 4, 2021

Graceful node shutdown kubernetes/enhancements#2000

Open

	if critical {
	pod.ObjectMeta.Namespace = kubeapi.NamespaceSystem
	pod.ObjectMeta.Annotations = map[string]string{
	kubelettypes.ConfigSourceAnnotationKey: kubelettypes.FileSource,
	}
	pod.Spec.PriorityClassName = scheduling.SystemNodeCritical

	framework.ExpectEqual(kubelettypes.IsCriticalPod(pod), true, "pod should be a critical pod")
	} else {
	framework.ExpectEqual(kubelettypes.IsCriticalPod(pod), false, "pod should not be a critical pod")
	}

Add GracefulNodeShutdown e2e test #98658

Add GracefulNodeShutdown e2e test #98658

Conversation

wzshiming commented Feb 1, 2021

k8s-ci-robot commented Feb 1, 2021

wzshiming commented Feb 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzshiming Feb 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbypage commented Feb 3, 2021

bobbypage commented Feb 3, 2021

oomichi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzshiming commented Feb 18, 2021

wzshiming commented Feb 18, 2021

bobbypage commented Feb 19, 2021

SergeyKanzhelev commented Feb 20, 2021

SergeyKanzhelev commented Feb 20, 2021

wzshiming commented Feb 20, 2021

oomichi commented Feb 21, 2021

k8s-ci-robot commented Feb 21, 2021

pacoxu commented Feb 21, 2021

wzshiming commented Feb 21, 2021

pacoxu commented Feb 21, 2021

fejta-bot commented Feb 21, 2021

SergeyKanzhelev commented Feb 22, 2021

wzshiming Feb 3, 2021 •

edited