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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/conformance/testdata/conformance.yaml
View file
Open in desktop
Expand Up @@ -1315,7 +1315,7 @@
description: Create a StatefulSet resource. Newly created StatefulSet resource MUST
have a scale of one. Bring the scale of the StatefulSet resource up to two. StatefulSet
scale MUST be at two replicas.
release: v1.16
release: v1.16, v1.21
file: test/e2e/apps/statefulset.go
- testname: StatefulSet, Rolling Update with Partition
codename: '[sig-apps] StatefulSet [k8s.io] Basic StatefulSet functionality [StatefulSetBasic]
Expand Down
22 changes: 21 additions & 1 deletion test/e2e/apps/statefulset.go
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@ package apps

import (
"context"
"encoding/json"
"fmt"
"strings"
"sync"
Expand All @@ -27,6 +28,7 @@ import (
"github.com/onsi/gomega"

appsv1 "k8s.io/api/apps/v1"
autoscalingv1 "k8s.io/api/autoscaling/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
Expand Down Expand Up @@ -830,7 +832,7 @@ var _ = SIGDescribe("StatefulSet", func() {
})

/*
Release: v1.16
Release: v1.16, v1.21
Testname: StatefulSet resource Replica scaling
Description: Create a StatefulSet resource.
Newly created StatefulSet resource MUST have a scale of one.
Expand Down Expand Up @@ -868,6 +870,24 @@ var _ = SIGDescribe("StatefulSet", func() {
framework.Failf("Failed to get statefulset resource: %v", err)
}
framework.ExpectEqual(*(ss.Spec.Replicas), int32(2))

ginkgo.By("Patch a scale subresource")
scale.ResourceVersion = "" // indicate the scale update should be unconditional
scale.Spec.Replicas = 4 // should be 2 after "UpdateScale" operation, now Patch to 4
ssScalePatchPayload, err := json.Marshal(autoscalingv1.Scale{
Spec: autoscalingv1.ScaleSpec{
Replicas: scale.Spec.Replicas,
},
})
framework.ExpectNoError(err, "Could not Marshal JSON for patch payload")

_, err = c.AppsV1().StatefulSets(ns).Patch(context.TODO(), ssName, types.StrategicMergePatchType, []byte(ssScalePatchPayload), metav1.PatchOptions{}, "scale")
framework.ExpectNoError(err, "Failed to patch stateful set: %v", err)

ginkgo.By("verifying the statefulset Spec.Replicas was modified")
ss, err = c.AppsV1().StatefulSets(ns).Get(context.TODO(), ssName, metav1.GetOptions{})
framework.ExpectNoError(err, "Failed to get statefulset resource: %v", err)
framework.ExpectEqual(*(ss.Spec.Replicas), int32(4), "statefulset should have 4 replicas")
Copy link

Choose a reason for hiding this comment

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

should we similar to the original status, also add ?
e2estatefulset.WaitForRunningAndReady(c, *ss.Spec.Replicas, ss)
waitForStatus(c, ss)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is included on line 847-848. The pattern looks to be using this only at the "create" stage of the statefulset.
Does this address your concern?

Copy link

Choose a reason for hiding this comment

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

agree its not the pattern, but extra validation makes the test more robust, that indeed after patching the scale subresource, the StatefulSet pod is ready and good to go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for helping to make this test more solid.
I believe WaitForRunningAndReady is for creating new resources.
However, I think adding WaitForStatusReplicas will add the value you would like to see.
I update with WaitForStatusReplicas and I trust this will make the test ready to be merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree its not the pattern, but extra validation makes the test more robust, that indeed after patching the scale subresource, the StatefulSet pod is ready and good to go

@krmayankk iirc there are other tests that ensure the scale is working as expected. The purpose of this test is to just ensure the update & patch API calls are working as expected. That additional scrutiny imo is unnecessary b/c we don't rely on the result at all and that wail will only make this test last longer. So I suggested dropping that additional wait.

Copy link
Member

Choose a reason for hiding this comment

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

/hold
where's the e2e/conformance test that verifies stateful sets scale as expected? as an end user I'd be surprised if the API allowed me to update the spec, but the status never reconciled

})
})

Expand Down