feat: add terminalState to jobset status

[googs1025]

Fix: #545

[netlify]

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name	Link
🔨 Latest commit	a1c05dc
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/66813df2f7c0fc0008963707

[googs1025]

root@VM-0-9-ubuntu:/home/ubuntu# kubectl get jobset
NAME                   PHASE       RESTARTS   COMPLETED   SUSPENDED   AGE
max-restarts           Failed      3                                  44m
paralleljobs           Completed              True                    44m
startup-driver-ready   Completed              True                    29m
success-policy         Completed              True                    44m

[googs1025]

        // JobSetRunning means the job is running.
	JobSetRunning JobSetConditionType = "Running"
	// JobSetCompleted means the job has completed its execution.
	JobSetCompleted JobSetConditionType = "Completed"
	// JobSetFailed means the job has failed its execution.
	JobSetFailed JobSetConditionType = "Failed"
	// JobSetSuspended means the job is suspended.
	JobSetSuspended JobSetConditionType = "Suspended"

The phase is converted according to these four fields. The default is Running

[googs1025]

/assign @danielvegamyhre
Please also help review, thank you!

[shrinandj]

Thanks for doing this @googs1025. I will test this out..

[ahg-g]

this should be in the status, not the spec, and we can't default it.

[googs1025]

Currently it is set in jobs status

// JobSetStatus defines the observed state of JobSet
type JobSetStatus struct {
	// +optional
	// +listType=map
	// +listMapKey=type
	Conditions []metav1.Condition `json:"conditions,omitempty"`

         ....
	// Phase of the JobSet.
	// +kubebuilder:default="Running"
	Phase string `json:"phase,omitempty"`

	
}

[ahg-g]

We don't need to pass this around, we should be able to compute the phase from the condition in the status update function.

[googs1025]

If we only use condition calculation, we may need to exclude other JobSetConditionType. I think it would be more convenient to use one more field such as phase or terminalState.

such as: These conditions are not what this field needs to care about

// These are built-in conditions of a JobSet.
const (
	...
	// JobSetSuspended means the job is suspended.
	JobSetSuspended JobSetConditionType = "Suspended"
	// JobSetStartupPolicyInProgress means the StartupPolicy is in progress.
	JobSetStartupPolicyInProgress JobSetConditionType = "StartupPolicyInProgress"
	// JobSetStartupPolicyCompleted means the StartupPolicy has completed.
	JobSetStartupPolicyCompleted JobSetConditionType = "StartupPolicyCompleted"
)

[shrinandj]

... TerminalState: success|failed, if more details are needed, then one needs to look that up in the conditions.

This would work perfectly..

[shrinandj]

I was able to do the following testing on this PR:

• Built a docker image with the code in this PR and pushed it a local docker registry
• Installed the new CRD and the new controller. The controller came up fine
• Create various types of jobsets that would succeed/fail. Verified that the status.phase field in the jobset object was set correctly to Completed or Failed
• Also liked the Phase field being in the output of kubectl get jobset

[danielvegamyhre]

I was able to do the following testing on this PR:

• Built a docker image with the code in this PR and pushed it a local docker registry
• Installed the new CRD and the new controller. The controller came up fine
• Create various types of jobsets that would succeed/fail. Verified that the status.phase field in the jobset object was set correctly to Completed or Failed
• Also liked the Phase field being in the output of kubectl get jobset

This is great! Thanks for testing this.

[googs1025]

I was able to do the following testing on this PR:

• Built a docker image with the code in this PR and pushed it a local docker registry
• Installed the new CRD and the new controller. The controller came up fine
• Create various types of jobsets that would succeed/fail. Verified that the status.phase field in the jobset object was set correctly to Completed or Failed
• Also liked the Phase field being in the output of kubectl get jobset

@shrinandj Thanks for your test
Later, it will be changed to the TerminalState field and will only have complated and failed, and the others will be empty by default.
I will revise this PR in the next few days. Thank all of you for suggestions.

[googs1025]

I changed the field to terminalState. When the jobset reaches the final state, it will be set to Completed or Failed.

root@VM-0-16-ubuntu:/home/ubuntu# kubectl get jobset
NAME             TERMINALSTATE   RESTARTS   COMPLETED   SUSPENDED   AGE
max-restarts     Failed          3                                  89m
paralleljobs     Completed                  True                    89m
success-policy   Completed                  True                    89m

[googs1025]

@ahg-g @danielvegamyhre /PTAL Thanks for review

[shrinandj]

Could this be merged and new release be cut with this feature?

[danielvegamyhre]

Could this be merged and new release be cut with this feature?

Yep, will do final review shortly

[danielvegamyhre]

Why did you choose to put the logic updating the terminal state in the function that is responsible for updating the condition, rather than put it in its own function? The logic doesn't seem to depend on the condition updates here, it just depends on the opts passed in, so I don't see why this logic belongs here.

[googs1025]

My initial thought was that the TerminalState field is actually a subtype of the Conditions field, specifically the JobSetCompleted or JobSetFailed types. TerminalState can be seen as a special case within Conditions. That's why I grouped these two together and updated them in the same method.

[googs1025]

If separating them, we can distinguish them into updateCondition and updateTerminalState methods.

// setCondition will add a new condition and terminalState to the JobSet status (or update an existing one),
// and enqueue an event for emission if the status update succeeds at the end of the reconcile.
func setCondition(js *jobset.JobSet, condOpts *conditionOpts, updateStatusOpts *statusUpdateOpts) {
	// Return early if no status update is required for this condition and terminalState.
	if !updateCondition(js, condOpts) && !updateTerminalState(js, condOpts) {
		return
	}

	if updateStatusOpts == nil {
		updateStatusOpts = &statusUpdateOpts{}
	}

	...
}

[ahg-g]

I suggest to set status.TerminalState in the same functions where we set the completed or failed condition. Meaning in setJobSetFailedCondition and setJobSetCompletedCondition

func setJobSetCompletedCondition(js *jobset.JobSet, updateStatusOpts *statusUpdateOpts) {
	setCondition(js, makeCompletedConditionsOpts(), updateStatusOpts)
        jobset.Status.TerminalState = JobSetCompleted
}

func setJobSetFailedCondition(ctx context.Context, js *jobset.JobSet, reason, msg string, updateStatusOpts *statusUpdateOpts) {
	setCondition(js, makeFailedConditionOpts(reason, msg), updateStatusOpts)
        jobset.Status.TerminalState = JobSetFailed
}

and that is all, we don't need to track anything in updateStatusOpts

[googs1025]

Thanks for the suggestion, this makes the code much simpler! done

[ahg-g]

Please add a checkJobSetTerminalState function to https://github.com/kubernetes-sigs/jobset/blob/main/test/util/util.go :

func checkJobSetTerminalState(ctx context.Context, k8sClient client.Client, js *jobset.JobSet, state string) (bool, error) {
	var fetchedJS jobset.JobSet
	if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: js.Namespace, Name: js.Name}, &fetchedJS); err != nil {
		return false, err
	}
        return fetchedJS.Status.TerminalState == state
}

and call it from JobSetCompleted to and JobSetFailed to verify the terminalState during integration tests.

[googs1025]

ok，i will do it soon

[googs1025]

done

[ahg-g]

/lgtm
/approve
/label tide/merge-method-squash

This is great, thank you for your patience during the review process!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, googs1025

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:

• OWNERS [ahg-g]

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

[tenzen-y]

Thank you for moving this forward!