add non graceful shutdown integration test #119478

sonasingh46 · 2023-07-20T14:42:59Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds integration test for node out of service detach feature.
Ref feature PR: #108486

Does this PR introduce a user-facing change?

None

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

- KEP:  https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/2268-non-graceful-shutdown

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

k8s-ci-robot · 2023-07-20T14:43:09Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

test/integration/volume/attach_detach_test.go

xing-yang · 2023-07-21T19:16:57Z

/retest

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

aojea · 2023-07-24T19:11:12Z

test/integration/volume/attach_detach_test.go

+		t.Fatalf("error in deleting pod: %v", err)
+	}
+	waitForPodDeletionTimeStampToSet(t, testClient, pod.Name, namespaceName)
+	waitForMetric(t, metrics.ForceDetachMetricCounter.WithLabelValues(metrics.ForceDetachReasonOutOfService), 1, "detach-metrics")


metrics are not thread safe, if there are other tests in the same binary they will use the same global registry, please check there is no possibility of test pollution, I can't remember if the metrics registry is already initialized when the apiserver starts

Looking into it.

Thanks for the heads up re the metrics.
I have seen existing tests using metrics with gotCountofEvent>=expectedCountOfEvent for similar reason and waitForMetric function does the same.
cc @xing-yang

aojea · 2023-07-24T19:13:26Z

test/integration/volume/attach_detach_test.go

+		pod, err := testingClient.CoreV1().Pods(podNamespace).Get(context.TODO(), podName, metav1.GetOptions{})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if pod.DeletionTimestamp != nil {
+			return true, nil
+		}


this will be racy if the pod does not have a finalizer because can be deleted and return a 404 Not found error, no?

The pod in question has been set a grace time period of 300 seconds in this test to allow enough time. So this should be fine. Let me know if you still have a different opinion.

sounds good, thanks for clarifying

aojea · 2023-07-24T19:14:54Z

test/integration/volume/attach_detach_test.go

+func waitForMetric(t *testing.T, m basemetric.CounterMetric, expectedCount float64, identifier string) {
+	if err := wait.Poll(100*time.Millisecond, 60*time.Second, func() (bool, error) {
+		gotCount, err := metricstestutil.GetCounterMetricValue(m)
+		fmt.Println(gotCount)


or this slipped from your local test or use t.Logf with a more meaninful message so it is debuggable

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

sonasingh46 · 2023-07-27T08:24:45Z

/retest

sonasingh46 · 2023-07-27T08:46:01Z

/retest

xing-yang · 2023-07-27T11:47:38Z

/assign @msau42 @gnufied

YuikoTakada

Thank you for your PR. Test scenario itself looks good. I've added some comments so please check them.

test/integration/volume/attach_detach_test.go

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

k8s-ci-robot · 2023-09-19T06:22:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sonasingh46
Once this PR has been reviewed and has the lgtm label, please ask for approval from msau42. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

test/OWNERS

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

YuikoTakada · 2023-09-19T12:13:47Z

@sonasingh46 Thank you for updating. Unfortunately, this failed on my local env.
error message is:

test/integration/volume/attach_detach_test.go:128:15: cannot use v1.ResourceRequirements{…} (value of type 
"k8s.io/kubernetes/vendor/k8s.io/api/core/v1".ResourceRequirements) as 
"k8s.io/kubernetes/vendor/k8s.io/api/core/v1".VolumeResourceRequirements value in struct literal

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

YuikoTakada · 2023-10-06T07:50:32Z

Thank you for updating. Looks good for me.

aojea · 2023-10-06T12:23:27Z

test/integration/volume/attach_detach_test.go

+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	go informers.Core().V1().Nodes().Informer().Run(ctx.Done())


I think that informers.Start() already starts this informer, is not this duplicate?

will remove this line.

aojea · 2023-10-06T12:25:40Z

test/integration/volume/attach_detach_test.go

+	// wait for volume to be attached
+	for i := 0; i < 10; i++ {
+		node, err = testClient.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{})
+		if err != nil {
+			t.Fatalf("Failed to get the node : %v", err)
+		}
+		if len(node.Status.VolumesAttached) > 1 {
+			break
+		}
+		time.Sleep(1 * time.Second)
+	}
+	if len(node.Status.VolumesAttached) < 1 {
+		t.Logf("failed to attach volume for pod %s on node %s", pod.Name, node.Name)


we have already libraries for doing these async checks

kubernetes/test/integration/apiserver/watchcache_test.go

Lines 78 to 83 in 854d0e7

if err := wait.Poll(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {

_, err := clientSet.CoreV1().Services("default").Get(ctx, "kubernetes", metav1.GetOptions{})

return err == nil, nil

}); err != nil {

t.Fatalf("Failed to wait for kubernetes service: %v:", err)

}

oh, you already using them below, why do you implement the loop to check here differently?

I will fix it. Missed this one. Thanks

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

sonasingh46 · 2023-10-12T15:15:21Z

/retest

sonasingh46 · 2023-10-12T16:58:47Z

looking into the test failures

xing-yang · 2023-11-01T01:45:06Z

/retest

k8s-ci-robot · 2023-11-01T02:44:06Z

@sonasingh46: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-integration	`c9bf834`	link	true	`/test pull-kubernetes-integration`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-triage-robot · 2024-01-30T03:08:43Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

xing-yang · 2024-01-30T04:07:49Z

/remove-lifecycle stale

k8s-ci-robot · 2024-03-02T13:11:30Z

PR needs rebase.

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.

xing-yang · 2024-04-09T15:34:19Z

Hi @sonasingh46, can you please rebase and address the CI failures? Thanks.

add non graceful shutdown integration test

bc8be2f

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 20, 2023

k8s-ci-robot requested review from aojea and dchen1107 July 20, 2023 14:44

xing-yang reviewed Jul 20, 2023

View reviewed changes

test/integration/volume/attach_detach_test.go Outdated Show resolved Hide resolved

test/integration/volume/attach_detach_test.go Show resolved Hide resolved

test/integration/volume/attach_detach_test.go Show resolved Hide resolved

xing-yang reviewed Jul 20, 2023

View reviewed changes

test/integration/volume/attach_detach_test.go Show resolved Hide resolved

xing-yang reviewed Jul 20, 2023

View reviewed changes

test/integration/volume/attach_detach_test.go Outdated Show resolved Hide resolved

xing-yang reviewed Jul 20, 2023

View reviewed changes

test/integration/volume/attach_detach_test.go Outdated Show resolved Hide resolved

incorporate review comments

de7e52d

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 24, 2023

incorporate review comments

898a7a1

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

sonasingh46 force-pushed the nongraceful_shutdown_integration_test branch from a4e7b44 to 898a7a1 Compare July 24, 2023 19:04

sonasingh46 changed the title ~~[WIP]add non graceful shutdown integration test~~ add non graceful shutdown integration test Jul 24, 2023

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 Jul 24, 2023

aojea reviewed Jul 24, 2023

View reviewed changes

incorporate review comments

9b63711

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

sonasingh46 force-pushed the nongraceful_shutdown_integration_test branch from 6081207 to 9b63711 Compare July 27, 2023 07:47

k8s-ci-robot assigned gnufied and msau42 Jul 27, 2023

This was referenced Jul 27, 2023

add scale test for non graceful node shutdown #118848

Closed

Non-graceful node shutdown kubernetes/enhancements#2268

Closed

YuikoTakada reviewed Aug 4, 2023

View reviewed changes

test/integration/volume/attach_detach_test.go Show resolved Hide resolved

test/integration/volume/attach_detach_test.go Outdated Show resolved Hide resolved

incorporate review comments

23b80ff

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

sonasingh46 added 2 commits October 5, 2023 21:14

fix lint errors

39c65e9

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

Fix lint

98e375c

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

aojea reviewed Oct 6, 2023

View reviewed changes

incorporate review comments

c9bf834

Signed-off-by: Ashutosh Kumar <sonasingh46@gmail.com>

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 30, 2024

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 30, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add non graceful shutdown integration test #119478

add non graceful shutdown integration test #119478

sonasingh46 commented Jul 20, 2023 •

edited

k8s-ci-robot commented Jul 20, 2023

xing-yang commented Jul 21, 2023

aojea Jul 24, 2023

sonasingh46 Jul 26, 2023

sonasingh46 Jul 26, 2023

aojea Jul 24, 2023

sonasingh46 Jul 26, 2023

aojea Oct 6, 2023

aojea Jul 24, 2023

sonasingh46 commented Jul 27, 2023

sonasingh46 commented Jul 27, 2023

xing-yang commented Jul 27, 2023

YuikoTakada left a comment

k8s-ci-robot commented Sep 19, 2023

YuikoTakada commented Sep 19, 2023 •

edited

YuikoTakada commented Oct 6, 2023

aojea Oct 6, 2023

sonasingh46 Oct 10, 2023

aojea Oct 6, 2023

aojea Oct 6, 2023

sonasingh46 Oct 10, 2023

sonasingh46 commented Oct 12, 2023

sonasingh46 commented Oct 12, 2023

xing-yang commented Nov 1, 2023

k8s-ci-robot commented Nov 1, 2023

k8s-triage-robot commented Jan 30, 2024

xing-yang commented Jan 30, 2024

k8s-ci-robot commented Mar 2, 2024

xing-yang commented Apr 9, 2024

	if err := wait.Poll(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {
	_, err := clientSet.CoreV1().Services("default").Get(ctx, "kubernetes", metav1.GetOptions{})
	return err == nil, nil
	}); err != nil {
	t.Fatalf("Failed to wait for kubernetes service: %v:", err)
	}

add non graceful shutdown integration test #119478

Are you sure you want to change the base?

add non graceful shutdown integration test #119478

Conversation

sonasingh46 commented Jul 20, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Jul 20, 2023

xing-yang commented Jul 21, 2023

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

sonasingh46 commented Jul 27, 2023

sonasingh46 commented Jul 27, 2023

xing-yang commented Jul 27, 2023

YuikoTakada left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 19, 2023

YuikoTakada commented Sep 19, 2023 • edited

YuikoTakada commented Oct 6, 2023

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

sonasingh46 commented Oct 12, 2023

sonasingh46 commented Oct 12, 2023

xing-yang commented Nov 1, 2023

k8s-ci-robot commented Nov 1, 2023

k8s-triage-robot commented Jan 30, 2024

xing-yang commented Jan 30, 2024

k8s-ci-robot commented Mar 2, 2024

xing-yang commented Apr 9, 2024

sonasingh46 commented Jul 20, 2023 •

edited

YuikoTakada commented Sep 19, 2023 •

edited