Skip to content

Commit 1fced79

Browse files
gavinkflamgavinkflam
committed
feat: deduplicate validation errors and increment metric
Signed-off-by: Gavin Lam <[email protected]>
1 parent a57091c commit 1fced79

File tree

6 files changed

+177
-6
lines changed

6 files changed

+177
-6
lines changed

‎staging/src/k8s.io/apiserver/pkg/registry/rest/update.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@ func BeforeUpdate(strategy RESTUpdateStrategy, ctx context.Context, obj, old run
153153

154154
errs = append(errs, strategy.ValidateUpdate(ctx, obj, old)...)
155155
if len(errs) > 0 {
156+
errs = DeduplicateValidationErrorsAndUpdateMetric(kind.GroupKind(), "UPDATE", errs)
156157
return errors.NewInvalid(kind.GroupKind(), objectMeta.GetName(), errs)
157158
}
158159

‎staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525

2626
"k8s.io/apimachinery/pkg/runtime"
2727
"k8s.io/apimachinery/pkg/runtime/schema"
28+
"k8s.io/apimachinery/pkg/util/sets"
2829
"k8s.io/apimachinery/pkg/util/validation/field"
2930
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
3031
validationmetrics "k8s.io/apiserver/pkg/validation"
@@ -320,3 +321,23 @@ func panicSafeValidateFunc(
320321
return validateUpdateFunc(ctx, scheme, obj, oldObj, o)
321322
}
322323
}
324+
325+
// DeduplicateValidationErrorsAndUpdateMetric removes duplicate validation errors
326+
// and increment duplicate validation error metric when duplicates are found.
327+
func DeduplicateValidationErrorsAndUpdateMetric(qualifiedKind schema.GroupKind, operation string, errs field.ErrorList) field.ErrorList {
328+
uniqueErrs := field.ErrorList{}
329+
errsSet := sets.New[string]()
330+
331+
for _, err := range errs {
332+
errStr := fmt.Sprintf("%+v", err)
333+
334+
if errsSet.Has(errStr) {
335+
// Increment the duplicate error metric counter
336+
validationmetrics.Metrics.IncDuplicateValidationErrorMetric(qualifiedKind.String(), operation)
337+
} else {
338+
uniqueErrs = append(uniqueErrs, err)
339+
errsSet = errsSet.Insert(errStr)
340+
}
341+
}
342+
return uniqueErrs
343+
}

‎staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ import (
3434
"k8s.io/apimachinery/pkg/runtime/schema"
3535
"k8s.io/apimachinery/pkg/util/validation/field"
3636
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
37+
"k8s.io/apiserver/pkg/validation"
38+
"k8s.io/component-base/metrics/legacyregistry"
39+
"k8s.io/component-base/metrics/testutil"
3740
"k8s.io/klog/v2"
3841
)
3942

@@ -678,6 +681,71 @@ func TestValidateUpdateDeclarativelyWithRecovery(t *testing.T) {
678681
})
679682
}
680683

684+
func TestDeduplicateValidationErrorsAndRecordDuplicates(t *testing.T) {
685+
testCases := []struct {
686+
name string
687+
operation string
688+
qualifiedKind schema.GroupKind
689+
errs field.ErrorList
690+
wantErrs field.ErrorList
691+
expectedMetric string
692+
}{
693+
{
694+
name: "deduplicate errors and increment metric",
695+
operation: "UPDATE",
696+
qualifiedKind: schema.GroupKind{Group: "apps", Kind: "ReplicaSet"},
697+
errs: field.ErrorList{
698+
field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("minimum"),
699+
field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("minimum"),
700+
field.Invalid(field.NewPath("spec").Child("selector"), &metav1.LabelSelector{MatchLabels: map[string]string{}, MatchExpressions: []metav1.LabelSelectorRequirement{}}, "empty selector is invalid for deployment"),
701+
field.Invalid(field.NewPath("spec").Child("selector"), &metav1.LabelSelector{MatchLabels: map[string]string{}, MatchExpressions: []metav1.LabelSelectorRequirement{}}, "empty selector is invalid for deployment"),
702+
},
703+
wantErrs: field.ErrorList{
704+
field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("minimum"),
705+
field.Invalid(field.NewPath("spec").Child("selector"), &metav1.LabelSelector{MatchLabels: map[string]string{}, MatchExpressions: []metav1.LabelSelectorRequirement{}}, "empty selector is invalid for deployment"),
706+
},
707+
expectedMetric: `
708+
# HELP apiserver_validation_duplicate_validation_error_total [BETA] Number of duplicate validation errors during validation.
709+
# TYPE apiserver_validation_duplicate_validation_error_total counter
710+
apiserver_validation_duplicate_validation_error_total{kind="ReplicaSet.apps",operation="UPDATE"} 2
711+
`,
712+
},
713+
{
714+
name: "no-op for errors with no duplicates",
715+
operation: "UPDATE",
716+
qualifiedKind: schema.GroupKind{Group: "apps", Kind: "ReplicaSet"},
717+
errs: field.ErrorList{
718+
field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("minimum"),
719+
},
720+
wantErrs: field.ErrorList{
721+
field.Invalid(field.NewPath("spec").Child("replicas"), -1, "must be greater than or equal to 0").WithOrigin("minimum"),
722+
},
723+
expectedMetric: `
724+
# HELP apiserver_validation_duplicate_validation_error_total [BETA] Number of duplicate validation errors during validation.
725+
# TYPE apiserver_validation_duplicate_validation_error_total counter
726+
`,
727+
},
728+
}
729+
730+
for _, tc := range testCases {
731+
t.Run(tc.name, func(t *testing.T) {
732+
defer legacyregistry.Reset()
733+
defer validation.ResetValidationMetricsInstance()
734+
735+
gotErrs := DeduplicateValidationErrorsAndUpdateMetric(tc.qualifiedKind, tc.operation, tc.errs)
736+
gotErrsStr := fmt.Sprintf("%+v", gotErrs)
737+
wantErrsStr := fmt.Sprintf("%+v", tc.wantErrs)
738+
if gotErrsStr != wantErrsStr {
739+
t.Errorf("DeduplicateValidationErrorsAndUpdateMetric() gotErrs = %s, want %s", gotErrsStr, wantErrsStr)
740+
}
741+
742+
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(tc.expectedMetric), "apiserver_validation_duplicate_validation_error_total"); err != nil {
743+
t.Fatal(err)
744+
}
745+
})
746+
}
747+
}
748+
681749
func equalErrorLists(a, b field.ErrorList) bool {
682750
// If both are nil, consider them equal
683751
if a == nil && b == nil {

‎staging/src/k8s.io/apiserver/pkg/validation/metrics.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ const (
3030
type ValidationMetrics interface {
3131
IncDeclarativeValidationMismatchMetric()
3232
IncDeclarativeValidationPanicMetric()
33+
IncDuplicateValidationErrorMetric(labelValues ...string)
3334
Reset()
3435
}
3536

@@ -52,6 +53,15 @@ var validationMetricsInstance = &validationMetrics{
5253
StabilityLevel: metrics.BETA,
5354
},
5455
),
56+
DuplicateValidationErrorCounter: metrics.NewCounterVec(
57+
&metrics.CounterOpts{
58+
Namespace: namespace,
59+
Subsystem: subsystem,
60+
Name: "duplicate_validation_error_total",
61+
Help: "Number of duplicate validation errors during validation.",
62+
StabilityLevel: metrics.BETA,
63+
},
64+
[]string{"kind", "operation"}),
5565
}
5666

5767
// Metrics provides access to validation metrics.
@@ -60,17 +70,20 @@ var Metrics ValidationMetrics = validationMetricsInstance
6070
func init() {
6171
legacyregistry.MustRegister(validationMetricsInstance.DeclarativeValidationMismatchCounter)
6272
legacyregistry.MustRegister(validationMetricsInstance.DeclarativeValidationPanicCounter)
73+
legacyregistry.MustRegister(validationMetricsInstance.DuplicateValidationErrorCounter)
6374
}
6475

6576
type validationMetrics struct {
6677
DeclarativeValidationMismatchCounter *metrics.Counter
6778
DeclarativeValidationPanicCounter *metrics.Counter
79+
DuplicateValidationErrorCounter *metrics.CounterVec
6880
}
6981

7082
// Reset resets the validation metrics.
7183
func (m *validationMetrics) Reset() {
7284
m.DeclarativeValidationMismatchCounter.Reset()
7385
m.DeclarativeValidationPanicCounter.Reset()
86+
m.DuplicateValidationErrorCounter.Reset()
7487
}
7588

7689
// IncDeclarativeValidationMismatchMetric increments the counter for the declarative_validation_mismatch_total metric.
@@ -83,6 +96,11 @@ func (m *validationMetrics) IncDeclarativeValidationPanicMetric() {
8396
m.DeclarativeValidationPanicCounter.Inc()
8497
}
8598

99+
// IncDuplicateValidationErrorMetric increments the counter for the duplicate_validation_error_total metric.
100+
func (m *validationMetrics) IncDuplicateValidationErrorMetric(labelValues ...string) {
101+
m.DuplicateValidationErrorCounter.WithLabelValues(labelValues...).Inc()
102+
}
103+
86104
func ResetValidationMetricsInstance() {
87105
validationMetricsInstance.Reset()
88106
}

‎staging/src/k8s.io/apiserver/pkg/validation/metrics_test.go

Lines changed: 60 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func TestDeclarativeValidationMismatchMetric(t *testing.T) {
3838
apiserver_validation_declarative_validation_mismatch_total 1
3939
`
4040

41-
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total"); err != nil {
41+
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_declarative_validation_mismatch_total"); err != nil {
4242
t.Fatal(err)
4343
}
4444
}
@@ -57,7 +57,26 @@ func TestDeclarativeValidationPanicMetric(t *testing.T) {
5757
apiserver_validation_declarative_validation_panic_total 1
5858
`
5959

60-
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_panic_total"); err != nil {
60+
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_declarative_validation_panic_total"); err != nil {
61+
t.Fatal(err)
62+
}
63+
}
64+
65+
// TestDuplicateValidationErrorMetric tests that the duplicate error metric correctly increments once
66+
func TestDuplicateValidationErrorMetric(t *testing.T) {
67+
defer legacyregistry.Reset()
68+
defer ResetValidationMetricsInstance()
69+
70+
// Increment the metric once
71+
Metrics.IncDuplicateValidationErrorMetric("ReplicaSet.apps", "UPDATE")
72+
73+
expected := `
74+
# HELP apiserver_validation_duplicate_validation_error_total [BETA] Number of duplicate validation errors during validation.
75+
# TYPE apiserver_validation_duplicate_validation_error_total counter
76+
apiserver_validation_duplicate_validation_error_total{kind="ReplicaSet.apps",operation="UPDATE"} 1
77+
`
78+
79+
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_duplicate_validation_error_total"); err != nil {
6180
t.Fatal(err)
6281
}
6382
}
@@ -78,7 +97,7 @@ func TestDeclarativeValidationMismatchMetricMultiple(t *testing.T) {
7897
apiserver_validation_declarative_validation_mismatch_total 3
7998
`
8099

81-
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total"); err != nil {
100+
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_declarative_validation_mismatch_total"); err != nil {
82101
t.Fatal(err)
83102
}
84103
}
@@ -99,7 +118,29 @@ func TestDeclarativeValidationPanicMetricMultiple(t *testing.T) {
99118
apiserver_validation_declarative_validation_panic_total 3
100119
`
101120

102-
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_panic_total"); err != nil {
121+
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_declarative_validation_panic_total"); err != nil {
122+
t.Fatal(err)
123+
}
124+
}
125+
126+
// TestDuplicateValidationErrorMetricMultiple tests that the duplicate error metric correctly increments multiple times
127+
func TestDuplicateValidationErrorMetricMultiple(t *testing.T) {
128+
defer legacyregistry.Reset()
129+
defer ResetValidationMetricsInstance()
130+
131+
// Increment the metric three times
132+
Metrics.IncDuplicateValidationErrorMetric("ReplicaSet.apps", "UPDATE")
133+
Metrics.IncDuplicateValidationErrorMetric("ReplicaSet.apps", "UPDATE")
134+
Metrics.IncDuplicateValidationErrorMetric("Ingress.networking.k8s.io", "CREATE")
135+
136+
expected := `
137+
# HELP apiserver_validation_duplicate_validation_error_total [BETA] Number of duplicate validation errors during validation.
138+
# TYPE apiserver_validation_duplicate_validation_error_total counter
139+
apiserver_validation_duplicate_validation_error_total{kind="ReplicaSet.apps",operation="UPDATE"} 2
140+
apiserver_validation_duplicate_validation_error_total{kind="Ingress.networking.k8s.io",operation="CREATE"} 1
141+
`
142+
143+
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "apiserver_validation_duplicate_validation_error_total"); err != nil {
103144
t.Fatal(err)
104145
}
105146
}
@@ -112,6 +153,7 @@ func TestDeclarativeValidationMetricsReset(t *testing.T) {
112153
// Increment both metrics
113154
Metrics.IncDeclarativeValidationMismatchMetric()
114155
Metrics.IncDeclarativeValidationPanicMetric()
156+
Metrics.IncDuplicateValidationErrorMetric("ReplicaSet.apps", "UPDATE")
115157

116158
// Reset the metrics
117159
Metrics.Reset()
@@ -124,15 +166,21 @@ func TestDeclarativeValidationMetricsReset(t *testing.T) {
124166
# HELP apiserver_validation_declarative_validation_panic_total [BETA] Number of times declarative validation has panicked during validation.
125167
# TYPE apiserver_validation_declarative_validation_panic_total counter
126168
apiserver_validation_declarative_validation_panic_total 0
169+
# HELP apiserver_validation_duplicate_validation_error_total [BETA] Number of duplicate validation errors during validation.
170+
# TYPE apiserver_validation_duplicate_validation_error_total counter
127171
`
128172

129-
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total", "declarative_validation_panic_total"); err != nil {
173+
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected),
174+
"apiserver_validation_declarative_validation_mismatch_total",
175+
"apiserver_validation_declarative_validation_panic_total",
176+
"apiserver_validation_duplicate_validation_error_total"); err != nil {
130177
t.Fatal(err)
131178
}
132179

133180
// Increment the metrics again to ensure they're still functional
134181
Metrics.IncDeclarativeValidationMismatchMetric()
135182
Metrics.IncDeclarativeValidationPanicMetric()
183+
Metrics.IncDuplicateValidationErrorMetric("ReplicaSet.apps", "UPDATE")
136184

137185
// Verify they've been incremented correctly
138186
expected = `
@@ -142,9 +190,15 @@ func TestDeclarativeValidationMetricsReset(t *testing.T) {
142190
# HELP apiserver_validation_declarative_validation_panic_total [BETA] Number of times declarative validation has panicked during validation.
143191
# TYPE apiserver_validation_declarative_validation_panic_total counter
144192
apiserver_validation_declarative_validation_panic_total 1
193+
# HELP apiserver_validation_duplicate_validation_error_total [BETA] Number of duplicate validation errors during validation.
194+
# TYPE apiserver_validation_duplicate_validation_error_total counter
195+
apiserver_validation_duplicate_validation_error_total{kind="ReplicaSet.apps",operation="UPDATE"} 1
145196
`
146197

147-
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected), "declarative_validation_mismatch_total", "declarative_validation_panic_total"); err != nil {
198+
if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(expected),
199+
"apiserver_validation_declarative_validation_mismatch_total",
200+
"apiserver_validation_declarative_validation_panic_total",
201+
"apiserver_validation_duplicate_validation_error_total"); err != nil {
148202
t.Fatal(err)
149203
}
150204
}

‎test/instrumentation/testdata/stable-metrics-list.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,15 @@
175175
help: Number of times declarative validation has panicked during validation.
176176
type: Counter
177177
stabilityLevel: BETA
178+
- name: duplicate_validation_error_total
179+
subsystem: validation
180+
namespace: apiserver
181+
help: Number of duplicate validation errors during validation.
182+
type: Counter
183+
stabilityLevel: BETA
184+
labels:
185+
- kind
186+
- operation
178187
- name: disabled_metrics_total
179188
help: The count of disabled metrics.
180189
type: Counter

0 commit comments

Comments
 (0)