Deprecate scheduler AlgorithmSource from v1alpha2 ComponentConfig #87999

damemi · 2020-02-10T22:12:00Z

What type of PR is this?
/kind deprecation

What this PR does / why we need it:
Removes AlgorithmSource from the v1alpha2 scheduler config API. This field is no longer needed and its removal is the first step toward deprecating policy config in favor of CC.

Which issue(s) this PR fixes:

Part of #87526

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

AlgorithmSource is removed from v1alpha2 Scheduler ComponentConfig

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/785
- [Other doc]: https://github.com/kubernetes/kubernetes/issues/87617

damemi · 2020-02-10T22:13:06Z

/hold
right now I have only taken the steps to remove the field from the API and add the manual conversion and defaulting updates. Please check my conversion functions, as the v1alpha2->config conversion is basically just what was removed from defaults.go

Next commits will actually remove this field from functional code (if that's the goal right now)

damemi · 2020-02-11T17:55:40Z

/hold cancel

ahg-g · 2020-02-11T18:25:43Z

pkg/scheduler/apis/config/v1alpha2/conversion.go

+	"unsafe"
+)
+
+func Convert_v1alpha2_KubeSchedulerConfiguration_To_config_KubeSchedulerConfiguration(in *v1alpha2.KubeSchedulerConfiguration, out *config.KubeSchedulerConfiguration, s conversion.Scope) error {


can you add unit tests for both functions?

pkg/scheduler/apis/config/v1alpha2/conversion.go

alculquicondor · 2020-02-11T18:20:37Z

pkg/scheduler/apis/config/v1alpha2/conversion.go

+}
+
+func Convert_config_KubeSchedulerConfiguration_To_v1alpha2_KubeSchedulerConfiguration(in *config.KubeSchedulerConfiguration, out *v1alpha2.KubeSchedulerConfiguration, s conversion.Scope) error {
+	out.TypeMeta = in.TypeMeta


why not use autoConvert_config_KubeSchedulerConfiguration_To_v1alpha2_KubeSchedulerConfiguration?

alculquicondor · 2020-02-11T18:24:52Z

pkg/scheduler/apis/config/v1alpha2/conversion.go

+		out.Plugins = nil
+	}
+	out.PluginConfig = *(*[]v1alpha2.PluginConfig)(unsafe.Pointer(&in.PluginConfig))
+	return nil


This one is tricky, but I'm not sure if we should care.

Theoretically, if there is an AlgorithmSource in the internal configuration, we should obtain the diff that needs to be applied to Plugins and Pluginconfig. But this is only needed if someone tries to do something like v1alpha1->internal->v1alpha2.

This is something that can be done by passing --config in v1alpha1 and --write-config-to, which writes to v1alpha2 all the time.

Again, should we care? I would say it's not worth the effort.

According to the API conventions, we need to be able to round-trip to older versions without losing any information (see item 4 in this list: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility)

So that brings up a good question, because like you suggested here we can just use the autoConvert function, but how do we handle roundtripping AlgorithmSource? We need to keep that info somewhere (like an annotation?), or we can just drop it once the Scheduler no longer supports AlgorithmSource.

And in this point, how can we get the information from AlgorithmSource that needs to be applied to Plugins and PluginConfig? If it's a configmap, I suppose we could read from that, but if it's a policy file that becomes tougher.

Better information more specific to deprecation here, but I'm still not clear. I think an annotation is the way it suggests? https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-parts-of-the-api

Though, this api is only in alpha so it might not be relevant for us. I'm ok with just dropping the information too

I would expect the round-trip requirement to be specific to APIs in which multiple clients interact with different versions of the same resource (e.g. REST APIs).

API types used in config files are input-only, and have multiple producers but a single consumer (the component whose configuration they are driving)

I think our policy doesn't cover it while alpha either way.

I think it's confusing at best to have config file formats that are like our api objects but have different rules. I don't have specific case in which they go wrong, though. But I expect the inconsistency will eventually be a problem, e.g. maybe people start making these into CRDs for some strange reason. Anyway, I don't have concerns for this particular change.

In general, these configuration types are not supposed to be outputted. If we could add some type of annotation in them such that no internal->versioned conversion code is auto-generated, that would be a win already.

alculquicondor · 2020-02-11T18:27:18Z

pkg/scheduler/apis/config/v1alpha2/conversion.go

@@ -0,0 +1,67 @@
+/*


add conversion_test, but I'm happy to only have v1alpha2->internal

alculquicondor · 2020-02-11T18:32:20Z

/priority important-longterm

damemi · 2020-02-11T19:14:39Z

/retest

fejta-bot · 2020-02-11T19:34:27Z

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.

ahg-g

/approve
/hold

ahg-g · 2020-02-11T20:25:23Z

pkg/scheduler/apis/config/v1alpha2/conversion_test.go

+	if out.AlgorithmSource.Provider != pointer.StringPtr(v1alpha2.SchedulerDefaultProviderName) {
+		t.Errorf("expected AlgorithmProvider to be %+v, got %+v", v1alpha2.SchedulerDefaultProviderName, out.AlgorithmSource.Provider)
+	}
+}


add empty line at the end.

And I believe you still need the exported function that calls the auto function. Just for it to compile.

damemi · 2020-02-14T16:15:07Z

@alculquicondor no problem, sometimes you can't avoid that. Rebased, please check if I missed anything

alculquicondor · 2020-02-14T16:47:38Z

pkg/scheduler/apis/config/v1alpha2/conversion_test.go

+)
+
+func TestV1alpha2ToConfigKubeSchedulerConfigurationConversion(t *testing.T) {
+	cases := []struct {


nit: I don't think we expect more cases, so no need for the cases slice.

alculquicondor · 2020-02-14T16:49:50Z

/lgtm

alculquicondor · 2020-02-18T21:30:28Z

Friendly ping @lavalamp

alculquicondor · 2020-02-24T14:53:28Z

@lavalamp PTAL

alculquicondor · 2020-02-25T02:55:52Z

@damemi there are new conflicts from the scheduling profiles. Please rebase.

alculquicondor · 2020-02-25T16:19:57Z

It looks like you need to run ./hack/update-codegen.sh again

This commit removes AlgorithmSource from the v1alpha2 scheduler ComponentConfig api. It also creates the necessary manual conversion functions and updates defaulting to accept the change.

alculquicondor · 2020-02-25T17:42:16Z

/retest

lavalamp · 2020-02-25T17:59:22Z

staging/src/k8s.io/kube-scheduler/config/v1alpha2/types.go

@@ -39,9 +39,6 @@ const (
 type KubeSchedulerConfiguration struct {
 	metav1.TypeMeta `json:",inline"`

-	// AlgorithmSource specifies the scheduler algorithm source.


Removing this will make alpha 1 and 2 not round-trippable. Which is ordinarily OK for alpha, but I thought you were relying on that for some of the other changes?

We only allow users to pass in one configuration (either v1alpha1 or v1alpha2). And then we use the internal types to build profiles.

We have a flag to write the configuration to a file and exit the scheduler. This only has debugging purposes (for kube-scheduler devs), but it would be broken with this change when using v1alpha1 (it only outputs in v1alpha2).

@lavalamp any additional concerns?

damemi · 2020-02-25T20:42:06Z

/retest

damemi · 2020-02-25T21:25:29Z

/retest

lavalamp · 2020-02-26T21:48:51Z

/approve

k8s-ci-robot · 2020-02-26T21:49:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, 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:

~~pkg/scheduler/apis/config/OWNERS~~ [ahg-g,lavalamp]
~~staging/src/k8s.io/kube-scheduler/config/OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alculquicondor · 2020-02-26T22:06:18Z

/lgtm

fejta-bot · 2020-02-27T04:31:33Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from liggitt and vishh February 10, 2020 22:13

damemi force-pushed the deprecate-algo-source branch 2 times, most recently from 572a429 to e346b43 Compare February 11, 2020 17:51

damemi changed the title ~~[wip] Deprecate scheduler AlgorithmSource from v1alpha2 ComponentConfig~~ Deprecate scheduler AlgorithmSource from v1alpha2 ComponentConfig Feb 11, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2020

ahg-g reviewed Feb 11, 2020

View reviewed changes

alculquicondor reviewed Feb 11, 2020

View reviewed changes

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 11, 2020

ahg-g reviewed Feb 11, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2020

damemi force-pushed the deprecate-algo-source branch from aad81cd to 2cd978f Compare February 11, 2020 20:26

alculquicondor reviewed Feb 14, 2020

View reviewed changes

k8s-ci-robot assigned alculquicondor Feb 14, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2020

damemi force-pushed the deprecate-algo-source branch from 285eb6f to ffe3005 Compare February 25, 2020 13:55

k8s-ci-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. labels Feb 25, 2020

damemi force-pushed the deprecate-algo-source branch from ffe3005 to d0d4e9d Compare February 25, 2020 14:27

alculquicondor mentioned this pull request Feb 25, 2020

Add Extenders to v1alpha2 Component Config #88529

Closed

Remove AlgorithmSource from v1alpha2

7e1ec59

This commit removes AlgorithmSource from the v1alpha2 scheduler ComponentConfig api. It also creates the necessary manual conversion functions and updates defaulting to accept the change.

damemi force-pushed the deprecate-algo-source branch from d0d4e9d to 7e1ec59 Compare February 25, 2020 16:42

lavalamp reviewed Feb 25, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2020

k8s-ci-robot merged commit 09edbcd into kubernetes:master Feb 27, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 27, 2020

damemi mentioned this pull request Mar 3, 2020

Add Extenders to v1alpha2 Component Config #88768

Merged

Deprecate scheduler AlgorithmSource from v1alpha2 ComponentConfig #87999

Deprecate scheduler AlgorithmSource from v1alpha2 ComponentConfig #87999

Conversation

damemi commented Feb 10, 2020 • edited Loading

damemi commented Feb 10, 2020 • edited Loading

damemi commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Feb 11, 2020

damemi commented Feb 11, 2020

fejta-bot commented Feb 11, 2020

ahg-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi commented Feb 14, 2020

Choose a reason for hiding this comment

alculquicondor commented Feb 14, 2020

alculquicondor commented Feb 18, 2020

alculquicondor commented Feb 24, 2020

alculquicondor commented Feb 25, 2020

alculquicondor commented Feb 25, 2020

alculquicondor commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi commented Feb 25, 2020

damemi commented Feb 25, 2020

lavalamp commented Feb 26, 2020

k8s-ci-robot commented Feb 26, 2020

alculquicondor commented Feb 26, 2020

fejta-bot commented Feb 27, 2020

damemi commented Feb 10, 2020 •

edited

Loading

damemi commented Feb 10, 2020 •

edited

Loading