Use SSA to add pod failure conditions #113304

mimowo · 2022-10-24T14:39:46Z

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

In order to eliminate the risk of failures due to conflicts when adding pod disruption conditions. Currently, pod conditions
are added by patches without resource version validation which can lead even to dropping a condition when combined with condition removals: kubernetes/enhancements#3463 (comment)

Which issue(s) this PR fixes:

Tracking issue: Retriable and non-retriable Pod failures for Jobs enhancements#3329

Special notes for your reviewer:

Added a 5s poll when asserting on the observed actions. This is because otherwise the test is flaky, probably even more than before because as it observes patches it takes longer than before to apply them.

Does this PR introduce a user-facing change?

The `kube-scheduler` and `kube-controller-manager` now use server side apply to set conditions related to pod disruption.

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/3329-retriable-and-non-retriable-failures

mimowo · 2022-10-24T14:40:54Z

/assign @lavalamp @alculquicondor

mimowo · 2022-10-24T14:52:48Z

/retest

lavalamp · 2022-10-24T16:48:21Z

@apelisse can you take a first pass on this?

mimowo · 2022-10-24T17:07:32Z

/retest

mimowo · 2022-10-24T17:45:56Z

/retest

pkg/controller/nodelifecycle/scheduler/taint_manager.go

staging/src/k8s.io/component-helpers/apps/podfailureconditions/helpers.go

pkg/controller/disruption/disruption.go

alculquicondor · 2022-10-25T17:29:33Z

pkg/controller/nodelifecycle/scheduler/taint_manager.go

+			WithStatus(v1.ConditionTrue).
+			WithReason("DeletionByTaintManager").
+			WithMessage("Taint manager: deleting due to NoExecute taint").
+			WithLastTransitionTime(metav1.Now()),


Does this mean that there is no alternative today? Or should we be copying the transition time from the condition if we see it in the status?

alculquicondor · 2022-10-25T17:30:41Z

pkg/controller/nodelifecycle/scheduler/taint_manager_test.go

-		}
-		if action.GetVerb() == "delete" && action.GetResource().Resource == "pods" {
-			podDeleted = true
+	err := wait.PollImmediate(10*time.Millisecond, 5*time.Second, func() (bool, error) {


why is the poll necessary now?

Test is time-based (as we await for the controller to make progress with its actions), waiting only 500ms to observer the actions.

As a result the test occasionally fails (it so happened that it failed on my branch, but was passing consistently locally). Thus, I thought that I could loop to wait a little longer to make the test more stable. I think I could decouple it from the code changes I made.

Also note that, before introducing pod disruption conditions, the test only waited for controller to perform the fake DELETE requests, but now it also needs to perform the fake PATCH / APPLY, so it makes sense to give it a little more time.

I have reverted the change and the tests passed, so probably the test flaked yesterday on the infra. Still, I think my changes make sense to make the test more reliable, but I can decouple them into a separate PR. Keep reverted from this PR for now.

Ticketed in an independent PR: #113386

pkg/controller/podgc/gc_controller.go

alculquicondor · 2022-10-25T17:33:52Z

pkg/controller/podgc/gc_controller.go

 				return err
 			}
 		}
 	}
 	return gcc.kubeClient.CoreV1().Pods(pod.Namespace).Delete(ctx, pod.Name, *metav1.NewDeleteOptions(0))
 }
+
+func updatePodCondition(podStatusApply *corev1apply.PodStatusApplyConfiguration, condition *corev1apply.PodConditionApplyConfiguration) {
+	if conditionIndex, _ := findPodConditionApplyByType(podStatusApply.Conditions, *condition.Type); conditionIndex < 0 {


should we be looking at all the conditions (including the ones that are not owned by this controller)?

alculquicondor · 2022-10-25T17:35:13Z

pkg/scheduler/framework/preemption/preemption.go

+					WithLastTransitionTime(metav1.Now()),
+				)
+
+				if _, err := cs.CoreV1().Pods(victim.Namespace).ApplyStatus(ctx, victimPodApply, metav1.ApplyOptions{FieldManager: "Scheduler", Force: true}); err != nil {


is there a rule that the field manager is camel case? We should be using the profile name (which matches the scheduler name)

I think there is no rule it should be camel case or in any other format - the documentation of the field is pretty relaxed:

kubernetes/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go

Line 541 in 5539a5b

FieldManager string `json:"fieldManager,omitempty" protobuf:"bytes,3,name=fieldManager"`

. In this blog post hypen is used: https://kubernetes.io/blog/2021/08/06/server-side-apply-ga/. I propose camel case just to pick something and be consistent as being consistent was one of the remarks by @apelisse. Also, we generally use camel-case in the reason field when naming the actor, for example PreemptionByKubeScheduler.

As for the profile name - could you explain what the value is, is it a constant?

I searched for profile name within the package and found this "

kubernetes/pkg/scheduler/framework/interface.go

Line 592 in 5539a5b

ProfileName() string

, but it doesn't seem ready to use in preemption.go as it is not passed via PostFilter invocations. I guess it would require extending the set of parameters when calling PostFilter or passing the value in context as a keyed value.

I think staying with a constant value for the actor is good enough, until we have a scenario in which it is problematic.

pkg/controller/disruption/disruption.go

pkg/scheduler/framework/preemption/preemption.go

alculquicondor · 2022-10-27T15:07:33Z

/approve
for scheduler

mimowo · 2022-10-27T15:40:49Z

/approve for scheduler

@alculquicondor please unhold the PR if no further issues

pkg/controller/disruption/disruption.go

alculquicondor · 2022-10-27T16:00:16Z

/hold cancel

mimowo · 2022-10-27T16:25:07Z

Applied remarks and squashed commits.
@apelisse please let me know if there is something more needed or lgtm so that it gets merged.

leilajal · 2022-10-27T16:54:44Z

/cc @apelisse
/triage accepted

alculquicondor · 2022-10-28T12:57:45Z

/lgtm
/approve

k8s-ci-robot · 2022-10-28T12:58:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, lavalamp, mimowo

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/OWNERS~~ [lavalamp]
~~pkg/scheduler/OWNERS~~ [alculquicondor,lavalamp]
~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]

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

sftim · 2022-11-07T16:22:00Z

My suggestion for the changelog entry:

The `kube-scheduler` and `kube-controller-manager` now use server side apply to set conditions related to  pod disruption.

liggitt · 2023-03-22T00:26:25Z

staging/src/k8s.io/client-go/testing/fixture.go

@@ -181,7 +181,7 @@ func ObjectReaction(tracker ObjectTracker) ReactionFunc {
 				if err := json.Unmarshal(modified, obj); err != nil {
 					return true, nil, err
 				}
-			case types.StrategicMergePatchType:
+			case types.StrategicMergePatchType, types.ApplyPatchType:


This change silently enabled unit tests written with server-side apply to appear to succeed, while actually testing subtly different behavior... I really don't think client-side strategic merge patch application is a good way to treat apply patches in the fake client

This is more likely to lead someone to release code they think they tested and have it break in weird and subtle ways in real life.

Indeed, it might be problematic in some cases. I've done it to enable adjusting the unit tests. I'm wondering how we should fix it. Some options that come to me:

provide a complete implementation in the testing library that would mimic the server (would be preferred, but is it feasible?)

detect some unsupported uses in the testing library and return an error, still allowing for simple test cases to work

revert the change, but then it isn't clear to me how to write / adjust existing unit tests

I would start by reverting the change to avoid misleading random unit tests that try to use apply

To enable specific unit tests, they could add a reactor that handles this patch type, asserts receiving specific apply patch content, and mocks the application of that patch to the test object

Ok, I have opened the issue for now: #116851. Hope there are some contributors who can help with that.

@apelisse has some stuff in flight to make unit testing SSA easier / possible...

k8s-ci-robot assigned alculquicondor and lavalamp Oct 24, 2022

k8s-ci-robot requested review from alculquicondor and chendave October 24, 2022 14:41

mimowo force-pushed the handling-pod-failures-beta-ssa branch from a323594 to 0e387bd Compare October 24, 2022 16:24

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 24, 2022

mimowo changed the title ~~SSA to add pod failure conditions - ready for review~~ SSA to add pod failure conditions Oct 24, 2022

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 24, 2022

apelisse reviewed Oct 24, 2022

View reviewed changes

mimowo force-pushed the handling-pod-failures-beta-ssa branch from 18f50e9 to 0dee1c1 Compare October 25, 2022 06:53

mimowo changed the title ~~SSA to add pod failure conditions~~ Use SSA to add pod failure conditions Oct 25, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2022

alculquicondor reviewed Oct 25, 2022

View reviewed changes

mimowo force-pushed the handling-pod-failures-beta-ssa branch 3 times, most recently from 9344048 to 791f191 Compare October 26, 2022 09:35

mimowo mentioned this pull request Oct 27, 2022

Enable the "Retriable and non-retriable pod failures for jobs" feature into beta #113360

Merged

alculquicondor mentioned this pull request Oct 27, 2022

Retriable and non-retriable Pod failures for Jobs kubernetes/enhancements#3329

Open

8 tasks

alculquicondor reviewed Oct 27, 2022

View reviewed changes

pkg/controller/disruption/disruption.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/preemption/preemption.go Outdated Show resolved Hide resolved

alculquicondor reviewed Oct 27, 2022

View reviewed changes

pkg/controller/disruption/disruption.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2022

SSA to add pod failure conditions - ready for review

fea8836

mimowo force-pushed the handling-pod-failures-beta-ssa branch from e6a31ea to fea8836 Compare October 27, 2022 16:22

k8s-ci-robot requested a review from apelisse October 27, 2022 16:54

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 Oct 27, 2022

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

k8s-ci-robot merged commit 3c9928e into kubernetes:master Oct 28, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 28, 2022

mimowo deleted the handling-pod-failures-beta-ssa branch March 18, 2023 18:42

liggitt reviewed Mar 22, 2023

View reviewed changes

liggitt mentioned this pull request Mar 22, 2023

Automated cherry pick of #115966: make MixedProtocolNotSupported public #116668

Merged

mimowo mentioned this pull request Mar 22, 2023

Do not use StrategicMergePatch for mocking server side apply with in unit tests #116851

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SSA to add pod failure conditions #113304

Use SSA to add pod failure conditions #113304

mimowo commented Oct 24, 2022 •

edited

mimowo commented Oct 24, 2022

mimowo commented Oct 24, 2022

lavalamp commented Oct 24, 2022

mimowo commented Oct 24, 2022

mimowo commented Oct 24, 2022

alculquicondor Oct 25, 2022

alculquicondor Oct 25, 2022

mimowo Oct 26, 2022

mimowo Oct 26, 2022

mimowo Oct 27, 2022

alculquicondor Oct 25, 2022

alculquicondor Oct 25, 2022

mimowo Oct 26, 2022 •

edited

alculquicondor commented Oct 27, 2022

mimowo commented Oct 27, 2022

alculquicondor commented Oct 27, 2022

mimowo commented Oct 27, 2022

leilajal commented Oct 27, 2022

alculquicondor commented Oct 28, 2022

k8s-ci-robot commented Oct 28, 2022

sftim commented Nov 7, 2022

liggitt Mar 22, 2023

mimowo Mar 22, 2023

liggitt Mar 22, 2023

mimowo Mar 22, 2023

lavalamp Mar 22, 2023

Use SSA to add pod failure conditions #113304

Use SSA to add pod failure conditions #113304

Conversation

mimowo commented Oct 24, 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.:

mimowo commented Oct 24, 2022

mimowo commented Oct 24, 2022

lavalamp commented Oct 24, 2022

mimowo commented Oct 24, 2022

mimowo commented Oct 24, 2022

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

mimowo Oct 26, 2022 • edited

Choose a reason for hiding this comment

alculquicondor commented Oct 27, 2022

mimowo commented Oct 27, 2022

alculquicondor commented Oct 27, 2022

mimowo commented Oct 27, 2022

leilajal commented Oct 27, 2022

alculquicondor commented Oct 28, 2022

k8s-ci-robot commented Oct 28, 2022

sftim commented Nov 7, 2022

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

mimowo commented Oct 24, 2022 •

edited

mimowo Oct 26, 2022 •

edited