Fix API encoding inconsistencies in KubeSchedulerConfig #91625

pancernik · 2020-06-01T11:41:03Z

What type of PR is this?
/kind api-change

What this PR does / why we need it:
v1 Scheduler Policy config includes Extender type with an inconsistent API encoding.

We fix it by Extender type with a correct API encoding in v1beta1 KubeSchedulerConfiguraton

Which issue(s) this PR fixes:

Fixes #65414

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`v1beta1` Scheduler `Extender` encoding is case-sensitive (`v1alpha1`/`v1alpha2` was case-insensitive), its `httpTimeout` field uses duration encoding (for example, one second is specified as `"1s"`), and the `enableHttps` field in `v1alpha1`/`v1alpha2` was renamed to `enableHTTPS`.

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/30bc12a2e26a06a1995c379df125fc6f6ae58e77/keps/sig-scheduling/785-scheduler-component-config-api

k8s-ci-robot · 2020-06-01T11:41:11Z

Hi @pancernik. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

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

pancernik · 2020-06-01T11:43:32Z

/cc @alculquicondor

fejta-bot · 2020-06-01T11:48:49Z

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.

Huang-Wei · 2020-06-02T00:40:48Z

/ok-to-test

Huang-Wei · 2020-06-02T00:46:45Z

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

@@ -102,7 +102,7 @@ type KubeSchedulerConfiguration struct {
 	// 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,omitempty"`
+	Extenders []Extender `json:"extenders,omitempty"`


So this means we currently have two Extenders copies in v1 and v1beta1, right?

Yes, Policy includes the old type. It will be removed once KubeSchedulerConfiguration fully replaces Policy.

Once we graduation KubeSchedulerConfiguration to GA, we can rename v1.Extender to v1.DeprecatedExtender.

this is super subtle... can we name this more clearly to make sure we don't use the wrong struct and flip encoding of the enableHttps field?

if we can rename the v1.Extender type later, then we can rename it now, if so, lets do it in this PR (perhaps we can using Legacy instead of Deprecated).

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

pancernik · 2020-06-02T08:24:32Z

/test pull-kubernetes-kubemark-e2e-gce-big

pancernik · 2020-06-02T08:24:40Z

/test pull-kubernetes-e2e-gce-100-performance

alculquicondor · 2020-06-02T15:02:49Z

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

@@ -102,7 +102,7 @@ type KubeSchedulerConfiguration struct {
 	// 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,omitempty"`
+	Extenders []Extender `json:"extenders,omitempty"`


Once we graduation KubeSchedulerConfiguration to GA, we can rename v1.Extender to v1.DeprecatedExtender.

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

alculquicondor · 2020-06-02T15:10:13Z

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

+	if err := autoConvert_config_Extender_To_v1beta1_Extender(in, out, s); err != nil {
+		return err
+	}
+	out.HTTPTimeout.Duration = in.HTTPTimeout


@liggitt why is this not handled by the generated conversion? Is it an anti-pattern to use time.Duration as the internal type?

Yep, I had a feeling that it should be handled there.

Other parts of k8s use metav1.Duration in internal type which offers no advantages over time.Duration apart of autoconvert.

use metav1.Duration in API type fields

pkg/scheduler/apis/config/testing/policy_test.go

liggitt · 2020-06-02T16:32:18Z

pkg/scheduler/apis/config/v1beta1/zz_generated.conversion.go

+	out.BindVerb = in.BindVerb
+	out.EnableHTTPS = in.EnableHTTPS
+	out.TLSConfig = (*config.ExtenderTLSConfig)(unsafe.Pointer(in.TLSConfig))
+	// WARNING: in.HTTPTimeout requires manual conversion: inconvertible types (k8s.io/apimachinery/pkg/apis/meta/v1.Duration vs time.Duration)


use meta/v1.Duration internally and avoid a manual conversion

Will still need to do a manual conversion from v1.Extender.

duplicate the type in pkg/scheduler/apis/config/types.go renaming the existing one as LegacyExtender.

Actually, that might not work, please check... but you can do the "manual" conversion in the type that will eventually go away in v1 folder.

If we duplicated Extender as LegacyExtender, we would still have to maintain a translation between them as we would use only one internally. I think it's better to do the manual conversion from v1.LegacyExtender.

correct, only one internally and do manual conversion from the legacy one.

alculquicondor · 2020-06-02T20:41:27Z

Please follow the suggestion here regarding release notes #91580 (comment)

pancernik · 2020-06-10T23:11:51Z

/retest

pancernik · 2020-06-23T12:38:18Z

/test pull-kubernetes-integration

alculquicondor · 2020-06-23T15:12:38Z

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

+		if err := Convert_v1_ExtenderTLSConfig_To_config_ExtenderTLSConfig(in.TLSConfig, out.TLSConfig, s); err != nil {
+			return err
+		}
+	}


IIRC, generated code also adds:

else { out.TLSConfig = nil }

Yep, that makes sense.

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

pancernik · 2020-06-23T17:31:56Z

/test pull-kubernetes-integration

pancernik · 2020-06-23T18:48:38Z

/retest

alculquicondor · 2020-06-23T19:22:42Z

lgtm, but will stamp after @liggitt's approval and squash

liggitt · 2020-06-24T04:41:00Z

/approve
/retest

k8s-ci-robot · 2020-06-24T04:41:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, liggitt, pancernik

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:

~~api/OWNERS~~ [liggitt]
~~hack/OWNERS~~ [liggitt]
~~pkg/scheduler/OWNERS~~ [alculquicondor,liggitt]
~~staging/src/k8s.io/kube-scheduler/config/OWNERS~~ [liggitt]

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

pancernik · 2020-06-24T12:43:56Z

/test pull-kubernetes-e2e-kind-ipv6

alculquicondor · 2020-06-24T14:36:25Z

/lgtm

pancernik · 2020-06-24T14:46:21Z

/sig scheduling

alculquicondor · 2020-06-24T15:16:16Z

/remove-sig scheduling

alculquicondor · 2020-06-24T15:16:33Z

/sig scheduling

clearly, there is a bug in the bot

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 1, 2020

k8s-ci-robot requested review from Huang-Wei and thockin June 1, 2020 11:41

pancernik commented Jun 1, 2020

View reviewed changes

staging/src/k8s.io/kube-scheduler/config/v1beta1/types.go Show resolved Hide resolved

k8s-ci-robot requested a review from alculquicondor June 1, 2020 11:43

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 2, 2020

Huang-Wei reviewed Jun 2, 2020

View reviewed changes

alculquicondor reviewed Jun 2, 2020

View reviewed changes

liggitt reviewed Jun 2, 2020

View reviewed changes

pkg/scheduler/apis/config/testing/policy_test.go Show resolved Hide resolved

liggitt reviewed Jun 2, 2020

View reviewed changes

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

pancernik force-pushed the v1beta1-extender-encoding branch from f6cb1a7 to f94b358 Compare June 10, 2020 17:50

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

pancernik force-pushed the v1beta1-extender-encoding branch 2 times, most recently from bc1a62c to 4ef2c9c Compare June 10, 2020 21:07

pancernik force-pushed the v1beta1-extender-encoding branch from 4ef2c9c to d2ec07d Compare June 23, 2020 10:23

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 23, 2020

alculquicondor reviewed Jun 23, 2020

View reviewed changes

pkg/scheduler/apis/config/v1/conversion.go Show resolved Hide resolved

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

Fix API encoding inconsistencies in KubeSchedulerConfig

d7c84e1

pancernik force-pushed the v1beta1-extender-encoding branch from 26719b4 to d7c84e1 Compare June 24, 2020 08:04

k8s-ci-robot assigned alculquicondor Jun 24, 2020

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

k8s-ci-robot removed the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Jun 24, 2020

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 24, 2020

k8s-ci-robot merged commit 67afc8e into kubernetes:master Jun 24, 2020

k8s-ci-robot added this to the v1.19 milestone Jun 24, 2020

alculquicondor mentioned this pull request Jun 24, 2020

v1.Extender.HTTPTimeout is a time.Duration #89396

Closed

github-actions bot mentioned this pull request Jun 30, 2020

Week Ending June 28, 2020 dev-obs/actus#187

Open

pancernik mentioned this pull request Jul 3, 2020

REQUEST: New membership for pancernik kubernetes/org#1987

Closed

6 tasks

alculquicondor mentioned this pull request May 17, 2021

Use aliases in sig apps and scheduling APIs OWNERS #102066

Merged

Fix API encoding inconsistencies in KubeSchedulerConfig #91625

Fix API encoding inconsistencies in KubeSchedulerConfig #91625

Conversation

pancernik commented Jun 1, 2020 • edited by liggitt Loading

k8s-ci-robot commented Jun 1, 2020

pancernik commented Jun 1, 2020

fejta-bot commented Jun 1, 2020

Huang-Wei commented Jun 2, 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

pancernik commented Jun 2, 2020

pancernik commented Jun 2, 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 Jun 2, 2020

pancernik commented Jun 10, 2020

pancernik commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pancernik commented Jun 23, 2020

pancernik commented Jun 23, 2020

alculquicondor commented Jun 23, 2020

liggitt commented Jun 24, 2020

k8s-ci-robot commented Jun 24, 2020

pancernik commented Jun 24, 2020

alculquicondor commented Jun 24, 2020

pancernik commented Jun 24, 2020

alculquicondor commented Jun 24, 2020

alculquicondor commented Jun 24, 2020

pancernik commented Jun 1, 2020 •

edited by liggitt

Loading