Skip to content

Add CronJob support#40

Merged
iameskild merged 9 commits intokbatch-dev:mainfrom
iameskild:cronjob
Jun 10, 2022
Merged

Add CronJob support#40
iameskild merged 9 commits intokbatch-dev:mainfrom
iameskild:cronjob

Conversation

@iameskild
Copy link
Collaborator

@iameskild iameskild commented May 9, 2022

EDIT:

See this comment for updated design approach.


@TomAugspurger

This PR allows users to specify the --schedule="* * * * *" they want their job to run on. Specifying a schedule will create an underlying CronJob resource as opposed to a Job. Otherwise, from the user's perspective the only difference is that a job runs once and cleans up, whereas a cronjob runs on a schedule and will have to be manually deleted it for it to stop.

As I was working through this PR, I started to get the sense that creating a new model for CronJob might be worthwhile. It felt like I was shoehorning the schedule into place when in reality, and from the Kubernetes API point of view, CronJobs create Jobs which in turn create Pods. Perhaps it would be useful to reflect this relationship.

I am submitting this PR based on the initial design proposal here: #38.

Lastly, I will need to wrap this up by adding some tests but I wanted to get your perspective on this before doing so.

Thanks again for all your feedback :)

@dharhas
Copy link

dharhas commented May 19, 2022

@TomAugspurger @yuvipanda

We would like to use this feature in a ESIP/OGC sprint activity end of June. Any chance of a review?

cc: @rsignell-usgs

@TomAugspurger
Copy link
Collaborator

Sorry, a bit swamped at the moment.

I gave things a quick glance. My only question would be whether we want to handle all Job and CronJob stuff through the same user-facing API (so list_jobs has to return a tuple of outputs) or whether we want a list_jobs and list_cron_jobs, each specialized to their purpose. Do other batch systems have a preference? kubectl seems to split them up.

I don't want to be a bottleneck here, so I'm inclined to just trust your all's judgement.

@rsignell-usgs
Copy link

@TomAugspurger thanks so much for taking a look -- this is a small/short fuse project, so it's greatly appreciated!

@yuvipanda, I know you are swamped too, but if you have 10 min, would value your opinion since you conceived this baby, right? 😸

@dharhas
Copy link

dharhas commented May 23, 2022

@costrouc do you have any preference here. I can see pros/cons to both approaches.

@Adam-D-Lewis
Copy link

I lean toward what Tom has proposed with separate list_jobs and list_cron_jobs commands. I think the user needs to understand that Cronjobs create Jobs. If you list them together, it implies you can think about them as basically the same thing, but that could be confusing to the user since the CronJob itself as well as the Jobs started by the Cronjob will show up together (attached image to try to demonstrate this) and they are really separate things that the user will need to think about differently to understand how to interact with each. E.g. Deleting a Cronjob doesn't delete the Job(s) created by that Cronjob. Deleting a Job started by a Cronjob doesn't delete the Cronjob.

image

@iameskild
Copy link
Collaborator Author

Thanks everyone for your feedback! I definitely see reasons why we would prefer to separate out these commands.

To follow up on this proposed approach, I think it might be worthwhile to not only separate the user facing job and cronjob commands but also reflect those models internally by adding a CronJob class.

The a subset of possible user facing cli commands might look like:

CronJob Job Pod
cli command: kbatch cronjob job pod
submit x x
list x x x
delete x x
Object type CronJob Job

I will push some of these proposed changes tomorrow.

@iameskild iameskild changed the title Add ability to specify schedule to jobs Add CronJob support Jun 1, 2022
Copy link
Collaborator

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating @iameskild. Just one question about the docs. I only glanced through the rest, but IMO we can merge this once we've confirmed that the docs are correct.

@iameskild
Copy link
Collaborator Author

@TomAugspurger @Adam-D-Lewis

This PR is ready for a review.

I know this is more than initially scoped out but I believe this is a better and more comprehensive solution. The initial approach I took had the schedule "bolted" onto the Job. However based on the feedback above and some more reading on the kubernetes docs about Jobs vs CronJobs, it felt like CronJobs should be handled as a separate object or model. I did my best to reuse existing code and as many existing patterns as possible. This was at least in part feasible because CronJobs (through their V1JobTemplateSpec) and V1Job both have a V1.JobSpec .

A side note about the kbatch._types:

  • For the CronJob class I simply copied the Job class and added schedule, not the most elegant solution.
    • options to simplify or extend these models might include using pydantic or mapping cli inputs directly into the kubernetes models themselves somehow.
      • this might allow us to expose more of theV1CronJobSpec properties to the end-user as well.

I added a few tests (more might be needed) and updated the docs. I have been manually testing many of the job and cronjob cli commands on k3d on my (apple intel) local machine and everything seems to work as expected. No changes to the jobs cli.

Thanks for any feedback or suggestions you might have :)

@TomAugspurger
Copy link
Collaborator

Sorry for the delay. I won't have a chance to fully review but this seems like a good improvement.

I've invited @iameskild to be a maintainer. Feel free to merge whenever you're ready.

@iameskild
Copy link
Collaborator Author

Thanks a lot @TomAugspurger!

@Adam-D-Lewis @dharhas @rsignell-usgs do any of you have any feedback before I merge this?

@iameskild iameskild merged commit 30b6883 into kbatch-dev:main Jun 10, 2022
@iameskild iameskild deleted the cronjob branch June 10, 2022 16:29
@iameskild iameskild mentioned this pull request Jun 10, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants