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

Add Extenders to v1alpha2 Component Config #88768

Merged
merged 1 commit into from Mar 6, 2020

Conversation

@damemi
Copy link
Contributor

damemi commented Mar 3, 2020

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change

What this PR does / why we need it:
Adds an Extenders []Extender field to the v1alpha2 Scheduler ComponentConfig, since once AlgorithmSource is deprecated (#87999) there will be no other way to specify extenders

Which issue(s) this PR fixes:

Fixes #88529

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Scheduler Extenders can now be configured in the v1alpha2 component config

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


@alculquicondor

This comment has been minimized.

Copy link
Member

alculquicondor commented Mar 3, 2020

/priority critical-urgent

Copy link
Member

alculquicondor left a comment

Please add the implementation in a separate commit

// Extenders are the list of scheduler extenders, each holding the values of how to communicate
// with the extender. These extenders are shared by all scheduler profiles.
// +listType=set
Extenders []v1.Extender `json:"extenders"`

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Mar 3, 2020

Member

I think this requires to update stating/publishing/import-restrictions.yaml

@alculquicondor

This comment has been minimized.

Copy link
Member

alculquicondor commented Mar 3, 2020

Also, in the first commit, include validation. Just refactor the part that refers to Extenders in pkg/scheduler/apis/config/validation.ValidatePolicy

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 3, 2020

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@damemi damemi force-pushed the damemi:extenders-cc branch from 71458dc to f59712b Mar 3, 2020
@damemi damemi force-pushed the damemi:extenders-cc branch from f59712b to 70f9ece Mar 3, 2020
@@ -71,6 +71,36 @@ func ValidateKubeSchedulerConfiguration(cc *config.KubeSchedulerConfiguration) f
allErrs = append(allErrs, field.Invalid(field.NewPath("podMaxBackoffSeconds"),
cc.PodMaxBackoffSeconds, "must be greater than or equal to PodInitialBackoffSeconds"))
}

binders := 0

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Mar 3, 2020

Member

prefer to refactor out the existing checks from the other function to use them here

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 4, 2020
@@ -208,7 +188,7 @@ func (c *Configurator) createFromProvider(providerName string) (*Scheduler, erro
prof.Plugins = plugins
}

return c.create(extenders)

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Mar 4, 2020

Member

no need to pass them, as they are part of c already.

This comment has been minimized.

Copy link
@damemi

damemi Mar 4, 2020

Author Contributor

True, I had that to remain compatible with createFromConfig but I see that where it parses out the extenders can also just store them in c

pkg/scheduler/framework/plugins/registry.go Outdated Show resolved Hide resolved
nodeaffinity.Name: nodeaffinity.New,
podtopologyspread.Name: podtopologyspread.New,
nodeunschedulable.Name: nodeunschedulable.New,
noderesources.FitName: func(plArgs *runtime.Unknown, handle framework.FrameworkHandle) (framework.Plugin, error) {

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Mar 4, 2020

Member

the alternative (which I prefer) is to get rid of FitArgs altogether and only ignore what is deduced from the extenders.

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Mar 4, 2020

Member

Let's remove FitArgs

This comment has been minimized.

Copy link
@damemi

damemi Mar 4, 2020

Author Contributor

done

extenders = append(extenders, ignorableExtenders...)
}

registry := frameworkplugins.NewInTreeRegistry(frameworkplugins.WithIgnoredResources(ignoredResources...))

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Mar 4, 2020

Member

Uhm... maybe we should move this until after we have processed extenders in createFromConfig.

This comment has been minimized.

Copy link
@damemi

damemi Mar 4, 2020

Author Contributor

I think we need the registry before that... createFromConfig calls create, which calls buildFramework (a wrapper for framework.NewFramework, which takes the registry as a param)

if err != nil {
return nil, err
}
plugin, err := noderesources.NewFit(&runtime.Unknown{Raw: newArgs}, handle)

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Mar 4, 2020

Member

You can change NewFit to have any signature. For example NewFit(opts.IgnoredResources).

This comment has been minimized.

Copy link
@damemi

damemi Mar 4, 2020

Author Contributor

Yeah, I was trying to see if we could just wrap it for this one case to keep the code changes to a minimum but that's what I'll do

@@ -187,7 +188,7 @@ func (c *Configurator) createFromProvider(providerName string) (*Scheduler, erro
prof.Plugins = plugins
}

return c.create([]core.SchedulerExtender{})
return c.create(c.extenders)

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Mar 4, 2020

Member

Given extenders is embedded in c, isn't that we can directly call c.create()?

This comment has been minimized.

Copy link
@damemi

damemi Mar 4, 2020

Author Contributor

Yeah, see #88768 (comment) my mistake

@damemi

This comment has been minimized.

Copy link
Contributor Author

damemi commented Mar 4, 2020

Pushed more changes in a new commit to remove fitargs and address other feedback

Still need to add tests for the extenders in componentconfig (including parsing the ignored resources)

@damemi damemi force-pushed the damemi:extenders-cc branch from 3b0eccb to 5d46bce Mar 5, 2020
@damemi damemi force-pushed the damemi:extenders-cc branch 3 times, most recently from 45d235e to c8e7940 Mar 5, 2020
@alculquicondor

This comment has been minimized.

Copy link
Member

alculquicondor commented Mar 5, 2020

/approve

/assign @lavalamp

@@ -20,6 +20,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
componentbaseconfigv1alpha1 "k8s.io/component-base/config/v1alpha1"
v1 "k8s.io/kube-scheduler/config/v1"

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 5, 2020

Member

Whoa, this is blowing my mind, I would have expected this to be v2alpha1 if you already have a v1?

At any rate, one generally copies types rather than cross-link versions like this.

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Mar 5, 2020

Member

The scheduler has currently 2 APIs: Policy (v1) and componentconfig (v1alpha2)

They aren't necessarily mutually exclusive and once graduated to GA, componentconfig will be appended in v1.

So, is it at that point that we should deduplicate the types? I assume we can still share the internal types, right?

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 5, 2020

Member

ah, I see. This is probably OK then.

@damemi damemi force-pushed the damemi:extenders-cc branch from c8e7940 to bea8a70 Mar 5, 2020
@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Mar 5, 2020

/approve

for api/config file

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 5, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, damemi, lavalamp

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

Weight: 1,
EnableHTTPS: false,
})

This comment has been minimized.

Copy link
@ahg-g

ahg-g Mar 5, 2020

Member

where are those configurations being used in the tests?

This comment has been minimized.

Copy link
@damemi

damemi Mar 5, 2020

Author Contributor

Ah I didn't need all those fields, and I forgot to enable these tests... fixed

@damemi damemi force-pushed the damemi:extenders-cc branch from bea8a70 to 3033e0b Mar 5, 2020
@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Mar 5, 2020

/lgtm
/hold

do you want to squash or keep them in separate commits?

@damemi damemi force-pushed the damemi:extenders-cc branch from 3033e0b to 1d7006c Mar 5, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 5, 2020
@damemi

This comment has been minimized.

Copy link
Contributor Author

damemi commented Mar 5, 2020

@ahg-g squashed

@ahg-g

This comment has been minimized.

Copy link
Member

ahg-g commented Mar 5, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 5, 2020
@Huang-Wei

This comment has been minimized.

Copy link
Member

Huang-Wei commented Mar 5, 2020

/lgtm
/retest

@alculquicondor

This comment has been minimized.

Copy link
Member

alculquicondor commented Mar 6, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 8b8dd79 into kubernetes:master Mar 6, 2020
17 checks passed
17 checks passed
cla/linuxfoundation damemi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test 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
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.