Skip to content

Commit 70dea6e

Browse files
yue9944882yue9944882
committed
review: several fixes and addressing comments
1 parent fe8ad90 commit 70dea6e

File tree

5 files changed

+205
-47
lines changed

5 files changed

+205
-47
lines changed

‎pkg/apis/flowcontrol/validation/validation_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -554,7 +554,7 @@ func TestFlowSchemaValidation(t *testing.T) {
554554
Name: "system-foo",
555555
},
556556
Spec: flowcontrol.FlowSchemaSpec{
557-
MatchingPrecedence: 50000,
557+
MatchingPrecedence: 10001,
558558
PriorityLevelConfiguration: flowcontrol.PriorityLevelConfigurationReference{
559559
Name: "system-bar",
560560
},
@@ -579,7 +579,7 @@ func TestFlowSchemaValidation(t *testing.T) {
579579
},
580580
},
581581
expectedErrors: field.ErrorList{
582-
field.Invalid(field.NewPath("spec").Child("matchingPrecedence"), int32(50000), "must not be greater than 10000"),
582+
field.Invalid(field.NewPath("spec").Child("matchingPrecedence"), int32(10001), "must not be greater than 10000"),
583583
},
584584
},
585585
}

‎pkg/registry/flowcontrol/rest/BUILD‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,11 @@ go_library(
88
deps = [
99
"//pkg/api/legacyscheme:go_default_library",
1010
"//pkg/apis/flowcontrol:go_default_library",
11+
"//pkg/apis/flowcontrol/v1alpha1:go_default_library",
1112
"//pkg/registry/flowcontrol/flowschema/storage:go_default_library",
1213
"//pkg/registry/flowcontrol/prioritylevelconfiguration/storage:go_default_library",
1314
"//staging/src/k8s.io/api/flowcontrol/v1alpha1:go_default_library",
15+
"//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
1416
"//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
1517
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
1618
"//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",
@@ -43,9 +45,11 @@ go_test(
4345
srcs = ["storage_flowcontrol_test.go"],
4446
embed = [":go_default_library"],
4547
deps = [
48+
"//pkg/apis/flowcontrol/v1alpha1:go_default_library",
4649
"//staging/src/k8s.io/api/flowcontrol/v1alpha1:go_default_library",
4750
"//staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap:go_default_library",
4851
"//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",
4952
"//vendor/github.com/stretchr/testify/assert:go_default_library",
53+
"//vendor/github.com/stretchr/testify/require:go_default_library",
5054
],
5155
)

‎pkg/registry/flowcontrol/rest/storage_flowcontrol.go‎

Lines changed: 71 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"time"
2222

2323
flowcontrolv1alpha1 "k8s.io/api/flowcontrol/v1alpha1"
24+
"k8s.io/apimachinery/pkg/api/equality"
2425
apierrors "k8s.io/apimachinery/pkg/api/errors"
2526
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2627
"k8s.io/apimachinery/pkg/util/wait"
@@ -33,6 +34,7 @@ import (
3334
"k8s.io/klog"
3435
"k8s.io/kubernetes/pkg/api/legacyscheme"
3536
"k8s.io/kubernetes/pkg/apis/flowcontrol"
37+
flowcontrolapisv1alpha1 "k8s.io/kubernetes/pkg/apis/flowcontrol/v1alpha1"
3638
flowschemastore "k8s.io/kubernetes/pkg/registry/flowcontrol/flowschema/storage"
3739
prioritylevelconfigurationstore "k8s.io/kubernetes/pkg/registry/flowcontrol/prioritylevelconfiguration/storage"
3840
)
@@ -94,20 +96,21 @@ func (p RESTStorageProvider) PostStartHook() (string, genericapiserver.PostStart
9496
_ = wait.PollImmediateUntil(
9597
retryCreatingSuggestedSettingsInterval,
9698
func() (bool, error) {
97-
shouldEnsureSuggested, err := shouldEnsureAllPredefined(flowcontrolClientSet)
99+
shouldEnsureSuggested, err := lastMandatoryExists(flowcontrolClientSet)
98100
if err != nil {
99101
klog.Errorf("failed getting exempt flow-schema, will retry later: %v", err)
100102
return false, nil
101103
}
102-
if shouldEnsureSuggested {
103-
err := ensure(
104-
flowcontrolClientSet,
105-
flowcontrolbootstrap.SuggestedFlowSchemas,
106-
flowcontrolbootstrap.SuggestedPriorityLevelConfigurations)
107-
if err != nil {
108-
klog.Errorf("failed ensuring suggested settings, will retry later: %v", err)
109-
return false, nil
110-
}
104+
if !shouldEnsureSuggested {
105+
return true, nil
106+
}
107+
err = ensure(
108+
flowcontrolClientSet,
109+
flowcontrolbootstrap.SuggestedFlowSchemas,
110+
flowcontrolbootstrap.SuggestedPriorityLevelConfigurations)
111+
if err != nil {
112+
klog.Errorf("failed ensuring suggested settings, will retry later: %v", err)
113+
return false, nil
111114
}
112115
return true, nil
113116
},
@@ -124,7 +127,7 @@ func (p RESTStorageProvider) PostStartHook() (string, genericapiserver.PostStart
124127
// the full initial set of objects from being created.
125128
flowcontrolbootstrap.MandatoryPriorityLevelConfigurations,
126129
); err != nil {
127-
klog.Errorf("failed creating default flowcontrol settings: %v", err)
130+
klog.Errorf("failed creating mandatory flowcontrol settings: %v", err)
128131
return false, nil
129132
}
130133
return false, nil // always retry
@@ -138,7 +141,7 @@ func (p RESTStorageProvider) PostStartHook() (string, genericapiserver.PostStart
138141

139142
// Returns false if there's a "exempt" priority-level existing in the cluster, otherwise returns a true
140143
// if the "exempt" priority-level is not found.
141-
func shouldEnsureAllPredefined(flowcontrolClientSet flowcontrolclient.FlowcontrolV1alpha1Interface) (bool, error) {
144+
func lastMandatoryExists(flowcontrolClientSet flowcontrolclient.FlowcontrolV1alpha1Interface) (bool, error) {
142145
if _, err := flowcontrolClientSet.PriorityLevelConfigurations().Get(flowcontrol.PriorityLevelConfigurationNameExempt, metav1.GetOptions{}); err != nil {
143146
if apierrors.IsNotFound(err) {
144147
return true, nil
@@ -175,39 +178,75 @@ func ensure(flowcontrolClientSet flowcontrolclient.FlowcontrolV1alpha1Interface,
175178
}
176179

177180
func upgrade(flowcontrolClientSet flowcontrolclient.FlowcontrolV1alpha1Interface, flowSchemas []*flowcontrolv1alpha1.FlowSchema, priorityLevels []*flowcontrolv1alpha1.PriorityLevelConfiguration) error {
178-
for _, flowSchema := range flowSchemas {
179-
_, err := flowcontrolClientSet.FlowSchemas().Get(flowSchema.Name, metav1.GetOptions{})
180-
if err != nil {
181-
return fmt.Errorf("failed getting FlowSchema %s due to %v, will retry later", flowSchema.Name, err)
181+
for _, expectedFlowSchema := range flowSchemas {
182+
actualFlowSchema, err := flowcontrolClientSet.FlowSchemas().Get(expectedFlowSchema.Name, metav1.GetOptions{})
183+
if err == nil {
184+
// TODO(yue9944882): extract existing version from label and compare
185+
// TODO(yue9944882): create w/ version string attached
186+
identical, err := flowSchemaHasWrongSpec(expectedFlowSchema, actualFlowSchema)
187+
if err != nil {
188+
return fmt.Errorf("failed checking if mandatory FlowSchema %s is up-to-date due to %v, will retry later", expectedFlowSchema.Name, err)
189+
}
190+
if !identical {
191+
if _, err := flowcontrolClientSet.FlowSchemas().Update(expectedFlowSchema); err != nil {
192+
return fmt.Errorf("failed upgrading mandatory FlowSchema %s due to %v, will retry later", expectedFlowSchema.Name, err)
193+
}
194+
}
195+
continue
196+
}
197+
if !apierrors.IsNotFound(err) {
198+
return fmt.Errorf("failed getting FlowSchema %s due to %v, will retry later", expectedFlowSchema.Name, err)
182199
}
183-
// TODO(yue9944882): extract existing version from label and compare
184-
// TODO(yue9944882): create w/ version string attached
185-
_, err = flowcontrolClientSet.FlowSchemas().Create(flowSchema)
200+
_, err = flowcontrolClientSet.FlowSchemas().Create(expectedFlowSchema)
186201
if apierrors.IsAlreadyExists(err) {
187-
klog.V(3).Infof("system preset FlowSchema %s already exists, skipping creating", flowSchema.Name)
202+
klog.V(3).Infof("system preset FlowSchema %s already exists, skipping creating", expectedFlowSchema.Name)
188203
continue
189204
}
190205
if err != nil {
191-
return fmt.Errorf("cannot create FlowSchema %s due to %v", flowSchema.Name, err)
206+
return fmt.Errorf("cannot create FlowSchema %s due to %v", expectedFlowSchema.Name, err)
192207
}
193-
klog.V(3).Infof("created system preset FlowSchema %s", flowSchema.Name)
208+
klog.V(3).Infof("created system preset FlowSchema %s", expectedFlowSchema.Name)
194209
}
195-
for _, priorityLevelConfiguration := range priorityLevels {
196-
_, err := flowcontrolClientSet.FlowSchemas().Get(priorityLevelConfiguration.Name, metav1.GetOptions{})
197-
if err != nil {
198-
return fmt.Errorf("failed getting PriorityLevelConfiguration %s due to %v, will retry later", priorityLevelConfiguration.Name, err)
210+
for _, expectedPriorityLevelConfiguration := range priorityLevels {
211+
actualPriorityLevelConfiguration, err := flowcontrolClientSet.PriorityLevelConfigurations().Get(expectedPriorityLevelConfiguration.Name, metav1.GetOptions{})
212+
if err == nil {
213+
// TODO(yue9944882): extract existing version from label and compare
214+
// TODO(yue9944882): create w/ version string attached
215+
identical, err := priorityLevelHasWrongSpec(expectedPriorityLevelConfiguration, actualPriorityLevelConfiguration)
216+
if err != nil {
217+
return fmt.Errorf("failed checking if mandatory PriorityLevelConfiguration %s is up-to-date due to %v, will retry later", expectedPriorityLevelConfiguration.Name, err)
218+
}
219+
if !identical {
220+
if _, err := flowcontrolClientSet.PriorityLevelConfigurations().Update(expectedPriorityLevelConfiguration); err != nil {
221+
return fmt.Errorf("failed upgrading mandatory PriorityLevelConfiguration %s due to %v, will retry later", expectedPriorityLevelConfiguration.Name, err)
222+
}
223+
}
224+
continue
199225
}
200-
// TODO(yue9944882): extract existing version from label and compare
201-
// TODO(yue9944882): create w/ version string attached
202-
_, err = flowcontrolClientSet.PriorityLevelConfigurations().Create(priorityLevelConfiguration)
226+
if !apierrors.IsNotFound(err) {
227+
return fmt.Errorf("failed getting PriorityLevelConfiguration %s due to %v, will retry later", expectedPriorityLevelConfiguration.Name, err)
228+
}
229+
_, err = flowcontrolClientSet.PriorityLevelConfigurations().Create(expectedPriorityLevelConfiguration)
203230
if apierrors.IsAlreadyExists(err) {
204-
klog.V(3).Infof("system preset PriorityLevelConfiguration %s already exists, skipping creating", priorityLevelConfiguration.Name)
231+
klog.V(3).Infof("system preset PriorityLevelConfiguration %s already exists, skipping creating", expectedPriorityLevelConfiguration.Name)
205232
continue
206233
}
207234
if err != nil {
208-
return fmt.Errorf("cannot create PriorityLevelConfiguration %s due to %v", priorityLevelConfiguration.Name, err)
235+
return fmt.Errorf("cannot create PriorityLevelConfiguration %s due to %v", expectedPriorityLevelConfiguration.Name, err)
209236
}
210-
klog.V(3).Infof("created system preset PriorityLevelConfiguration %s", priorityLevelConfiguration.Name)
237+
klog.V(3).Infof("created system preset PriorityLevelConfiguration %s", expectedPriorityLevelConfiguration.Name)
211238
}
212239
return nil
213240
}
241+
242+
func flowSchemaHasWrongSpec(expected, actual *flowcontrolv1alpha1.FlowSchema) (bool, error) {
243+
copiedExpectedFlowSchema := expected.DeepCopy()
244+
flowcontrolapisv1alpha1.SetObjectDefaults_FlowSchema(copiedExpectedFlowSchema)
245+
return !equality.Semantic.DeepEqual(copiedExpectedFlowSchema.Spec, actual.Spec), nil
246+
}
247+
248+
func priorityLevelHasWrongSpec(expected, actual *flowcontrolv1alpha1.PriorityLevelConfiguration) (bool, error) {
249+
copiedExpectedPriorityLevel := expected.DeepCopy()
250+
flowcontrolapisv1alpha1.SetObjectDefaults_PriorityLevelConfiguration(copiedExpectedPriorityLevel)
251+
return !equality.Semantic.DeepEqual(copiedExpectedPriorityLevel.Spec, actual.Spec), nil
252+
}

‎pkg/registry/flowcontrol/rest/storage_flowcontrol_test.go‎

Lines changed: 117 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,20 @@ limitations under the License.
1717
package rest
1818

1919
import (
20+
"github.com/stretchr/testify/require"
2021
"testing"
2122

2223
"github.com/stretchr/testify/assert"
23-
flowcontrol "k8s.io/api/flowcontrol/v1alpha1"
24+
flowcontrolv1alpha1 "k8s.io/api/flowcontrol/v1alpha1"
2425
"k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap"
2526
"k8s.io/client-go/kubernetes/fake"
27+
flowcontrolapisv1alpha1 "k8s.io/kubernetes/pkg/apis/flowcontrol/v1alpha1"
2628
)
2729

2830
func TestShouldEnsurePredefinedSettings(t *testing.T) {
2931
testCases := []struct {
3032
name string
31-
existingPriorityLevel *flowcontrol.PriorityLevelConfiguration
33+
existingPriorityLevel *flowcontrolv1alpha1.PriorityLevelConfiguration
3234
expected bool
3335
}{
3436
{
@@ -49,9 +51,121 @@ func TestShouldEnsurePredefinedSettings(t *testing.T) {
4951
if testCase.existingPriorityLevel != nil {
5052
c.FlowcontrolV1alpha1().PriorityLevelConfigurations().Create(testCase.existingPriorityLevel)
5153
}
52-
should, err := shouldEnsureAllPredefined(c.FlowcontrolV1alpha1())
54+
should, err := lastMandatoryExists(c.FlowcontrolV1alpha1())
5355
assert.NoError(t, err)
5456
assert.Equal(t, testCase.expected, should)
5557
})
5658
}
5759
}
60+
61+
func TestFlowSchemaHasWrongSpec(t *testing.T) {
62+
fs1 := &flowcontrolv1alpha1.FlowSchema{
63+
Spec: flowcontrolv1alpha1.FlowSchemaSpec{},
64+
}
65+
fs2 := &flowcontrolv1alpha1.FlowSchema{
66+
Spec: flowcontrolv1alpha1.FlowSchemaSpec{
67+
MatchingPrecedence: 1,
68+
},
69+
}
70+
fs1Defaulted := &flowcontrolv1alpha1.FlowSchema{
71+
Spec: flowcontrolv1alpha1.FlowSchemaSpec{
72+
MatchingPrecedence: flowcontrolapisv1alpha1.FlowSchemaDefaultMatchingPrecedence,
73+
},
74+
}
75+
testCases := []struct {
76+
name string
77+
expected *flowcontrolv1alpha1.FlowSchema
78+
actual *flowcontrolv1alpha1.FlowSchema
79+
hasWrongSpec bool
80+
}{
81+
{
82+
name: "identical flow-schemas should work",
83+
expected: bootstrap.MandatoryFlowSchemaCatchAll,
84+
actual: bootstrap.MandatoryFlowSchemaCatchAll,
85+
hasWrongSpec: false,
86+
},
87+
{
88+
name: "defaulted flow-schemas should work",
89+
expected: fs1,
90+
actual: fs1Defaulted,
91+
hasWrongSpec: false,
92+
},
93+
{
94+
name: "non-defaulted flow-schema has wrong spec",
95+
expected: fs1,
96+
actual: fs2,
97+
hasWrongSpec: true,
98+
},
99+
}
100+
for _, testCase := range testCases {
101+
t.Run(testCase.name, func(t *testing.T) {
102+
w, err := flowSchemaHasWrongSpec(testCase.expected, testCase.actual)
103+
require.NoError(t, err)
104+
assert.Equal(t, testCase.hasWrongSpec, w)
105+
})
106+
}
107+
}
108+
109+
func TestPriorityLevelHasWrongSpec(t *testing.T) {
110+
pl1 := &flowcontrolv1alpha1.PriorityLevelConfiguration{
111+
Spec: flowcontrolv1alpha1.PriorityLevelConfigurationSpec{
112+
Type: flowcontrolv1alpha1.PriorityLevelEnablementLimited,
113+
Limited: &flowcontrolv1alpha1.LimitedPriorityLevelConfiguration{
114+
LimitResponse: flowcontrolv1alpha1.LimitResponse{
115+
Type: flowcontrolv1alpha1.LimitResponseTypeReject,
116+
},
117+
},
118+
},
119+
}
120+
pl2 := &flowcontrolv1alpha1.PriorityLevelConfiguration{
121+
Spec: flowcontrolv1alpha1.PriorityLevelConfigurationSpec{
122+
Type: flowcontrolv1alpha1.PriorityLevelEnablementLimited,
123+
Limited: &flowcontrolv1alpha1.LimitedPriorityLevelConfiguration{
124+
AssuredConcurrencyShares: 1,
125+
},
126+
},
127+
}
128+
pl1Defaulted := &flowcontrolv1alpha1.PriorityLevelConfiguration{
129+
Spec: flowcontrolv1alpha1.PriorityLevelConfigurationSpec{
130+
Type: flowcontrolv1alpha1.PriorityLevelEnablementLimited,
131+
Limited: &flowcontrolv1alpha1.LimitedPriorityLevelConfiguration{
132+
AssuredConcurrencyShares: flowcontrolapisv1alpha1.PriorityLevelConfigurationDefaultAssuredConcurrencyShares,
133+
LimitResponse: flowcontrolv1alpha1.LimitResponse{
134+
Type: flowcontrolv1alpha1.LimitResponseTypeReject,
135+
},
136+
},
137+
},
138+
}
139+
testCases := []struct {
140+
name string
141+
expected *flowcontrolv1alpha1.PriorityLevelConfiguration
142+
actual *flowcontrolv1alpha1.PriorityLevelConfiguration
143+
hasWrongSpec bool
144+
}{
145+
{
146+
name: "identical priority-level should work",
147+
expected: bootstrap.MandatoryPriorityLevelConfigurationCatchAll,
148+
actual: bootstrap.MandatoryPriorityLevelConfigurationCatchAll,
149+
hasWrongSpec: false,
150+
},
151+
{
152+
name: "defaulted priority-level should work",
153+
expected: pl1,
154+
actual: pl1Defaulted,
155+
hasWrongSpec: false,
156+
},
157+
{
158+
name: "non-defaulted priority-level has wrong spec",
159+
expected: pl1,
160+
actual: pl2,
161+
hasWrongSpec: true,
162+
},
163+
}
164+
for _, testCase := range testCases {
165+
t.Run(testCase.name, func(t *testing.T) {
166+
w, err := priorityLevelHasWrongSpec(testCase.expected, testCase.actual)
167+
require.NoError(t, err)
168+
assert.Equal(t, testCase.hasWrongSpec, w)
169+
})
170+
}
171+
}

0 commit comments

Comments
 (0)