apis: add CronJob resource #192
Conversation
|
Welcome @GoingCharlie! |
|
@GoingCharlie: The label(s) In response to this:
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/test-infra repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @GoingCharlie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new CronJob resource to the volcano.sh/v1alpha1 API group, enabling scheduled job execution within the Volcano ecosystem. This new resource provides functionality similar to Kubernetes CronJobs, allowing users to define jobs that run on a time-based schedule, seamlessly integrated with Volcano's existing job orchestration capabilities.
Highlights
- New API Definition: A new CronJob Custom Resource Definition (CRD) has been added, including CronJobSpec for defining scheduling parameters and CronJobStatus for tracking job execution.
- Concurrency Control: The CronJobSpec now includes a ConcurrencyPolicy to manage how concurrent job runs are handled (Allow, Forbid, Replace).
- Job Template Integration: The CronJob resource integrates with existing JobTemplateSpec to define the actual job to be created and executed.
- API Scheme and Client Updates: The API scheme has been updated to register the new CronJob and CronJobList types, and corresponding client-go code (clientset, informers, listers, apply configurations) has been generated.
- New Annotation: A CronJobScheduledTimestampAnnotation has been introduced for tracking scheduled job timestamps.
- DeepCopy Function Generation: Necessary deepcopy functions for the new CronJob related types have been automatically generated.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new CronJob resource to enable scheduled job execution in Volcano, which is a great addition. The implementation is comprehensive, adding the API definition, generated clientset, informers, and listers. My review has identified a few areas for improvement in the API definition to enhance correctness and maintainability. Specifically, I've pointed out a typo in an annotation constant, non-standard syntax for kubebuilder markers, and some inconsistencies in protobuf tagging, including an incorrect tag for a list field. Addressing these points will strengthen the new API. Overall, this is a solid contribution.
pkg/apis/batch/v1alpha1/types.go
Outdated
| type CronJobList struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ListMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` | ||
| Items []CronJob `json:"items,omitempty" protobuf:"bytes,2,opt,name=items"` |
There was a problem hiding this comment.
For a repeated field (a slice), the protobuf tag should be rep, not opt. Also, for consistency with JobList in this file, the omitempty tag should be removed from the JSON tag for Items. This ensures that an empty list is serialized as [] rather than being omitted, which is a common convention in Kubernetes APIs.
| Items []CronJob `json:"items,omitempty" protobuf:"bytes,2,opt,name=items"` | |
| Items []CronJob `json:"items" protobuf:"bytes,2,rep,name=items"` |
pkg/apis/batch/v1alpha1/labels.go
Outdated
| // BurstToSiloClusterAnnotation labels key for resource only in silo cluster | ||
| BurstToSiloClusterAnnotation = "volcano.sh/silo-resource" | ||
| BurstToSiloClusterAnnotation = "volcano.sh/silo-resource" | ||
| CronJobScheduledTimestampAnnotation = "volcano.sh/cronjob-schedules-timestamp" |
There was a problem hiding this comment.
The value of this constant appears to have a typo. The constant name is CronJobScheduledTimestampAnnotation, but the value is "volcano.sh/cronjob-schedules-timestamp". To be consistent and grammatically correct, "schedules" should likely be "scheduled".
| CronJobScheduledTimestampAnnotation = "volcano.sh/cronjob-schedules-timestamp" | |
| CronJobScheduledTimestampAnnotation = "volcano.sh/cronjob-scheduled-timestamp" |
| type CronJobSpec struct { | ||
|
|
||
| // The schedule in Cron format, see https://en.wikipedia.org/wiki/Cron. | ||
| Schedule string `json:"schedule" protobuf:"bytes,1,opt,name=schedule"` | ||
|
|
||
| // The time zone name for the given schedule, see https://en.wikipedia.org/wiki/List_of_tz_database_time_zones. | ||
| // If not specified, this will default to the time zone of the kube-controller-manager process. | ||
| // The set of valid time zone names and the time zone offset is loaded from the system-wide time zone | ||
| // database by the API server during CronJob validation and the controller manager during execution. | ||
| // If no system-wide time zone database can be found a bundled version of the database is used instead. | ||
| // If the time zone name becomes invalid during the lifetime of a CronJob or due to a change in host | ||
| // configuration, the controller will stop creating new new Jobs and will create a system event with the | ||
| // reason UnknownTimeZone. | ||
| // More information can be found in https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/#time-zones | ||
| // +optional | ||
| TimeZone *string `json:"timeZone,omitempty" protobuf:"bytes,8,opt,name=timeZone"` | ||
|
|
||
| // Optional deadline in seconds for starting the job if it misses scheduled | ||
| // time for any reason. Missed jobs executions will be counted as failed ones. | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +optional | ||
| StartingDeadlineSeconds *int64 `json:"startingDeadlineSeconds,omitempty" protobuf:"varint,2,opt,name=startingDeadlineSeconds"` | ||
|
|
||
| // Specifies how to treat concurrent executions of a Job. | ||
| // Valid values are: | ||
| // - "Allow" (default): allows CronJobs to run concurrently; | ||
| // - "Forbid": forbids concurrent runs, skipping next run if previous run hasn't finished yet; | ||
| // - "Replace": cancels currently running job and replaces it with a new one | ||
| // +kubebuilder:default=Allow | ||
| // +kubebuilder:validation:Enum=Allow;Forbid;Replace | ||
| // +optional | ||
| ConcurrencyPolicy ConcurrencyPolicy `json:"concurrencyPolicy,omitempty" protobuf:"bytes,3,opt,name=concurrencyPolicy,casttype=ConcurrencyPolicy"` | ||
|
|
||
| // This flag tells the controller to suspend subsequent executions, it does | ||
| // not apply to already started executions. Defaults to false. | ||
| // +kubebuilder:default=false | ||
| // +optional | ||
| Suspend *bool `json:"suspend,omitempty" protobuf:"varint,4,opt,name=suspend"` | ||
|
|
||
| // Specifies the job that will be created when executing a CronJob. | ||
| JobTemplate JobTemplateSpec `json:"jobTemplate" protobuf:"bytes,5,opt,name=jobTemplate"` | ||
|
|
||
| // The number of successful finished jobs to retain. | ||
| // This is a pointer to distinguish between explicit zero and not specified. | ||
| // Defaults to 3. | ||
| // +kubebuilder:default:=3 | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +optional | ||
| SuccessfulJobsHistoryLimit *int32 `json:"successfulJobsHistoryLimit,omitempty" protobuf:"varint,6,opt,name=successfulJobsHistoryLimit"` | ||
|
|
||
| // The number of failed finished jobs to retain. | ||
| // This is a pointer to distinguish between explicit zero and not specified. | ||
| // Defaults to 3. | ||
| // +kubebuilder:default:=1 | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +optional | ||
| FailedJobsHistoryLimit *int32 `json:"failedJobsHistoryLimit,omitempty" protobuf:"varint,7,opt,name=failedJobsHistoryLimit"` | ||
| } |
There was a problem hiding this comment.
The protobuf field numbers in CronJobSpec are not in increasing order in the struct definition. For instance, TimeZone has tag 8 but is defined before other fields with smaller tags. While this is valid, it's unconventional and can be confusing for future maintenance. For better readability and maintainability, please consider reordering the fields to match the protobuf tag order or re-numbering the tags to be sequential based on field order.
pkg/apis/batch/v1alpha1/types.go
Outdated
| // The number of successful finished jobs to retain. | ||
| // This is a pointer to distinguish between explicit zero and not specified. | ||
| // Defaults to 3. | ||
| // +kubebuilder:default:=3 |
There was a problem hiding this comment.
The +kubebuilder:default annotation uses a non-standard := operator. The correct syntax is =. Using the standard syntax ensures compatibility with future versions of controller-tools.
| // +kubebuilder:default:=3 | |
| // +kubebuilder:default=3 |
pkg/apis/batch/v1alpha1/types.go
Outdated
| // The number of failed finished jobs to retain. | ||
| // This is a pointer to distinguish between explicit zero and not specified. | ||
| // Defaults to 3. | ||
| // +kubebuilder:default:=1 |
There was a problem hiding this comment.
The +kubebuilder:default annotation uses a non-standard := operator. The correct syntax is =. Using the standard syntax ensures compatibility with future versions of controller-tools.
| // +kubebuilder:default:=1 | |
| // +kubebuilder:default=1 |
| type CronJobStatus struct { | ||
| // A list of pointers to currently running jobs. | ||
| // +optional | ||
| // +listType=atomic | ||
| Active []v1.ObjectReference `json:"active,omitempty" protobuf:"bytes,1,rep,name=active"` | ||
|
|
||
| // Information when was the last time the job was successfully scheduled. | ||
| // +optional | ||
| LastScheduleTime *metav1.Time `json:"lastScheduleTime,omitempty" protobuf:"bytes,4,opt,name=lastScheduleTime"` | ||
|
|
||
| // Information when was the last time the job successfully completed. | ||
| // +optional | ||
| LastSuccessfulTime *metav1.Time `json:"lastSuccessfulTime,omitempty" protobuf:"bytes,5,opt,name=lastSuccessfulTime"` | ||
| } |
There was a problem hiding this comment.
The protobuf field numbers in CronJobStatus are not sequential (1, 4, 5). Tags 2 and 3 are skipped. While this is valid, it's a good practice to use sequential numbers or add comments to reserve the missing numbers for future fields. This improves the API's maintainability.
GoingCharlie
force-pushed
the
feature/add-cronjob
branch
from
9ec2216 to
f8b4dd8
Compare
|
Please do not change the file mode. |
GoingCharlie
force-pushed
the
feature/add-cronjob
branch
from
f8b4dd8 to
ed273b6
Compare
Thanks for the reminder! I've restored the original file permissions and removed the permission changes from the commit. The file modes are now back to their correct state. |
pkg/apis/batch/v1alpha1/job.go
Outdated
| // +genclient | ||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
| // +kubebuilder:object:root=true | ||
| // +kubebuilder:resource:shortName=vccronjob;vccj;cronvcjob |
There was a problem hiding this comment.
| // +kubebuilder:resource:shortName=vccronjob;vccj;cronvcjob | |
| // +kubebuilder:resource:shortName=cronvcjob;cronvj |
pkg/apis/batch/v1alpha1/job.go
Outdated
|
|
||
| // CronJobSpec defines the desired state of Cronjob | ||
| type CronJobSpec struct { | ||
|
|
| } | ||
|
|
||
| // CronJobSpec defines the desired state of Cronjob | ||
| type CronJobSpec struct { |
There was a problem hiding this comment.
These fields are party copied from kubernetes, so we need give a a statement in the file's header, see this sample: https://github.com/volcano-sh/volcano/pull/4379/files#diff-357319a72a2fc1d4893c84206804fb7f3da46b923bae73b28870f584749d9396
Signed-off-by: GoingCharlie <[email protected]>
GoingCharlie
force-pushed
the
feature/add-cronjob
branch
from
ed273b6 to
5866abc
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kevin-wangzefeng 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:
Approvers can indicate their approval by writing |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds a new CronJob resource to the volcano.sh/v1alpha1 API group to enable scheduled job execution in Volcano. The CronJob resource allows users to create jobs that run on a time-based schedule, similar to Kubernetes CronJobs but integrated with Volcano's job orchestration capabilities.
Special notes for your reviewer:
Does this PR introduce a user-facing change?