From 4ac0116a5d826d97cc839dca55e3351ef68f2db1 Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Wed, 6 Mar 2019 14:30:04 -0800 Subject: [PATCH 1/4] Track dry-run and apply in metrics --- .../pkg/apis/meta/v1/validation/validation.go | 11 ++--- .../meta/v1/validation/validation_test.go | 4 +- .../apiserver/pkg/endpoints/metrics/BUILD | 2 + .../pkg/endpoints/metrics/metrics.go | 43 ++++++++++++++++--- 4 files changed, 47 insertions(+), 13 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go index 02364d7fa31..4ce2d70d0b6 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go @@ -86,16 +86,16 @@ func ValidateDeleteOptions(options *metav1.DeleteOptions) field.ErrorList { *options.PropagationPolicy != metav1.DeletePropagationOrphan { allErrs = append(allErrs, field.NotSupported(field.NewPath("propagationPolicy"), options.PropagationPolicy, []string{string(metav1.DeletePropagationForeground), string(metav1.DeletePropagationBackground), string(metav1.DeletePropagationOrphan), "nil"})) } - allErrs = append(allErrs, validateDryRun(field.NewPath("dryRun"), options.DryRun)...) + allErrs = append(allErrs, ValidateDryRun(field.NewPath("dryRun"), options.DryRun)...) return allErrs } func ValidateCreateOptions(options *metav1.CreateOptions) field.ErrorList { - return validateDryRun(field.NewPath("dryRun"), options.DryRun) + return ValidateDryRun(field.NewPath("dryRun"), options.DryRun) } func ValidateUpdateOptions(options *metav1.UpdateOptions) field.ErrorList { - return validateDryRun(field.NewPath("dryRun"), options.DryRun) + return ValidateDryRun(field.NewPath("dryRun"), options.DryRun) } func ValidatePatchOptions(options *metav1.PatchOptions, patchType types.PatchType) field.ErrorList { @@ -103,13 +103,14 @@ func ValidatePatchOptions(options *metav1.PatchOptions, patchType types.PatchTyp if patchType != types.ApplyPatchType && options.Force != nil { allErrs = append(allErrs, field.Forbidden(field.NewPath("force"), "may not be specified for non-apply patch")) } - allErrs = append(allErrs, validateDryRun(field.NewPath("dryRun"), options.DryRun)...) + allErrs = append(allErrs, ValidateDryRun(field.NewPath("dryRun"), options.DryRun)...) return allErrs } var allowedDryRunValues = sets.NewString(metav1.DryRunAll) -func validateDryRun(fldPath *field.Path, dryRun []string) field.ErrorList { +// ValidateDryRun validates that a dryRun query param only contains allowed values. +func ValidateDryRun(fldPath *field.Path, dryRun []string) field.ErrorList { allErrs := field.ErrorList{} if !allowedDryRunValues.HasAll(dryRun...) { allErrs = append(allErrs, field.NotSupported(fldPath, dryRun, allowedDryRunValues.List())) diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go index a7046a54892..bef85109736 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go @@ -103,7 +103,7 @@ func TestValidDryRun(t *testing.T) { for _, test := range tests { t.Run(fmt.Sprintf("%v", test), func(t *testing.T) { - if errs := validateDryRun(field.NewPath("dryRun"), test); len(errs) != 0 { + if errs := ValidateDryRun(field.NewPath("dryRun"), test); len(errs) != 0 { t.Errorf("%v should be a valid dry-run value: %v", test, errs) } }) @@ -118,7 +118,7 @@ func TestInvalidDryRun(t *testing.T) { for _, test := range tests { t.Run(fmt.Sprintf("%v", test), func(t *testing.T) { - if len(validateDryRun(field.NewPath("dryRun"), test)) == 0 { + if len(ValidateDryRun(field.NewPath("dryRun"), test)) == 0 { t.Errorf("%v shouldn't be a valid dry-run value", test) } }) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD index ea44cdc05e8..ac80944cf2e 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD @@ -18,6 +18,8 @@ go_library( importmap = "k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/endpoints/metrics", importpath = "k8s.io/apiserver/pkg/endpoints/metrics", deps = [ + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//vendor/github.com/emicklei/go-restful:go_default_library", diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index 1c658d4c826..d307cd18551 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -21,11 +21,14 @@ import ( "net" "net/http" "regexp" + "sort" "strconv" "strings" "sync" "time" + "k8s.io/apimachinery/pkg/apis/meta/v1/validation" + "k8s.io/apimachinery/pkg/types" utilnet "k8s.io/apimachinery/pkg/util/net" "k8s.io/apiserver/pkg/endpoints/request" @@ -50,9 +53,13 @@ var ( requestCounter = prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "apiserver_request_total", - Help: "Counter of apiserver requests broken out for each verb, group, version, resource, scope, component, client, and HTTP response contentType and code.", + Help: "Counter of apiserver requests broken out for each verb, dry run value, group, version, resource, scope, component, client, and HTTP response contentType and code.", }, - []string{"verb", "group", "version", "resource", "subresource", "scope", "component", "client", "contentType", "code"}, + // The label_name contentType doesn't follow the label_name convention defined here: + // https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/instrumentation.md + // But changing it would break backwards compatibility. Future label_names + // should be all lowercase and separated by underscores. + []string{"verb", "dry_run", "group", "version", "resource", "subresource", "scope", "component", "client", "contentType", "code"}, ) deprecatedRequestCounter = prometheus.NewCounterVec( prometheus.CounterOpts{ @@ -71,14 +78,14 @@ var ( requestLatencies = prometheus.NewHistogramVec( prometheus.HistogramOpts{ Name: "apiserver_request_duration_seconds", - Help: "Response latency distribution in seconds for each verb, group, version, resource, subresource, scope and component.", + Help: "Response latency distribution in seconds for each verb, dry run value, group, version, resource, subresource, scope and component.", // This metric is used for verifying api call latencies SLO, // as well as tracking regressions in this aspects. // Thus we customize buckets significantly, to empower both usecases. Buckets: []float64{0.05, 0.1, 0.15, 0.2, 0.25, 0.3, 0.35, 0.4, 0.45, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0, 1.25, 1.5, 1.75, 2.0, 2.5, 3.0, 3.5, 4.0, 4.5, 5, 6, 7, 8, 9, 10, 15, 20, 25, 30, 40, 50, 60}, }, - []string{"verb", "group", "version", "resource", "subresource", "scope", "component"}, + []string{"verb", "dry_run", "group", "version", "resource", "subresource", "scope", "component"}, ) deprecatedRequestLatencies = prometheus.NewHistogramVec( prometheus.HistogramOpts{ @@ -225,12 +232,13 @@ func RecordLongRunning(req *http.Request, requestInfo *request.RequestInfo, comp // a request. verb must be uppercase to be backwards compatible with existing monitoring tooling. func MonitorRequest(req *http.Request, verb, group, version, resource, subresource, scope, component, contentType string, httpCode, respSize int, elapsed time.Duration) { reportedVerb := cleanVerb(verb, req) + dryRun := cleanDryRun(req.URL.Query()["dryRun"]) client := cleanUserAgent(utilnet.GetHTTPClient(req)) elapsedMicroseconds := float64(elapsed / time.Microsecond) elapsedSeconds := elapsed.Seconds() - requestCounter.WithLabelValues(reportedVerb, group, version, resource, subresource, scope, component, client, contentType, codeToString(httpCode)).Inc() + requestCounter.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component, client, contentType, codeToString(httpCode)).Inc() deprecatedRequestCounter.WithLabelValues(reportedVerb, group, version, resource, subresource, scope, component, client, contentType, codeToString(httpCode)).Inc() - requestLatencies.WithLabelValues(reportedVerb, group, version, resource, subresource, scope, component).Observe(elapsedSeconds) + requestLatencies.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component).Observe(elapsedSeconds) deprecatedRequestLatencies.WithLabelValues(reportedVerb, group, version, resource, subresource, scope, component).Observe(elapsedMicroseconds) deprecatedRequestLatenciesSummary.WithLabelValues(reportedVerb, group, version, resource, subresource, scope, component).Observe(elapsedMicroseconds) // We are only interested in response sizes of read requests. @@ -315,9 +323,32 @@ func cleanVerb(verb string, request *http.Request) string { if verb == "WATCHLIST" { reportedVerb = "WATCH" } + if verb == "PATCH" && request.Header.Get("Content-Type") == string(types.ApplyPatchType) { + reportedVerb = "APPLY" + } return reportedVerb } +func cleanDryRun(dryRun []string) string { + if err := validation.ValidateDryRun(nil, dryRun); err != nil { + return "invalid" + } + + // Since dryRun could be valid with any arbitrarily long length + // we have to dedup and sort the elements before joining them together + dryRunSet := map[string]bool{} + for _, element := range dryRun { + dryRunSet[element] = true + } + dryRunUnique := []string{} + for element := range dryRunSet { + dryRunUnique = append(dryRunUnique, element) + } + sort.Strings(dryRunUnique) + + return strings.Join(dryRunUnique, ",") +} + func cleanUserAgent(ua string) string { // We collapse all "web browser"-type user agents into one "browser" to reduce metric cardinality. if strings.HasPrefix(ua, "Mozilla/") { From 51b75460aa356a6b07cd5d727cc3ad6c00a13f20 Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Wed, 6 Mar 2019 15:57:18 -0800 Subject: [PATCH 2/4] handle validation errors correctly --- staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index d307cd18551..149659df9ce 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -330,7 +330,7 @@ func cleanVerb(verb string, request *http.Request) string { } func cleanDryRun(dryRun []string) string { - if err := validation.ValidateDryRun(nil, dryRun); err != nil { + if errs := validation.ValidateDryRun(nil, dryRun); len(errs) > 0 { return "invalid" } From 077dd28df4025c69b89e75b0f32cb0954401ec80 Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Thu, 7 Mar 2019 14:20:10 -0800 Subject: [PATCH 3/4] use utilsets.NewString --- .../k8s.io/apiserver/pkg/endpoints/metrics/BUILD | 1 + .../apiserver/pkg/endpoints/metrics/metrics.go | 15 ++------------- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD index ac80944cf2e..ed35a432068 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD @@ -21,6 +21,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//vendor/github.com/emicklei/go-restful:go_default_library", "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index 149659df9ce..588399dfb15 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -21,7 +21,6 @@ import ( "net" "net/http" "regexp" - "sort" "strconv" "strings" "sync" @@ -30,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/types" utilnet "k8s.io/apimachinery/pkg/util/net" + utilsets "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/endpoints/request" "github.com/emicklei/go-restful" @@ -333,20 +333,9 @@ func cleanDryRun(dryRun []string) string { if errs := validation.ValidateDryRun(nil, dryRun); len(errs) > 0 { return "invalid" } - // Since dryRun could be valid with any arbitrarily long length // we have to dedup and sort the elements before joining them together - dryRunSet := map[string]bool{} - for _, element := range dryRun { - dryRunSet[element] = true - } - dryRunUnique := []string{} - for element := range dryRunSet { - dryRunUnique = append(dryRunUnique, element) - } - sort.Strings(dryRunUnique) - - return strings.Join(dryRunUnique, ",") + return strings.Join(utilsets.NewString(dryRun...).List(), ",") } func cleanUserAgent(ua string) string { From 6e512eb8758fb32878d7baa33daf3c49561086c0 Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Thu, 7 Mar 2019 15:14:43 -0800 Subject: [PATCH 4/4] Feature-gate the APPLY metric value --- staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD | 2 ++ staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD index ed35a432068..4df68923804 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD @@ -23,6 +23,8 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/features:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//vendor/github.com/emicklei/go-restful:go_default_library", "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", ], diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index 588399dfb15..cbe632d14ec 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -31,6 +31,8 @@ import ( utilnet "k8s.io/apimachinery/pkg/util/net" utilsets "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/features" + utilfeature "k8s.io/apiserver/pkg/util/feature" "github.com/emicklei/go-restful" "github.com/prometheus/client_golang/prometheus" @@ -323,7 +325,7 @@ func cleanVerb(verb string, request *http.Request) string { if verb == "WATCHLIST" { reportedVerb = "WATCH" } - if verb == "PATCH" && request.Header.Get("Content-Type") == string(types.ApplyPatchType) { + if verb == "PATCH" && request.Header.Get("Content-Type") == string(types.ApplyPatchType) && utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply) { reportedVerb = "APPLY" } return reportedVerb