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

Update: StatefulSet Replica scaling to include Patch Scale +1 endpoint #98126

Merged
merged 1 commit into from Feb 12, 2021
Merged

Update: StatefulSet Replica scaling to include Patch Scale +1 endpoint #98126

merged 1 commit into from Feb 12, 2021

Conversation

riaankleinhans
Copy link
Contributor

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR adds a test to test the following untested endpoints:

  • patchAppsV1NamespacedStatefulSetScale

Which issue(s) this PR fixes:
Fixes #98125

Testgrid Link:

Special notes for your reviewer:
Adds +1 endpoint test coverage (good for conformance)

Does this PR introduce a user-facing change?:

NONE

Release note:

NONE

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

NONE

/sig testing
/sig architecture
/area conformance

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. area/conformance Issues or PRs related to kubernetes conformance tests needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 17, 2021
@riaankleinhans riaankleinhans mentioned this pull request Jan 17, 2021
Closed
6 tasks
@k8s-ci-robot k8s-ci-robot added area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Jan 17, 2021
@riaankleinhans
Copy link
Contributor Author

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 17, 2021
@riaankleinhans
Copy link
Contributor Author

/test all

@riaankleinhans
Copy link
Contributor Author

riaankleinhans commented Jan 18, 2021 •
edited

Test: TestAdmit/skip_webhook_whose_objectSelector_does_not_match_CRD's_labels Messages: skip webhook whose objectSelector does not match CRD's labels: annotations not set as expected. W0117 23:57:43.395337 127900 dispatcher.go:170] Failed calling webhook, failing open cache2: failed calling webhook "cache2": an error on the server ("webhook internal server error") has prevented the request from succeeding E0117 23:57:43.395507 127900 dispatcher.go:171] failed calling webhook "cache2": an error on the server ("webhook internal server error") has prevented the request from succeeding FAIL - pull-kubernetes-bazel-test

/test all

@riaankleinhans
Copy link
Contributor Author

/test all

4 similar comments
@riaankleinhans
Copy link
Contributor Author

/test all

@riaankleinhans
Copy link
Contributor Author

/test all

@riaankleinhans
Copy link
Contributor Author

/test all

@riaankleinhans
Copy link
Contributor Author

/test all

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-conformance-image-test

@riaankleinhans
Copy link
Contributor Author

/assign @smarterclayton
Updated the test to add patchAppsV1NamespacedStatefulSetScale endpoint to conformance.
Ran /test all several times and the update seems good to go.
The only issue was related to my incorrect updating of the conformance.yaml now fix by @heyste
The test metadata is updated as required by the rules noting changes to conformance test in subsequent releases and now include 1.21

krmayankk
krmayankk reviewed Jan 19, 2021
View reviewed changes
test/e2e/apps/statefulset.go Outdated
@@ -45,6 +42,9 @@ import (
e2eservice "k8s.io/kubernetes/test/e2e/framework/service"
e2estatefulset "k8s.io/kubernetes/test/e2e/framework/statefulset"
imageutils "k8s.io/kubernetes/test/utils/image"
"strings"
"sync"
"time"
Copy link

Choose a reason for hiding this comment

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

can you keep the separation, go fmt should do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes as requested. Thank you for the review.

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind
Unrelated flake

[Fail] [sig-cli] Kubectl client Simple pod [It] should return command exit codes 
test/e2e/kubectl/kubectl.go:515

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-node-e2e

Unrelated flake

[Fail] [k8s.io] Summary API [NodeConformance] when querying /stats/summary [It] should report resource usage through the stats api 
I0211 17:04:00.746] _output/local/go/src/k8s.io/kubernetes/test/e2e_node/summary_test.go:344

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-verify

2 similar comments
@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-verify

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-verify

@dims
Copy link
Member

dims commented Feb 11, 2021

looks like you need to run hack/update-gofmt.sh @Riaankl

@riaankleinhans riaankleinhans mentioned this pull request Feb 11, 2021
Merged
@dims
Copy link
Member

dims commented Feb 11, 2021

/lgtm
/approve

/assign @smarterclayton @spiffxp

Aaron, Clayton, this is ready to merge

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 11, 2021
@heyste
Update e2e patch scale statefulset test
866f658 
Test now validates patchAppsV1NamespacedStatefulSetScale endpoint.
Update conformance metadata to include v1.21

Co-authored-by: Stephen Heywood <stephen@ii.coop>
@riaankleinhans
Copy link
Contributor Author

Sorry for the confusion. pull-kubernetes-verify failed on a gofmt error.
Fixed and pushed

dims
dims approved these changes Feb 11, 2021
View reviewed changes
Copy link
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2021
soltysh
soltysh approved these changes Feb 12, 2021
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@riaankleinhans
Copy link
Contributor Author

/assign @smarterclayton

@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, johnbelamaric, Riaankl, soltysh

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 12, 2021
@spiffxp
Copy link
Member

spiffxp commented Feb 12, 2021

I put on a /hold because I'd like to verify whether other conformance tests that update the scale subresource wait for status to reconcile. I'm not sure "read after write works" is sufficient to say stateful sets behave as expected

@spiffxp
Copy link
Member

spiffxp commented Feb 12, 2021

/hold cancel
There are two statefulset conformance tests that exercise scaling by updating spec.Replicas directly

  • Scaling should happen in predictable order and halt if any stateful pod is unhealthy [Slow]
  • Burst scaling should run to completion even with unhealthy pods [Slow]

So agree with @soltysh, it's fine if this verifies the behavior that spec.Replicas gets updated, since those tests cover the behavior of scaling actually happening.

@k8s-ci-robot 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 12, 2021
@k8s-ci-robot k8s-ci-robot merged commit 5283903 into kubernetes:master Feb 12, 2021
conformance-definition automation moved this from In Progress PRs to Done Feb 12, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spiffxp spiffxp spiffxp left review comments

@krmayankk krmayankk krmayankk left review comments

@dims dims dims approved these changes

@soltysh soltysh soltysh approved these changes

@foxish foxish Awaiting requested review from foxish

Assignees

@dims dims

@spiffxp spiffxp

@mattfarina mattfarina

@soltysh soltysh

@smarterclayton smarterclayton

@krmayankk krmayankk

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance Issues or PRs related to kubernetes conformance tests area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
conformance-definition
  
Done
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

Write Updated test for StatefulSet Replica scaling to include Patch Scale +1 endpoint
10 participants
@riaankleinhans @heyste @krmayankk @dims @johnbelamaric @k8s-ci-robot @spiffxp @soltysh @mattfarina @smarterclayton