Update daemonSet status even if syncDaemonSet fails #112127

gjkim42 · 2022-08-30T15:24:36Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently, if syncDaemonSet has an error, we cannot update the current status for the daemonSet. This PR ensures the Kubernetes updates daemonSet status even if syncDaemonSet fails.

Which issue(s) this PR fixes:

Fixes #
ref: #112010

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixed DaemonSet to update the status even if it fails to create a pod.

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

gjkim42 · 2022-08-31T03:12:58Z

/cc @alculquicondor

Could you PTAL? This PR is similar to #109694.

gjkim42 · 2022-08-31T03:13:13Z

/priority important-soon

alculquicondor · 2022-08-31T18:48:00Z

Please add integration and unit tests

atiratree

looks/functions well - just a few nits

pkg/controller/daemon/daemon_controller.go

pkg/controller/daemon/daemon_controller_test.go

test/integration/daemonset/daemonset_test.go

pkg/controller/daemon/daemon_controller.go

gjkim42 · 2022-10-11T13:59:04Z

looks/functions well - just a few nits

all done, thanks.

@soltysh
Could you PTAL this and #112737?

soltysh

This is so much better - thank you!
/lgtm
/approve

k8s-ci-robot · 2022-11-07T19:51:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gjkim42, soltysh

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:

~~pkg/controller/daemon/OWNERS~~ [soltysh]
~~test/integration/daemonset/OWNERS~~ [soltysh]

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

soltysh · 2022-11-07T19:52:33Z

/triage accepted

gjkim42 · 2022-11-08T13:40:32Z

test/integration/daemonset/util.go

+var _ admission.ValidationInterface = &fakePodFailAdmission{}
+
+type fakePodFailAdmission struct {
+	limitedPodNumber int
+	succeedPodsCount int
+}
+
+func (f *fakePodFailAdmission) Handles(operation admission.Operation) bool {
+	return operation == admission.Create
+}
+
+func (f *fakePodFailAdmission) Validate(ctx context.Context, attr admission.Attributes, o admission.ObjectInterfaces) (err error) {
+	if attr.GetKind().GroupKind() != api.Kind("Pod") {
+		return nil
+	}
+
+	if f.succeedPodsCount >= f.limitedPodNumber {
+		return fmt.Errorf("fakePodFailAdmission error")
+	}
+	f.succeedPodsCount++
+	return nil
+}


@aojea

I guess this function is not thread-safe.
Let me make a fix for this.

@gjkim42 given we're hours before the freeze date I don't want to block the queue, so I'm reverting this in #113733 when you'll be submitting a fix you'll need to include this PR as well.

As we don't have enough time to confirm this fix, I respect your decision.

liggitt · 2022-11-08T13:50:25Z

are we sure the increased failures are only a test issue, or should we back out the change until we can bring it in flake-free?

gjkim42 · 2022-11-08T14:04:27Z

$ go test -c -run=TestUpdateStatusDespitePodCreationFailure ./test/integration/daemonset/
$ stress ./daemonset.test -test.run=TestUpdateStatusDespitePodCreationFailure
...
3m0s: 86 runs so far, 9 failures (10.47%)
3m5s: 88 runs so far, 9 failures (10.23%)
  
/tmp/go-stress-20221108T225506-3322889609
I1108 22:57:02.386727  134048 etcd.go:107] starting etcd on http://127.0.0.1:45669
I1108 22:57:02.386802  134048 etcd.go:113] storing etcd data in: /tmp/integration_test_etcd_data3293147976
{"level":"warn","ts":"2022-11-08T22:57:02.513+0900","caller":"auth/store.go:1233","msg":"simple token is not cryptographically signed"}
I1108 22:57:03.036300  134048 serving.go:342] Generated self-signed cert (/tmp/test-integration-TestUpdateStatusDespitePodCreationFailure_TestUpdateStatusDespitePodCreationFailure_OnDelete4011098137/apiserver.crt, /tmp/test-integration-TestUpdateStatusDespitePodCreationFailure_TestUpdateStatusDespitePodCreationFailure_OnDelete4011098137/apiserver.key)
I1108 22:57:03.036315  134048 server.go:554] external host was not specified, using 192.168.0.80
I1108 22:57:03.425543  134048 shared_informer.go:273] Waiting for caches to sync for node_authorizer
I1108 22:57:03.426401  134048 plugins.go:158] Loaded 11 mutating admission controller(s) successfully in the following order: NamespaceLifecycle,LimitRanger,ServiceAccount,TaintNodesByCondition,Priority,DefaultTolerationSeconds,DefaultStorageClass,StorageObjectInUseProtection,RuntimeClass,DefaultIngressClass,MutatingAdmissionWebhook.
I1108 22:57:03.426410  134048 plugins.go:161] Loaded 12 validating admission controller(s) successfully in the following order: LimitRanger,ServiceAccount,PodSecurity,Priority,PersistentVolumeClaimResize,RuntimeClass,CertificateApproval,CertificateSigning,CertificateSubjectRestriction,ValidatingAdmissionPolicy,ValidatingAdmissionWebhook,ResourceQuota.
I1108 22:57:03.426682  134048 instance.go:266] Using reconciler: lease
I1108 22:57:03.494069  134048 instance.go:609] API group "internal.apiserver.k8s.io" is not enabled, skipping.
W1108 22:57:03.643933  134048 genericapiserver.go:652] Skipping API authentication.k8s.io/v1beta1 because it has no resources.
W1108 22:57:03.643955  134048 genericapiserver.go:652] Skipping API authentication.k8s.io/v1alpha1 because it has no resources.
W1108 22:57:03.646580  134048 genericapiser
…
3m10s: 91 runs so far, 10 failures (10.99%)

/tmp/go-stress-20221108T225506-1628729645
I1108 22:57:06.693933  134122 etcd.go:107] starting etcd on http://127.0.0.1:46509
I1108 22:57:06.694009  134122 etcd.go:113] storing etcd data in: /tmp/integration_test_etcd_data344882241
{"level":"warn","ts":"2022-11-08T22:57:06.758+0900","caller":"auth/store.go:1233","msg":"simple token is not cryptographically signed"}
I1108 22:57:07.248480  134122 serving.go:342] Generated self-signed cert (/tmp/test-integration-TestUpdateStatusDespitePodCreationFailure_TestUpdateStatusDespitePodCreationFailure_OnDelete864243084/apiserver.crt, /tmp/test-integration-TestUpdateStatusDespitePodCreationFailure_TestUpdateStatusDespitePodCreationFailure_OnDelete864243084/apiserver.key)
I1108 22:57:07.248495  134122 server.go:554] external host was not specified, using 192.168.0.80
I1108 22:57:07.468446  134122 shared_informer.go:273] Waiting for caches to sync for node_authorizer
I1108 22:57:07.469272  134122 plugins.go:158] Loaded 11 mutating admission controller(s) successfully in the following order: NamespaceLifecycle,LimitRanger,ServiceAccount,TaintNodesByCondition,Priority,DefaultTolerationSeconds,DefaultStorageClass,StorageObjectInUseProtection,RuntimeClass,DefaultIngressClass,MutatingAdmissionWebhook.
I1108 22:57:07.469284  134122 plugins.go:161] Loaded 12 validating admission controller(s) successfully in the following order: LimitRanger,ServiceAccount,PodSecurity,Priority,PersistentVolumeClaimResize,RuntimeClass,CertificateApproval,CertificateSigning,CertificateSubjectRestriction,ValidatingAdmissionPolicy,ValidatingAdmissionWebhook,ResourceQuota.
I1108 22:57:07.469558  134122 instance.go:266] Using reconciler: lease
I1108 22:57:07.532437  134122 instance.go:609] API group "internal.apiserver.k8s.io" is not enabled, skipping.
W1108 22:57:07.692444  134122 genericapiserver.go:652] Skipping API authentication.k8s.io/v1beta1 because it has no resources.
W1108 22:57:07.692464  134122 genericapiserver.go:652] Skipping API authentication.k8s.io/v1alpha1 because it has no resources.
W1108 22:57:07.694524  134122 genericapiserver
…
3m15s: 95 runs so far, 11 failures (11.58%)

I can reproduce this flake quite easily.

After cherrypicking #113732, it looks that the flake is resolved.

$ git cherry-pick 9ac7112906b24a516f53e86c62658cee8c9a4141 # PR 113732
$ go test -c -run=TestUpdateStatusDespitePodCreationFailure ./test/integration/daemonset/
$ stress ./daemonset.test -test.run=TestUpdateStatusDespitePodCreationFailure
5s: 0 runs so far, 0 failures
10s: 0 runs so far, 0 failures
15s: 0 runs so far, 0 failures
20s: 12 runs so far, 0 failures
25s: 12 runs so far, 0 failures
30s: 12 runs so far, 0 failures
35s: 12 runs so far, 0 failures
40s: 24 runs so far, 0 failures
45s: 24 runs so far, 0 failures
50s: 24 runs so far, 0 failures
55s: 32 runs so far, 0 failures
1m0s: 36 runs so far, 0 failures
1m5s: 36 runs so far, 0 failures
1m10s: 40 runs so far, 0 failures
1m15s: 48 runs so far, 0 failures
1m20s: 48 runs so far, 0 failures
1m25s: 52 runs so far, 0 failures
1m30s: 58 runs so far, 0 failures
1m35s: 60 runs so far, 0 failures
1m40s: 60 runs so far, 0 failures
1m45s: 65 runs so far, 0 failures
1m50s: 72 runs so far, 0 failures
1m55s: 72 runs so far, 0 failures
2m0s: 77 runs so far, 0 failures
2m5s: 84 runs so far, 0 failures
2m10s: 84 runs so far, 0 failures
2m15s: 89 runs so far, 0 failures
2m20s: 92 runs so far, 0 failures
2m25s: 96 runs so far, 0 failures
2m30s: 97 runs so far, 0 failures
2m35s: 104 runs so far, 0 failures

atiratree · 2022-11-08T14:22:07Z

I can confirm this as well, even though the flake is less common for me. With the proposed fix I cannot see it even after a larger number of runs (500+).

Also, when I compile the test with -race flag I can clearly see it fail because of that:

--- FAIL: TestUpdateStatusDespitePodCreationFailure (7.91s)
    --- FAIL: TestUpdateStatusDespitePodCreationFailure/TestUpdateStatusDespitePodCreationFailure_OnDelete (7.91s)
        testing.go:1319: race detected during execution of test
    testing.go:1319: race detected during execution of test
FAIL

after the fix, the race is not detected anymore

gjkim42 · 2022-11-08T14:25:07Z

I can confirm this as well, even though the flake is less common for me.

$ git diff
diff --git a/test/integration/daemonset/daemonset_test.go b/test/integration/daemonset/daemonset_test.go
index 587beb997a2..98de4061121 100644
--- a/test/integration/daemonset/daemonset_test.go
+++ b/test/integration/daemonset/daemonset_test.go
@@ -996,7 +996,7 @@ func TestUnschedulableNodeDaemonDoesLaunchPod(t *testing.T) {
 
 func TestUpdateStatusDespitePodCreationFailure(t *testing.T) {
        forEachStrategy(t, func(t *testing.T, strategy *apps.DaemonSetUpdateStrategy) {
-               limitedPodNumber := 2
+               limitedPodNumber := 4
                // use framework.StartTestServer to inject admission control
                // e.g. to emulate the resource quota behavior

FYI, I boosted the flake by increasing replicas a bit.

k8s-ci-robot requested review from smarterclayton and soltysh August 30, 2022 15:25

gjkim42 force-pushed the update-status-despite-error branch from 43c0227 to 7cf7867 Compare August 31, 2022 03:05

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 31, 2022

gjkim42 changed the title ~~WIP: Update daemonSet status even if syncDaemonSet fails~~ Update daemonSet status even if syncDaemonSet fails Aug 31, 2022

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 Aug 31, 2022

k8s-ci-robot requested a review from alculquicondor August 31, 2022 03:12

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 31, 2022

gjkim42 force-pushed the update-status-despite-error branch 2 times, most recently from c9a3342 to 9273287 Compare August 31, 2022 03:22

Shubham82 mentioned this pull request Sep 2, 2022

Mention bug in StatefulSets and DaemonSets where pod count in status can be invalid kubernetes/website#36527

Closed

gjkim42 force-pushed the update-status-despite-error branch from 9273287 to e429045 Compare September 2, 2022 13:21

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 2, 2022

gjkim42 force-pushed the update-status-despite-error branch from c679c18 to 7a1fc25 Compare September 26, 2022 13:29

atiratree reviewed Oct 10, 2022

View reviewed changes

pkg/controller/daemon/daemon_controller.go Outdated Show resolved Hide resolved

Update daemonSet status even if syncDaemonSet fails

2ee024a

gjkim42 force-pushed the update-status-despite-error branch from 7a1fc25 to 2ee024a Compare October 11, 2022 13:56

soltysh approved these changes Nov 7, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 7, 2022

k8s-ci-robot merged commit b4f4286 into kubernetes:master Nov 8, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

gjkim42 deleted the update-status-despite-error branch November 8, 2022 00:08

aojea mentioned this pull request Nov 8, 2022

[Flaky test] k8s.io/kubernetes/test/integration/daemonset #113036

Closed

gjkim42 commented Nov 8, 2022

View reviewed changes

soltysh mentioned this pull request Nov 8, 2022

Revert "Update daemonSet status even if syncDaemonSet fails" #113733

Merged

gjkim42 mentioned this pull request Nov 9, 2022

Update daemonSet status even if syncDaemonSet fails #113787

Merged

alculquicondor mentioned this pull request Nov 11, 2022

daemonset.status.desiredNumberScheduled=0 for some time in v1.26.0-beta.0 #113844

Closed

This was referenced Dec 15, 2022

Statefuleset/DaemonSet reports invalid status due to ResourceQuota. #112010

Closed

Removed the reverted changes from Changelogv1.26.md #114602

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update daemonSet status even if syncDaemonSet fails #112127

Update daemonSet status even if syncDaemonSet fails #112127

gjkim42 commented Aug 30, 2022 •

edited

gjkim42 commented Aug 31, 2022

gjkim42 commented Aug 31, 2022

alculquicondor commented Aug 31, 2022

atiratree left a comment

gjkim42 commented Oct 11, 2022

soltysh left a comment

k8s-ci-robot commented Nov 7, 2022

soltysh commented Nov 7, 2022

gjkim42 Nov 8, 2022

soltysh Nov 8, 2022

gjkim42 Nov 8, 2022

liggitt commented Nov 8, 2022

gjkim42 commented Nov 8, 2022

atiratree commented Nov 8, 2022

gjkim42 commented Nov 8, 2022

Update daemonSet status even if syncDaemonSet fails #112127

Update daemonSet status even if syncDaemonSet fails #112127

Conversation

gjkim42 commented Aug 30, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

gjkim42 commented Aug 31, 2022

gjkim42 commented Aug 31, 2022

alculquicondor commented Aug 31, 2022

atiratree left a comment

Choose a reason for hiding this comment

gjkim42 commented Oct 11, 2022

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 7, 2022

soltysh commented Nov 7, 2022

gjkim42 Nov 8, 2022

Choose a reason for hiding this comment

soltysh Nov 8, 2022

Choose a reason for hiding this comment

gjkim42 Nov 8, 2022

Choose a reason for hiding this comment

liggitt commented Nov 8, 2022

gjkim42 commented Nov 8, 2022

atiratree commented Nov 8, 2022

gjkim42 commented Nov 8, 2022

gjkim42 commented Aug 30, 2022 •

edited