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

Move scheduler extender API V1 to staging k8s.io/kube-scheduler #88540

Merged
merged 2 commits into from Feb 28, 2020

Conversation

@damemi
Copy link
Contributor

damemi commented Feb 25, 2020

What type of PR is this?
/kind feature

What this PR does / why we need it:
This moves the Extender api for the scheduler, previously under pkg/scheduler/apis/extender/v1 to the k8s.io/kube-scheduler staging repo

Which issue(s) this PR fixes:
Fixes #88536

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Scheduler Extender API is now located under k8s.io/kube-scheduler/extender

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@damemi

This comment has been minimized.

Copy link
Contributor Author

damemi commented Feb 25, 2020

/sig scheduling

@damemi

This comment has been minimized.

Copy link
Contributor Author

damemi commented Feb 25, 2020

/cc @ahg-g

@k8s-ci-robot k8s-ci-robot requested a review from ahg-g Feb 25, 2020
@k8s-ci-robot k8s-ci-robot requested review from dixudx and draveness Feb 25, 2020
@damemi damemi force-pushed the damemi:move-extender-api-to-staging branch from 9610548 to 19e2409 Feb 25, 2020
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Feb 25, 2020

/cc @liggitt

@k8s-ci-robot k8s-ci-robot requested a review from liggitt Feb 25, 2020
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Feb 25, 2020

I think we need to define an internal type that the versioned type gets converted to.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 25, 2020

There are quite a few unusual things going on with this types:

  1. No json tags defined (so all fields serialize capitalized)
  2. No internal type or conversions defined
  3. No TypeMeta fields in the objects (so no apiVersion/kind identification is possible)

I'd like to understand more about how these are used/documented today

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Feb 25, 2020

I don't know the history of those types, and why they are not properly setup, this API seems to have been around for sometime.

This API is used to define request/reply types for the scheduler to call out to external services (called Extenders) to make decisions when scheduling a pod (filtering, scoring and preemption).

The only documentation I could find was this: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/scheduling/scheduler_extender.md

@damemi damemi changed the title Move scheduler extender API V1 to staging k8s.io/kube-scheduler [wip] Move scheduler extender API V1 to staging k8s.io/kube-scheduler Feb 26, 2020
@alculquicondor

This comment has been minimized.

Copy link
Member

alculquicondor commented Feb 26, 2020

I think we can all agree that this API is not well defined.

However, we can not fix it easily. So I think we have to add some api-violation exceptions and then release a v2 next cycle.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 27, 2020

Add an OWNERS file to the extenders dir:

# See the OWNERS docs at https://go.k8s.io/owners

# Disable inheritance as this is an api owners file
options:
  no_parent_owners: true
approvers:
- api-approvers
reviewers:
- api-reviewers
- sig-scheduling
labels:
- kind/api-change
- sig/scheduling
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 27, 2020

I think we can all agree that this API is not well defined.

However, we can not fix it easily. So I think we have to add some api-violation exceptions and then release a v2 next cycle.

Are there any tests that would catch attempts to "fix" these types by adding lowercased json tags (since that would break compatibility with existing extenders)?

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 27, 2020

Locating the types in an exported package makes sense. Make sure there's an OWNERS file covering the directory that routes to API reviewers/approvers, and add a test that ensures the following work, then lgtm:

  • populating a struct, then encoding, produces upper-case field names
  • upper-case field names can be decoded into the struct
  • lower-case field names can be decoded into the struct
@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels Feb 27, 2020
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 27, 2020

/approve

lgtm, will probably need to fixup the publishing rules yaml file (the verify job will tell you if that's needed). will leave final lgtm to @kubernetes/sig-scheduling-pr-reviews

@damemi damemi changed the title [wip] Move scheduler extender API V1 to staging k8s.io/kube-scheduler Move scheduler extender API V1 to staging k8s.io/kube-scheduler Feb 27, 2020
Copy link
Member

alculquicondor left a comment

LGTM after all tests are fixed :)

"k8s.io/apimachinery/pkg/types"
)

func TestCompatibility(t *testing.T) {

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Feb 27, 2020

Member

Add a comment indicating what is this checking and why. Perhaps also leave a TODO with an open issue for a v2.

@damemi damemi force-pushed the damemi:move-extender-api-to-staging branch 2 times, most recently from c74623f to 7ac5648 Feb 27, 2020

// TestCompatibility verifies that the types in extender/v1 can be successfully encoded to json and decoded back, even when lowercased,
// since these types were written around JSON tags and we need to enforce consistency on them now.
// @TODO: v2 of these types should be defined with proper JSON tags to enforce this

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Feb 27, 2020

Member

are you missing a sentence here? 😅

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Feb 27, 2020

Member

Also, prefer something like TODO(#1234) for tracking

This comment has been minimized.

Copy link
@damemi

damemi Feb 27, 2020

Author Contributor

I don't think so, is there something you'd like me to add? Opened an issue for this here: #88634

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Feb 27, 2020

Member

It's not clear what "this" means in v2 of these types should be defined with proper JSON tags to enforce this

This comment has been minimized.

Copy link
@liggitt

liggitt Feb 27, 2020

Member

I would just reference the issue for making the API more consistent with other serialized APIs, and put details in that issue.

This comment has been minimized.

Copy link
@damemi

damemi Feb 27, 2020

Author Contributor

Did a little of both, I agree we can discuss details in the issue

@damemi damemi force-pushed the damemi:move-extender-api-to-staging branch 2 times, most recently from a44ad5e to ced37e6 Feb 27, 2020
@alculquicondor

This comment has been minimized.

Copy link
Member

alculquicondor commented Feb 27, 2020

/lgtm

@damemi damemi force-pushed the damemi:move-extender-api-to-staging branch from ced37e6 to e398302 Feb 27, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm label Feb 27, 2020
@damemi

This comment has been minimized.

Copy link
Contributor Author

damemi commented Feb 27, 2020

@alculquicondor had to update import restrictions for kube-scheduler to allow k8s.io/api

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 27, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, liggitt

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

Copy link
Member

Huang-Wei left a comment

/lgtm

Thanks @damemi.

@Huang-Wei

This comment has been minimized.

Copy link
Member

Huang-Wei commented Feb 28, 2020

A side note: this PR is actually a leftover I didn't finish in last release, when I thought the serialized extender APIs would be better to behave in an unversioned way, while staging/... usually holds versioned APIs, so I left them in pkg/scheduler/apis.

Although it looks unusual, it seems @liggitt is ok with putting unversioned APIs in staging/..., so I'm glad to see this PR moving forward, so that extender users don't need to vendor k/k.

BTW: I have some thoughts on future refactoring on incorporating TypeMeta into this API, we can discuss the details in #88634.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 28, 2020

Although it looks unusual, it seems @liggitt is ok with putting unversioned APIs in staging/..., so I'm glad to see this PR moving forward, so that extender users don't need to vendor k/k.

It was already exposed as an API via the extender framework. Keeping the go types in a hard-to-use location didn't improve that. Making our API types accessible, adding tests to ensure we stay compatible with them, and making plans to improve their structure in the future is the best thing we can do.

@k8s-ci-robot k8s-ci-robot merged commit e25ff53 into kubernetes:master Feb 28, 2020
18 checks passed
18 checks passed
cla/linuxfoundation damemi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ipv6-parallel Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 28, 2020
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.