-
Notifications
You must be signed in to change notification settings - Fork 994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup Job after a configured ttl #149
Conversation
88f2a9f
to
553533a
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index -> 9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index -> 9
EDIT: Will update kube-batch
misread
/assign @k82cn @TommyLike |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests are required.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case, we need to do a double check ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can not guarantee the job TTL not changed between gc received the it and processing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ordered filed by protobuf id
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate can be applied in admission controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC @wangyuqing4 will work on admission controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 |
Cleanup Job after a configured ttl
No description provided.