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.

Sign up for GitHub

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

Jump to bottom

StatefulSet scale subresource #49168

Merged
merged 1 commit into from
Aug 8, 2017
Merged

StatefulSet scale subresource #49168

merged 1 commit into from
Aug 8, 2017

Conversation

crimsonfaith91
Copy link
Contributor

@crimsonfaith91 crimsonfaith91 commented Jul 19, 2017 •
edited
Loading

What this PR does / why we need it: This PR implements scale subresource for StatefulSet.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #46005

Special notes for your reviewer:

Release note:

StatefulSet uses scale subresource when scaling in accord with ReplicationController, ReplicaSet, and Deployment implementations.

Feature Checklist:

  • Introduce Registry interface for storage purpose
  • Introduce ScaleREST New(), Get() and Update() utility functions
  • Create a ScaleREST object at NewREST() and return it
  • Enable scale subresource by adding /scale field to the storage map

Testing Checklist:

  • Unit testing

    • Modify newStorage() to call NewStorage(), and change all unit tests accordingly
    • Add unit tests for ScaleREST Get() and Update() utility functions
    • Add missing unit test for ShortNames

  • Manual testing

    • Verify existence of the subresource using kubectl proxy command
    • Modify the subresource using curl via POST

  • e2e testing

    • Add e2e tests using RESTClient

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 19, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 19, 2017
@crimsonfaith91 crimsonfaith91 force-pushed the apps-v1beta2 branch 3 times, most recently from 924479f to d4151de Compare July 19, 2017 18:40
@crimsonfaith91 crimsonfaith91 mentioned this pull request Jul 19, 2017
Closed
@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jul 19, 2017
@crimsonfaith91 crimsonfaith91 changed the title WIP: scale subresource for StatefulSet WIP: StatefulSet scale subresource Jul 19, 2017
@k8s-github-robot k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 20, 2017
@crimsonfaith91 crimsonfaith91 changed the title WIP: StatefulSet scale subresource StatefulSet scale subresource Jul 20, 2017
@enisoc
Copy link
Member

enisoc commented Jul 20, 2017

Looks like a great start! It just needs integration/e2e tests to show that the subresource is properly plumbed through all the way.

@kow3ns kow3ns mentioned this pull request Jul 21, 2017
Closed
11 tasks
@crimsonfaith91 crimsonfaith91 force-pushed the apps-v1beta2 branch 3 times, most recently from 3a527a4 to 79a028d Compare July 21, 2017 18:15
@crimsonfaith91
Copy link
Contributor Author

crimsonfaith91 commented Aug 2, 2017 •
edited
Loading

@liggitt @enisoc @janetkuo The reason submit queue pull-kubernetes-e2e-gce-etcd3 test failed is v1beta2 storage is not registered for API server:

W0801 20:56:39.586156       5 genericapiserver.go:321] Skipping API apps/v1beta2 because it has no resources.

apps/v1beta2 is skipped due to VersionedResourcesStorageMap does not have the version as the key.

IMO there are two ways to resolve this:
(1) register the v1beta2 storage to the API server
(2) register scale subresource for v1beta1 and test with v1beta1

I will pursue (1), which requires understanding API server and storage codes. Registering the subresource to v1beta1 just for testing purpose does not sound like a good idea.

@crimsonfaith91 crimsonfaith91 force-pushed the apps-v1beta2 branch 3 times, most recently from 0727e9f to 976ac2a Compare August 3, 2017 19:31
liggitt
liggitt reviewed Aug 3, 2017
View reviewed changes
pkg/registry/apps/rest/storage_apps.go Outdated
storage["statefulsets/status"] = statefulsetStatusStorage
// enable scale subresource for v1beta1 for e2e testing
// we are not able to test v1beta2 as v1beta2 is disabled at master.go
// TODO (juntee): remove the subresource before 1.8 release
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason to remove this before 1.8... why wouldn't we want the scale subresource in v1beta1 as well?

Copy link
Contributor Author

@crimsonfaith91 crimsonfaith91 Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO there is no reason to add a new subresource to a version that will be retired soon. The only reason adding the /scale for v1beta1 is for e2e testing purpose. If having integration test in this PR is better, we should not add /scale to v1beta1 at the first place.
@kow3ns @foxish

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, i prefer spending more time investigating how to make e2e test works for v1beta2.

Copy link
Contributor Author

@crimsonfaith91 crimsonfaith91 Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt @kow3ns @foxish After discussion, /scale will NOT be remove for v1beta1 as there is no maintenance cost.

I plan to:
(1) change e2e test's version from v1beta1 to v1beta2 BEFORE code freeze
(2) replace e2e test with integration test sometime AFTER 1.8 release branches cut

Tell me if there is a better way. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to:
(1) change e2e test's version from v1beta1 to v1beta2 BEFORE code freeze
(2) replace e2e test with integration test sometime AFTER 1.8 release

SGTM. if the switch to integration test happens before code freeze, that works as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming! I opened issue #50109 for reference.

@kow3ns
Copy link
Member

kow3ns commented Aug 3, 2017

@janetkuo are you satisfied that @crimsonfaith91 has addressed your review?

@crimsonfaith91 crimsonfaith91 mentioned this pull request Aug 3, 2017
Closed
@liggitt
Copy link
Member

liggitt commented Aug 4, 2017

/approve, though still needs @kubernetes/api-approvers and LGTM

@kow3ns
Copy link
Member

kow3ns commented Aug 4, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2017
smarterclayton
smarterclayton reviewed Aug 7, 2017
View reviewed changes
test/e2e/apps/statefulset.go Outdated
@@ -47,6 +48,8 @@ const (
readTimeout = 60 * time.Second
)

var proxyRegexp = regexp.MustCompile("Starting to serve on 127.0.0.1:([0-9]+)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton Thanks for reviewing! Yes, this line is unnecessary. :)

@smarterclayton
Copy link
Contributor

API approved. One minor comment, you can do that in a follow up.

/approve

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 7, 2017
@k8s-github-robot k8s-github-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 Aug 7, 2017
@kow3ns
Copy link
Member

kow3ns commented Aug 7, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crimsonfaith91, kow3ns, smarterclayton

Associated issue: 46005

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

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

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

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 0967f95 into kubernetes:master Aug 8, 2017
@crimsonfaith91 crimsonfaith91 deleted the apps-v1beta2 branch August 8, 2017 00:24
@kow3ns kow3ns mentioned this pull request Oct 9, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kow3ns kow3ns kow3ns left review comments

@deads2k deads2k deads2k left review comments

@enisoc enisoc enisoc left review comments

@janetkuo janetkuo janetkuo requested changes

@smarterclayton smarterclayton smarterclayton left review comments

@liggitt liggitt liggitt left review comments

@DirectXMan12 DirectXMan12 DirectXMan12 left review comments

Assignees

@janetkuo janetkuo

@enisoc enisoc

@erictune erictune

@kow3ns kow3ns

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No scale subresource for StatefulSets