Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Promote CronJobs to batch/v1beta1 - just the API #41901

Merged
merged 2 commits into from Aug 16, 2017
Merged

Promote CronJobs to batch/v1beta1 - just the API #41901

merged 2 commits into from Aug 16, 2017

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Feb 22, 2017 •
edited

This PR promotes CronJobs to beta.

@erictune @kubernetes/sig-apps-api-reviews @kubernetes/api-approvers ptal

This builds on top of #41890 and needs #40932 as well

Promote CronJobs to batch/v1beta1.

@soltysh soltysh added area/batch area/workload-api/cronjob do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Feb 22, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@soltysh
Copy link
Contributor Author

soltysh commented Feb 22, 2017

@kubernetes/sig-api-machinery-pr-reviews ptal as well

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 22, 2017
@soltysh soltysh added this to the v1.6 milestone Feb 23, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017
@calebamiles calebamiles modified the milestones: v1.7, v1.6 Mar 2, 2017
@calebamiles
Copy link
Contributor

Moved this issue into the next milestone @soltysh. This PR did not get lgtm before code freeze and needs rebase and or rework. Feel free to file an exception if you feel it's warranted.

cc: @ethernetdan, @kubernetes/kubernetes-release-managers

@soltysh
Copy link
Contributor Author

soltysh commented Mar 2, 2017

@calebamiles I did file an exception, I'm waiting for hearing back.

@soltysh
Copy link
Contributor Author

soltysh commented Mar 2, 2017

It looks like the cronjob controller is tightly coupled with batch/v2alpha version and when batch/v2alpha1 and batch/v1beta1 are enabled at once it confuses kubectl on the other hand. So I need to put a lot more work to make it usable. Again multiversioning problems, sigh... 😕

This was referenced Mar 11, 2017
Merged
Merged
@janetkuo
Copy link
Member

@soltysh would it be easier if we remove batch/v2alpha1 altogether?

@soltysh
Copy link
Contributor Author

soltysh commented Apr 19, 2017

@soltysh would it be easier if we remove batch/v2alpha1 altogether?

It would definitely be easier, not sure if desirable, though. @erictune any objections on Janet's suggestion? I'll bring it up during next sig-apps call, to have a broader audience.

@deads2k
Copy link
Contributor

deads2k commented Apr 21, 2017

It would definitely be easier, not sure if desirable, though. @erictune any objections on Janet's suggestion? I'll bring it up during next sig-apps call, to have a broader audience.

It seems mean to break users when the serialization and behavior is actually compatible. Perhaps a series of simple reproducers will help us push past the problems with help from @kubernetes/sig-cli-bugs

@davidopp
Copy link
Member

There's a proposal that could in theory subsume cron jobs
https://docs.google.com/document/d/1-H2hnZap7gQivcSU-9j4ZrJ8wE_WwcfOkTeAGjzUyLA/edit
but it will require a lot of work.

@soltysh
Copy link
Contributor Author

soltysh commented Apr 24, 2017

@davidopp will you be able to attend sig-apps today and we can have a discussion.

@davidopp
Copy link
Member

@soltysh Sorry, didn't see this in time.

We'll be discussing the proposal at the sig-scheduling meeting today at 2:30 PM PT https://zoom.us/j/966775765

@soltysh
Copy link
Contributor Author

soltysh commented Apr 24, 2017

@davidopp sorry but too late for me :( I'll watch the recording and will catch up with you later this week.

@soltysh soltysh changed the title Promote CronJobs to batch/v1beta1 Promote CronJobs to batch/v1beta1 - just the API Aug 10, 2017
@soltysh soltysh removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 10, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Aug 10, 2017
@soltysh
Copy link
Contributor Author

soltysh commented Aug 10, 2017

@janetkuo ptal, I've enabled only the API here so that we can move with this forward. I need to address the remaining bits, such us how to deal with CronJobs when both v2alpha1 and v1beta1 is enabled. But I'd prefer to do it as a followup, for now I've turned off this new API and when all the pieces will be in I'll re-enable it permanently.

@soltysh
Copy link
Contributor Author

soltysh commented Aug 10, 2017

Forgot to actually comment the v1beta1 in master.go. Fixed, should be good.

@soltysh soltysh force-pushed the cronjobs_beta branch 2 times, most recently from 9f026cc to 6d0e522 Compare August 11, 2017 13:37
@soltysh
Copy link
Contributor Author

soltysh commented Aug 11, 2017

One more try, I've rebased hack/update-all.sh-ed and crossing my fingers.

@soltysh
Copy link
Contributor Author

soltysh commented Aug 11, 2017

Fixed last bit, the test/integration/etcd/etcd_storage_path_test.go. Should be green now 🤞

@soltysh
Copy link
Contributor Author

soltysh commented Aug 14, 2017

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 14, 2017 •
edited

@soltysh: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
Jenkins Cross Build d3db3ce link @k8s-bot cross build this
Jenkins verification d3db3ce link @k8s-bot verify test this
Jenkins GCE e2e d3db3ce link @k8s-bot cvm gce e2e test this
Jenkins GCE etcd3 e2e d3db3ce link @k8s-bot gce etcd3 e2e test this
Jenkins GKE smoke e2e d3db3ce link @k8s-bot cvm gke e2e test this
Jenkins GCI GKE smoke e2e d3db3ce link @k8s-bot gci gke e2e test this
Jenkins GCI GCE e2e d3db3ce link @k8s-bot gci gce e2e test this
Jenkins kops AWS e2e d3db3ce link @k8s-bot kops aws e2e test this
Jenkins Kubemark GCE e2e d3db3ce link @k8s-bot kubemark e2e test this
Jenkins GCE Node e2e d3db3ce link @k8s-bot node e2e test this
Jenkins non-CRI GCE e2e d3db3ce link @k8s-bot non-cri e2e test this
Jenkins non-CRI GCE Node e2e d3db3ce link @k8s-bot non-cri node e2e test this
Jenkins unit/integration d3db3ce link @k8s-bot unit test this
Jenkins Bazel Build d3db3ce link @k8s-bot bazel test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2017
@soltysh
Copy link
Contributor Author

soltysh commented Aug 16, 2017

@erictune @janetkuo it's finally green 😄 mind pushing this forward?

@smarterclayton
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erictune, smarterclayton, soltysh

Associated issue: 41890

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 4032896 into kubernetes:master Aug 16, 2017
@soltysh soltysh deleted the cronjobs_beta branch August 17, 2017 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@smarterclayton smarterclayton

@janetkuo janetkuo

@erictune erictune

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/batch area/workload-api/cronjob cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.

None yet