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.

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

Create pkg/scheduling/apis/v1beta1 and move priorityClass to beta #63100

Merged
merged 2 commits into from May 14, 2018

Conversation

@ravisantoshgudimetla
Copy link
Contributor

ravisantoshgudimetla commented Apr 24, 2018

What this PR does / why we need it:
This is for creating pkg/apis/scheduling/v1beta1 so that priorityClasses could be moved to beta.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #57471

Special notes for your reviewer:
/cc @bsalamat @aveshagarwal

Release note:

The `PriorityClass` API is promoted to `scheduling.k8s.io/v1beta1`

@k8s-ci-robot k8s-ci-robot requested review from aveshagarwal and bsalamat Apr 24, 2018

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:priority-beta-api branch 2 times, most recently from 302c5a1 to 30f7f25 Apr 24, 2018

@@ -66,6 +66,7 @@ pkg/apis/rbac/v1beta1
pkg/apis/rbac/validation
pkg/apis/scheduling
pkg/apis/scheduling/v1alpha1

This comment has been minimized.

@aveshagarwal

aveshagarwal Apr 25, 2018

Member

why are you keeping v1alpha1?

This comment has been minimized.

@aveshagarwal

aveshagarwal Apr 25, 2018

Member

I think as I understand, it might be needed, if some apis are alpha and some are beta?

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Apr 25, 2018

Author Contributor

That is correct. I think we don't have a beta group for scheduling yet. So, we need it say if new api like priorityClass comes up in future.

@@ -37,12 +38,13 @@ func Install(groupFactoryRegistry announced.APIGroupFactoryRegistry, registry *r
if err := announced.NewGroupMetaFactory(
&announced.GroupMetaFactoryArgs{
GroupName: scheduling.GroupName,
VersionPreferenceOrder: []string{v1alpha1.SchemeGroupVersion.Version},
VersionPreferenceOrder: []string{v1beta1.SchemeGroupVersion.Version, v1alpha1.SchemeGroupVersion.Version},
RootScopedKinds: sets.NewString("PriorityClass"),

This comment has been minimized.

@aveshagarwal

aveshagarwal Apr 25, 2018

Member

Here it says that rootscopekind is PriorityClass? So wondering why to keep both v1alpha1 and v1beta1 if its only for priorityclass?

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Apr 25, 2018

Author Contributor

I am not a 100% sure. I believe it is because of backward compatibility. I have looked at existing API which have graduated to beta and all of them have this pattern of beta and alpha.

This comment has been minimized.

@aveshagarwal

aveshagarwal Apr 26, 2018

Member

In my understanding, there is no guarantee provided by Kubernetes of backward compatibility for alpha apis. See the alpha api section: https://kubernetes.io/docs/concepts/overview/kubernetes-api/

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:priority-beta-api branch from 30f7f25 to 255bfc7 Apr 25, 2018

@bsalamat
Copy link
Contributor

bsalamat left a comment

Looks generally good, but I am not so sure about keeping v1alpha1. API folks can help us review that part.
Please make sure verifications and tests pass.

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Apr 25, 2018

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:priority-beta-api branch 2 times, most recently from 538bd90 to 2542a83 Apr 25, 2018

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented Apr 25, 2018

Looks generally good, but I am not so sure about keeping v1alpha1. API folks can help us review that part.

You mean priorityClass in v1alpha1 or remove v1alpha1 in general?

Please make sure verifications and tests pass.

Yes. There was an issue with go 1.9 version on my machine for gofmt. Changing to 1.10.1 solved it.

@ravisantoshgudimetla

This comment has been minimized.

@bsalamat

This comment has been minimized.

Copy link
Contributor

bsalamat commented Apr 26, 2018

You mean priorityClass in v1alpha1 or remove v1alpha1 in general?

Removing priorityClass from v1alpha1 and since it is the only item there, maybe removing v1alpha1 as well. Basically, I would like to know what the recommended workflow is when we graduate an API to beta.

@k82cn

This comment has been minimized.

Copy link
Member

k82cn commented Apr 26, 2018

AFAIK, we should remove v1alpha1 in graduation :)

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented Apr 26, 2018

Removing priorityClass from v1alpha1 and since it is the only item there, maybe removing v1alpha1 as well. Basically, I would like to know what the recommended workflow is when we graduate an API to beta.

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Apr 30, 2018

@bsalamat can you check why it was removed from 1.11 milestone?

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented May 11, 2018

/retest

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented May 11, 2018

/retest

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:priority-beta-api branch from 1f530ec to aab8378 May 11, 2018

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented May 11, 2018

/test pull-kubernetes-e2e-kops-aws

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:priority-beta-api branch from aab8378 to 99a3484 May 12, 2018

@@ -302,7 +302,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
HugePages: {Default: true, PreRelease: utilfeature.Beta},
DebugContainers: {Default: false, PreRelease: utilfeature.Alpha},
PodShareProcessNamespace: {Default: false, PreRelease: utilfeature.Alpha},
PodPriority: {Default: false, PreRelease: utilfeature.Alpha},

This comment has been minimized.

@liggitt

liggitt May 12, 2018

Member

@aveshagarwal are the quota controls in place to allow this to be enabled by default? (flipping the feature gate isn't required to add v1beta1 priority classes)

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla May 12, 2018

Author Contributor

@liggitt I believe you are talking about #57963, this got an lgtm but waiting on approval from @bsalamat

This comment has been minimized.

@liggitt

liggitt May 12, 2018

Member

yes, that. until that is available, we should not enable the feature by default.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla May 12, 2018

Author Contributor

Ok, I will make this alpha so that this PR can go ahead.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla May 12, 2018

Author Contributor

Done. PTAL

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented May 12, 2018

/lgtm

ravisantoshgudimetla added some commits May 9, 2018

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:priority-beta-api branch from 99a3484 to f20bd00 May 12, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 12, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented May 12, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 12, 2018

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented May 12, 2018

/retest

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented May 12, 2018

@liggitt Thanks for the review.

/assign @smarterclayton

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 12, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@liggitt @ravisantoshgudimetla @smarterclayton

Pull Request Labels
  • sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help
@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 14, 2018

/approve

based on api approver comments and sig approval

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, ravisantoshgudimetla, smarterclayton

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented May 14, 2018

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 14, 2018

Automatic merge from submit-queue (batch tested with PRs 55511, 63372, 63400, 63100, 63769). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit a1b54f3 into kubernetes:master May 14, 2018

16 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation ravisantoshgudimetla authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@ravisantoshgudimetla ravisantoshgudimetla deleted the ravisantoshgudimetla:priority-beta-api branch May 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.