Cleanup Job after a configured ttl #149
Conversation
hzxuzhonghu
force-pushed
the
ttl
branch
2 times, most recently
from
88f2a9f
to
553533a
Compare
hzxuzhonghu
changed the title
|
Should be merged after #128 |
pkg/apis/batch/v1alpha1/job.go
Outdated
| // the Job won't be automatically deleted. If this field is set to zero, | ||
| // the Job becomes eligible to be deleted immediately after it finishes. | ||
| // +optional | ||
| TTLSecondsAfterFinished *int32 `json:"ttlSecondsAfterFinished,omitempty" protobuf:"varint,8,opt,name=ttlSecondsAfterFinished"` |
There was a problem hiding this comment.
Should we set a default value for this ? Current behavior is when no set, gc will not delete Job.
There was a problem hiding this comment.
Should we set a default value for this ? Current behavior is when no set, gc will not delete Job.
Yes, we should set default for this in admission controller.
There was a problem hiding this comment.
index -> 9
EDIT: Will update kube-batch
misread
|
/assign @k82cn @TommyLike |
| // Before deleting the Job, do a final sanity check. | ||
| // If TTL is modified before we do this check, we cannot be sure if the TTL truly expires. | ||
| // The latest Job may have a different UID, but it's fine because the checks will be run again. | ||
| fresh, err := gb.vkClient.BatchV1alpha1().Jobs(namespace).Get(name, metav1.GetOptions{}) |
There was a problem hiding this comment.
In which case, we need to do a double check ?
There was a problem hiding this comment.
We can not guarantee the job TTL not changed between gc received the it and processing here.
There was a problem hiding this comment.
Mostly these codes are referred from k8s ttlafterfinished controller
| @@ -31,17 +33,18 @@ func (ps *abortingState) Execute(action vkv1.Action) error { | |||
| // Already in Restarting phase, just sync it | |||
| return KillJob(ps.job, func(status *vkv1.JobStatus) { | |||
| status.State.Phase = vkv1.Restarting | |||
| status.State.LastTransitionTime = metav1.Now() | |||
There was a problem hiding this comment.
There a lots of codes:
status.State.LastTransitionTime = metav1.Now()
which can be upgraded into a function which used to update status and update the LastTransitionTime when required.
pkg/apis/batch/v1alpha1/job.go
Outdated
| @@ -218,6 +227,10 @@ type JobState struct { | |||
| // +optional | |||
| Phase JobPhase `json:"phase,omitempty" protobuf:"bytes,1,opt,name=phase"` | |||
|
|
|||
| // Last time the condition transit from one phase to another. | |||
| // +optional | |||
| LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty" protobuf:"bytes,4,opt,name=lastTransitionTime"` | |||
| // the Job won't be automatically deleted. If this field is set to zero, | ||
| // the Job becomes eligible to be deleted immediately after it finishes. | ||
| // +optional | ||
| TTLSecondsAfterFinished *int32 `json:"ttlSecondsAfterFinished,omitempty" protobuf:"varint,9,opt,name=ttlSecondsAfterFinished"` |
There was a problem hiding this comment.
I think we should add the validation as it should not be negative value. we can add the same in validateJob() func.
There was a problem hiding this comment.
validate can be applied in admission controller
There was a problem hiding this comment.
validate can be applied in admission controller
yeah thats correct we should add the validation in admission controller
There was a problem hiding this comment.
IIRC @wangyuqing4 will work on admission controller.
There was a problem hiding this comment.
@hzxuzhonghu , please confirm with @wangyuqing4 ; if she is not working this, please feel free to open a PR for that.
|
@k82cn This is ready to go? |
|
/lgtm |
volcano-sh-bot
added
approved
Cleanup Job after a configured ttl
No description provided.