Use omitempty for optional fields in Job Pod Failure Policy

[mimowo]

What type of PR is this?

/kind bug
/kind api-change

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #126040

Special notes for your reviewer:

My procedure for testing (repro):
0. repro by creating Jobset with Job template embedding the snippet:

        podFailurePolicy:
          rules:
          - action: FailJob
            onPodConditions:
            - type: DisruptionTarget
          - action: Ignore
            onExitCodes:
              operator: In
              values: [ 42 ]

In results in the following errors:

> k create -f jobset.yaml
The JobSet "failurepolicy-abcdef" is invalid: 
* spec.replicatedJobs[0].template.spec.podFailurePolicy.rules[0].onExitCodes: Invalid value: "null": spec.replicatedJobs[0].template.spec.podFailurePolicy.rules[0].onExitCodes in body must be of type object: "null"
* spec.replicatedJobs[0].template.spec.podFailurePolicy.rules[1].onExitCodes.containerName: Invalid value: "null": spec.replicatedJobs[0].template.spec.podFailurePolicy.rules[1].onExitCodes.containerName in body must be of type string: "null"
* spec.replicatedJobs[0].template.spec.podFailurePolicy.rules[1].onPodConditions: Invalid value: "null": spec.replicatedJobs[0].template.spec.podFailurePolicy.rules[1].onPodConditions in body must be of type array: "null"
* <nil>: Invalid value: "null": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation

1. Build JobSet using the updated k8s.io/api:

• clone https://github.com/kubernetes/api into JobSet repo under bin/k8s_api/api, and checkout the release-1.29 branch
• make code adjustments as in this repro (adding "omitempty")
• adjust JobSet's go.mod with replace k8s.io/api => ./bin/k8s_api/api to use the code, and adjust its Dockerfile with new line replace k8s.io/api => ./bin/k8s_api/api, then go mod tidy
• build JobSet image with make kind-image-build
• load the image to kind: kind load docker-image gcr.io/k8s-staging-jobset/jobset:0b8c19c-dirty --name kind
• adjust the JobSet image for the running deployment with kubectl edit deploy/jobset-controller-manager -njobset-system

2. The JobSet creation succeeds, the kubectl get job -oyaml returns the correct snippet:

    podFailurePolicy:
      rules:
      - action: FailJob
        onPodConditions:
        - status: "True"
          type: DisruptionTarget
      - action: Ignore
        onExitCodes:
          containerName: null
          operator: In
          values:
          - 42

(same snippet is created when creating the Job directly, without JobSet)

I have also confirmed that it only matters which version of k8s API is used to build the JobSet. In particular, when used fixed version to build JobSet, then I was able to create JobSets on 1.26 and 1.27 k8s clusters.

I see we did similar fixes in the past like here: #121000

Does this PR introduce a user-facing change?

Use omitempty for optional Job Pod Failure Policy fields

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

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

[k8s-ci-robot]

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-sigs/prow repository.

[mimowo]

/cc @alculquicondor @deads2k @liggitt

[mimowo]

@alculquicondor please add to kubernetes/enhancements#3329
Hope it does not need to block the graduation in #125442 and we can cherry-pick the fix

[k8s-triage-robot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[mimowo]

/label api-review

[mimowo]

I've checked that it only matters which version of k8s api is used to build the JobSet webhook. In particular, I was able to successfully create JobSets on k8s 1.26 and 1.27 clusters using JobSet built against a patched version of k8s API (for 1.29).

[deads2k]

Thinking through our serialization issues

1. if serialized with zero values into etcd, the kube-apiserver will start returning responses without values. ContainerName nil versus missing makes no semantic difference. OnExitCodes nil versus empty makes no semantic difference. OnPodConditions nil versus zero length array makes no semantic difference: some languages may handle length interactions differently, but had to tolerate old servers anyway.
2. if serialized without zero values, the kube-apiserver will stop providing zero values. As above, no semantic difference for controllers and the difference will be zero length array versus null.

On balance, I think it's better to let a consumer handle the empty array to nil shift than to have weird embedding problems.

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 3c37175e4d9e62b18975b5ffa9caefeb956f6c20

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, 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:

• staging/src/k8s.io/api/OWNERS [deads2k]

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

[mimowo]

Thanks, I will prepare cherry-picks down to 1.28 https://kubernetes.io/releases/. I think it will be particularly useful to cherry-pick to 1.29 or 1.30 for JobSet. cc @danielvegamyhre

[mimowo]

One more thing tested is I compared the response patches generated by the webhook generated using k8s.io/api before and after the change. To see that I enable log level 10 in the API server, and use k logs kube-apiserver-kind-control-plane -nkube-system | grep "Response Body", then decode base64 the patch by the webhook.

before:

[{"op":"add","path":"/metadata/creationTimestamp","value":null},
{"op":"add","path":"/spec/replicatedJobs/0/template/metadata","value":{"creationTimestamp":null}},
{"op":"add","path":"/spec/replicatedJobs/0/template/spec/template/metadata","value":{"creationTimestamp":null}},
{"op":"add","path":"/spec/replicatedJobs/0/template/spec/template/spec/containers/0/resources","value":{}},
{"op":"add","path":"/spec/replicatedJobs/0/template/spec/completionMode","value":"Indexed"},
{"op":"add","path":"/spec/replicatedJobs/0/template/spec/podFailurePolicy/rules/0/onExitCodes","value":null},
{"op":"add","path":"/spec/replicatedJobs/0/template/spec/podFailurePolicy/rules/0/onPodConditions/0/status","value":""},
{"op":"add","path":"/spec/replicatedJobs/0/template/spec/podFailurePolicy/rules/1/onExitCodes/containerName","value"
:null},{"op":"add","path":"/spec/replicatedJobs/0/template/spec/podFailurePolicy/rules/1/onPodConditions","value":null},
{"op":"add","path":"/spec/network","value":{"enableDNSHostnames":true}},
{"op":"add","path":"/spec/successPolicy","value":{"operator":"All"}},{"op":"add","path":"/spec/startupPolicy","value":
{"startupPolicyOrder":"AnyOrder"}},{"op":"add","path":"/status","value":{}}]

after:

[{"op":"add","path":"/metadata/creationTimestamp","value":null},
{"op":"add","path":"/spec/replicatedJobs/0/template/metadata","value":{"creationTimestamp":null}},
{"op":"add","path":"/spec/replicatedJobs/0/template/spec/completionMode","value":"Indexed"},
{"op":"add","path":"/spec/replicatedJobs/0/template/spec/podFailurePolicy/rules/0/onPodConditions/0/status","value":""
},{"op":"add","path":"/spec/replicatedJobs/0/template/spec/template/metadata","value":{"creationTimestamp":null}},
{"op":"add","path":"/spec/replicatedJobs/0/template/spec/template/spec/containers/0/resources","value":{}},
{"op":"add","path":"/spec/network","value":{"enableDNSHostnames":true,"publishNotReadyAddresses":true}},
{"op":"add","path":"/spec/successPolicy","value":{"operator":"All"}},{"op":"add","path":"/spec/startupPolicy","value":
{"startupPolicyOrder":"AnyOrder"}},{"op":"add","path":"/status","value":{}}]

So it is surprising that in the broken flow API server receives "null" for onExitCodes which should be semantically equivalent to no value (I suppose?).

[mimowo]

I've opened the cherry-pick PR. PTAL

[danielvegamyhre]

I've opened the cherry-pick PR. PTAL

Awesome thanks for the quick fix on this! Taking a look now.

[liggitt]

where we're validating immutable jobs on update:

allErrs = append(allErrs, apivalidation.ValidateImmutableField(spec.PodFailurePolicy, oldSpec.PodFailurePolicy, fldPath.Child("podFailurePolicy"))...)

would an empty array vs nil cause a validation failure, or are we collapsing those with apiequality.Semantic.DeepEqual?

[mimowo]

I added to validation_test.go the following test case and it passes:

		"update pod failure policy - nil to empty": {
			old: batch.Job{
				ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
				Spec: batch.JobSpec{
					Selector: validGeneratedSelector,
					Template: validPodTemplateSpecForGeneratedRestartPolicyNever,
					PodFailurePolicy: &batch.PodFailurePolicy{
						Rules: []batch.PodFailurePolicyRule{{
							Action:          batch.PodFailurePolicyActionIgnore,
							OnPodConditions: []batch.PodFailurePolicyOnPodConditionsPattern{},
							OnExitCodes: &batch.PodFailurePolicyOnExitCodesRequirement{
								Operator: batch.PodFailurePolicyOnExitCodesOpIn,
								Values:   []int32{42},
							},
						}},
					},
				},
			},
			update: func(job *batch.Job) {
				job.Spec.PodFailurePolicy.Rules[0].OnPodConditions = nil
			},
		},

(I could post a follow up PR). Yes, we use apiequality.Semantic.DeepEqual underneath by calling

kubernetes/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go

Line 131 in 8ba158c

func ValidateImmutableField(newVal, oldVal interface{}, fldPath *field.Path) field.ErrorList {

[liggitt]

Do you have the jobset CRD that gets produced prior to this PR?

Those validation errors when submitting the jobset CR object look to be coming from the server, not client-side validation.

I'm surprised the null values aren't getting pruned out by the server by PruneNonNullableNullsWithoutDefaults when submitting the object.

[liggitt]

I mostly agree that the peer on* fields should have omitempty, and I think it is safe to do (once we double-check that won't break immutability checks on job updates).

However, I don't understand why a CRD produced from this is complaining about null values for these fields... the server explicitly strips out null values for non-nullable fields without default values in the schema, so it should not have been complaining about these fields. Seeing the JobSet CRD schema would help in understanding why this is causing an error.

[danielvegamyhre]

I mostly agree that the peer on* fields should have omitempty, and I think it is safe to do (once we double-check that won't break immutability checks on job updates).

However, I don't understand why a CRD produced from this is complaining about null values for these fields... the server explicitly strips out null values for non-nullable fields without default values in the schema, so it should not have been complaining about these fields. Seeing the JobSet CRD schema would help in understanding why this is causing an error.

Here is a link to the relevant section of the base CRD in the JobSet repo. Note the CRD is quite large since it has a batchv1.JobTemplate embedded in it.

[mimowo]

I'm surprised the null values aren't getting pruned out by the server by PruneNonNullableNullsWithoutDefaults when submitting the object.

A guess, maybe there is a different code path in the api server, because the webhook actually returns the base64 encoded patch rather than the full object (as in the #126046 (comment))

[liggitt]

there are custom steps done after applying the patch from the webhook to decode and apply defaulting:

kubernetes/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

Lines 381 to 390 in 8ba158c

	if newVersionedObject, _, err = jsonSerializer.Decode(patchedJS, nil, newVersionedObject); err != nil {
	return false, apierrors.NewInternalError(err)
	}
	changed = !apiequality.Semantic.DeepEqual(attr.VersionedObject, newVersionedObject)
	span.AddEvent("Patch applied")
	annotator.addPatchAnnotation(patchObj, result.PatchType)
	attr.Dirty = true
	attr.VersionedObject = newVersionedObject
	o.GetObjectDefaulter().Default(attr.VersionedObject)

it appears that not all of the special things added into CR decoding are being done there

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

Line 1099 in 8ba158c

d := schemaCoercingDecoder{delegate: decoder, validator: unstructuredSchemaCoercer{structuralSchemas: s.structuralSchemas, structuralSchemaGK: s.structuralSchemaGK, preserveUnknownFields: s.preserveUnknownFields, returnUnknownFieldPaths: returnUnknownFieldPaths}}

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

Lines 1226 to 1251 in 8ba158c

	func (d schemaCoercingDecoder) Decode(data []byte, defaults *schema.GroupVersionKind, into runtime.Object) (runtime.Object, *schema.GroupVersionKind, error) {
	var decodingStrictErrs []error
	obj, gvk, err := d.delegate.Decode(data, defaults, into)
	if err != nil {
	decodeStrictErr, ok := runtime.AsStrictDecodingError(err)
	if !ok || obj == nil {
	return nil, gvk, err
	}
	decodingStrictErrs = decodeStrictErr.Errors()
	}
	var unknownFields []string
	if u, ok := obj.(*unstructured.Unstructured); ok {
	unknownFields, err = d.validator.apply(u)
	if err != nil {
	return nil, gvk, err
	}
	}
	if d.validator.returnUnknownFieldPaths && (len(decodingStrictErrs) > 0 || len(unknownFields) > 0) {
	for _, unknownField := range unknownFields {
	decodingStrictErrs = append(decodingStrictErrs, fmt.Errorf(`unknown field "%s"`, unknownField))
	}
	return obj, gvk, runtime.NewStrictDecodingError(decodingStrictErrs)
	}
	return obj, gvk, nil
	}

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

Lines 1311 to 1376 in 8ba158c

	func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) (unknownFieldPaths []string, err error) {
	// save implicit meta fields that don't have to be specified in the validation spec
	kind, foundKind, err := unstructured.NestedString(u.UnstructuredContent(), "kind")
	if err != nil {
	return nil, err
	}
	apiVersion, foundApiVersion, err := unstructured.NestedString(u.UnstructuredContent(), "apiVersion")
	if err != nil {
	return nil, err
	}
	objectMeta, foundObjectMeta, metaUnknownFields, err := schemaobjectmeta.GetObjectMetaWithOptions(u.Object, schemaobjectmeta.ObjectMetaOptions{
	DropMalformedFields: v.dropInvalidMetadata,
	ReturnUnknownFieldPaths: v.returnUnknownFieldPaths,
	})
	if err != nil {
	return nil, err
	}
	unknownFieldPaths = append(unknownFieldPaths, metaUnknownFields...)
	// compare group and kind because also other object like DeleteCollection options pass through here
	gv, err := schema.ParseGroupVersion(apiVersion)
	if err != nil {
	return nil, err
	}
	if gv.Group == v.structuralSchemaGK.Group && kind == v.structuralSchemaGK.Kind {
	if !v.preserveUnknownFields {
	// TODO: switch over pruning and coercing at the root to schemaobjectmeta.Coerce too
	pruneOpts := structuralschema.UnknownFieldPathOptions{}
	if v.returnUnknownFieldPaths {
	pruneOpts.TrackUnknownFieldPaths = true
	}
	unknownFieldPaths = append(unknownFieldPaths, structuralpruning.PruneWithOptions(u.Object, v.structuralSchemas[gv.Version], true, pruneOpts)...)
	structuraldefaulting.PruneNonNullableNullsWithoutDefaults(u.Object, v.structuralSchemas[gv.Version])
	}
	err, paths := schemaobjectmeta.CoerceWithOptions(nil, u.Object, v.structuralSchemas[gv.Version], false, schemaobjectmeta.CoerceOptions{
	DropInvalidFields: v.dropInvalidMetadata,
	ReturnUnknownFieldPaths: v.returnUnknownFieldPaths,
	})
	if err != nil {
	return nil, err
	}
	unknownFieldPaths = append(unknownFieldPaths, paths...)
	// fixup missing generation in very old CRs
	if v.repairGeneration && objectMeta.Generation == 0 {
	objectMeta.Generation = 1
	}
	}
	// restore meta fields, starting clean
	if foundKind {
	u.SetKind(kind)
	}
	if foundApiVersion {
	u.SetAPIVersion(apiVersion)
	}
	if foundObjectMeta {
	if err := schemaobjectmeta.SetObjectMeta(u.Object, objectMeta); err != nil {
	return nil, err
	}
	}
	return unknownFieldPaths, nil
	}