Switch internal scale type to autoscaling, enable apps/v1 scale subresources #55413

liggitt · 2017-11-09T15:59:48Z

Switch workload internal scale type to autoscaling.Scale (internal-only change)
Enable scale subresources for apps/v1 deployments, replicasets, statefulsets

NONE

sttts · 2017-11-09T16:27:09Z

pkg/apis/apps/v1beta1/conversion.go

 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/kubernetes/pkg/apis/apps"
+	autoscaling "k8s.io/kubernetes/pkg/apis/autoscaling"


nit: no need for the alias

liggitt · 2017-11-09T17:11:13Z

cc @crimsonfaith91 @DirectXMan12

crimsonfaith91 · 2017-11-09T18:23:41Z

@liggitt I think the round-tripping test failed due to different selector type for autoscaling.Scale and extensions.Scale. The selector field is a string for autoscaling.Scale, but it is LabelSelector type for extensions.Scale. Correct me if i am wrong.

liggitt · 2017-11-09T18:29:22Z

it's not because it's a different type, it's because the fuzzed data is an invalid selector. making a custom fuzzer now that only allows valid selectors.

liggitt · 2017-11-09T18:47:19Z

Fixed fuzzing test, added a commit to enable scale subresources for apps/v1

liggitt · 2017-11-09T19:36:12Z

cc @kubernetes/sig-apps-api-reviews @kubernetes/api-reviewers

crimsonfaith91 · 2017-11-09T20:10:31Z

pkg/apis/apps/v1beta1/conversion.go

 	}
+	if len(selector.MatchExpressions) == 0 {


Do we need to handle the case when len(selector.MatchExpressions) != 0?

no, if MatchExpressions is non-empty, then MatchLabels is insufficient to express the full selector, and we should not set it.

This is the same logic as before, only setting out.Selector = in.Selector.MatchLabels when MatchExpressions is zero-length:

if in.Selector.MatchExpressions == nil || len(in.Selector.MatchExpressions) == 0 { out.Selector = in.Selector.MatchLabels }

Good point, thanks!

crimsonfaith91 · 2017-11-09T20:13:36Z

pkg/apis/extensions/types.go

@@ -42,44 +42,6 @@ const (
 	SysctlsPodSecurityPolicyAnnotationKey string = "security.alpha.kubernetes.io/sysctls"
 )

-// describes the attributes of a scale subresource


Just to verify - removing these will not break backward compatibility, right?

Correct, as seen by there being zero changes in the openapi/swagger schemas (only apps/v1 additions), or external API types, or client-go methods.

crimsonfaith91 · 2017-11-09T20:25:58Z

pkg/registry/extensions/controller/storage/storage.go

@@ -58,7 +60,7 @@ var _ = rest.Patcher(&ScaleREST{})

 // New creates a new Scale object
 func (r *ScaleREST) New() runtime.Object {
-	return &extensions.Scale{}
+	return &autoscaling.Scale{}


@liggitt IMO we should not change any codes in pkg/registry/extensions/controller/storage directory as it is relevant to replication controller, which is v1.

not an option, the internal extensions.Scale type is going away. This has no external API implications.

Also, this is only used for /apis/extensions/v1beta1/namespaces/ns/replicationcontrollers/rc/scale, which is not how replication controllers should be scaled. The /api/v1/namespaces/ns/replicationcontrollers/rc/scale API is the v1 scale subresource for replication controllers

noted! didn't know about that, thanks for pointing it out!

crimsonfaith91 · 2017-11-09T20:29:42Z

/lgtm

k8s-github-robot · 2017-11-09T20:31:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [liggitt]
~~docs/api-reference/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/client/OWNERS~~ [liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

liggitt · 2017-11-09T20:34:40Z

/hold
would like ack on API review

kow3ns

LGTM
IMO this fixes #49504
Can we add a release note that indicates the app/v1 group uses autoscaling/v1 Scale.

liggitt · 2017-11-10T13:43:29Z

IMO this fixes #49504

this lays the groundwork for it, but until kubectl scale actually uses discovery and scale subresources consistently, that should stay open

Can we add a release note that indicates the app/v1 group uses autoscaling/v1 Scale.

I can... since apps/v1 is new in this release, I figured we'd combine release notes for all the apps/v1 stuff into a single note, not per-PR

liggitt · 2017-11-10T13:43:56Z

/hold cancel

k8s-github-robot · 2017-11-10T15:00:42Z

Automatic merge from submit-queue (batch tested with PRs 53047, 54861, 55413, 55395, 55308). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot assigned sttts and lavalamp Nov 9, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2017

liggitt mentioned this pull request Nov 9, 2017

change extensions.Scale to autoscaling.Scale for apps/v1 statefulset #55354

Closed

liggitt force-pushed the internal-autoscaling branch from 64fd8f9 to 7aff354 Compare November 9, 2017 16:25

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2017

sttts reviewed Nov 9, 2017

View reviewed changes

liggitt force-pushed the internal-autoscaling branch from 7aff354 to 348b994 Compare November 9, 2017 16:49

k8s-github-robot added the kind/new-api label Nov 9, 2017

liggitt force-pushed the internal-autoscaling branch from 348b994 to 88ac9c6 Compare November 9, 2017 17:16

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 9, 2017

liggitt force-pushed the internal-autoscaling branch from 88ac9c6 to 2bcc412 Compare November 9, 2017 17:19

liggitt assigned kow3ns and crimsonfaith91 and unassigned lavalamp Nov 9, 2017

liggitt added 4 commits November 9, 2017 13:42

Switch to autoscaling.Scale internally

9c1be33

generated files

f927f2a

Enable scale subresources for apps/v1

f9e2e40

Generated files

cccbf62

liggitt force-pushed the internal-autoscaling branch from 2bcc412 to cccbf62 Compare November 9, 2017 18:44

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 9, 2017

liggitt changed the title ~~Switch internal scale type to autoscaling~~ Switch internal scale type to autoscaling, enable apps/v1 scale subresources Nov 9, 2017

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Nov 9, 2017

crimsonfaith91 reviewed Nov 9, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2017

kow3ns approved these changes Nov 9, 2017

View reviewed changes

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

k8s-github-robot merged commit ae2edc4 into kubernetes:master Nov 10, 2017

liggitt deleted the internal-autoscaling branch November 10, 2017 17:47

frobware mentioned this pull request Nov 13, 2017

[e2e failure] [sig-autoscaling] [HPA] Horizontal pod autoscaling (scale resource: CPU) [sig-autoscaling] [Serial] [Slow] ReplicaSet Should scale ... #54574

Closed

shyamjvs mentioned this pull request Nov 14, 2017

HPA scale-up/scale-down e2e tests failing on large clusters #54637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch internal scale type to autoscaling, enable apps/v1 scale subresources #55413

Switch internal scale type to autoscaling, enable apps/v1 scale subresources #55413

liggitt commented Nov 9, 2017 •

edited

sttts Nov 9, 2017

liggitt commented Nov 9, 2017

crimsonfaith91 commented Nov 9, 2017 •

edited

liggitt commented Nov 9, 2017

liggitt commented Nov 9, 2017

liggitt commented Nov 9, 2017

crimsonfaith91 Nov 9, 2017

liggitt Nov 9, 2017 •

edited

crimsonfaith91 Nov 9, 2017

crimsonfaith91 Nov 9, 2017

liggitt Nov 9, 2017

crimsonfaith91 Nov 9, 2017

liggitt Nov 9, 2017

liggitt Nov 9, 2017

crimsonfaith91 Nov 9, 2017

crimsonfaith91 commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

liggitt commented Nov 9, 2017

kow3ns left a comment

liggitt commented Nov 10, 2017

liggitt commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

Switch internal scale type to autoscaling, enable apps/v1 scale subresources #55413

Switch internal scale type to autoscaling, enable apps/v1 scale subresources #55413

Conversation

liggitt commented Nov 9, 2017 • edited

Choose a reason for hiding this comment

liggitt commented Nov 9, 2017

crimsonfaith91 commented Nov 9, 2017 • edited

liggitt commented Nov 9, 2017

liggitt commented Nov 9, 2017

liggitt commented Nov 9, 2017

Choose a reason for hiding this comment

liggitt Nov 9, 2017 • edited

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

crimsonfaith91 commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

liggitt commented Nov 9, 2017

kow3ns left a comment

Choose a reason for hiding this comment

liggitt commented Nov 10, 2017

liggitt commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

liggitt commented Nov 9, 2017 •

edited

crimsonfaith91 commented Nov 9, 2017 •

edited

liggitt Nov 9, 2017 •

edited