From 6cd4f079184e7b338faefd9938def710a9367fc1 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 13 Jul 2020 11:18:22 -0700 Subject: [PATCH 01/37] Create a maximum revision GC setting --- pkg/gc/config.go | 5 +++++ pkg/gc/config_test.go | 10 ++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 78f53fa7faf8..98d0abe62d25 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -39,6 +39,9 @@ type Config struct { StaleRevisionTimeout time.Duration // Minimum number of generations of revisions to keep before considering for GC StaleRevisionMinimumGenerations int64 + // Maximum number of stale revisions to keep before considering for GC. + // Note that if time-bounds are set, the system may retain more than the max specified. + StaleRevisionMaximumGenerations int64 // Minimum staleness duration before updating lastPinned StaleRevisionLastpinnedDebounce time.Duration } @@ -49,6 +52,7 @@ func defaultConfig() *Config { StaleRevisionTimeout: 15 * time.Hour, StaleRevisionLastpinnedDebounce: 5 * time.Hour, StaleRevisionMinimumGenerations: 20, + StaleRevisionMaximumGenerations: 1000, } } @@ -63,6 +67,7 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf cm.AsDuration("stale-revision-timeout", &c.StaleRevisionTimeout), cm.AsDuration("stale-revision-lastpinned-debounce", &c.StaleRevisionLastpinnedDebounce), + cm.AsInt64("stale-revision-maximum-generations", &c.StaleRevisionMaximumGenerations), cm.AsInt64("stale-revision-minimum-generations", &c.StaleRevisionMinimumGenerations), ); err != nil { return nil, fmt.Errorf("failed to parse data: %w", err) diff --git a/pkg/gc/config_test.go b/pkg/gc/config_test.go index 268a883918b3..fc317183ffb9 100644 --- a/pkg/gc/config_test.go +++ b/pkg/gc/config_test.go @@ -50,12 +50,14 @@ func TestOurConfig(t *testing.T) { StaleRevisionCreateDelay: 15 * time.Hour, StaleRevisionTimeout: 14 * time.Hour, StaleRevisionMinimumGenerations: 10, + StaleRevisionMaximumGenerations: 2500, StaleRevisionLastpinnedDebounce: 2*time.Hour + 30*time.Minute + 44*time.Second, }, data: map[string]string{ "stale-revision-create-delay": "15h", "stale-revision-timeout": "14h", "stale-revision-minimum-generations": "10", + "stale-revision-maximum-generations": "2500", "stale-revision-lastpinned-debounce": "2h30m44s", }, }, { @@ -85,13 +87,13 @@ func TestOurConfig(t *testing.T) { want: &Config{ StaleRevisionCreateDelay: 15 * time.Hour, StaleRevisionTimeout: 15 * time.Hour, - StaleRevisionMinimumGenerations: 10, + StaleRevisionMinimumGenerations: 20, + StaleRevisionMaximumGenerations: 1000, StaleRevisionLastpinnedDebounce: 5 * time.Hour, }, data: map[string]string{ - "stale-revision-create-delay": "15h", - "stale-revision-minimum-generations": "10", - "stale-revision-timeout": "1h", + "stale-revision-create-delay": "15h", + "stale-revision-timeout": "1h", }, }} { t.Run(tt.name, func(t *testing.T) { From c08705a7ed07e4779507499fd395ecd742fbc941 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 13 Jul 2020 11:22:04 -0700 Subject: [PATCH 02/37] update yaml alpha note added update checksum fix comments checksum and validation Update config/core/configmaps/gc.yaml Co-authored-by: Victor Agababov fix comments fix unit test fix both check include unit test make new settings for v2 v2 gc settings fix defaults default values fix unit test. expand comment fix validation error Widen collector test timeouts and some cleansing. (#8607) Make sorting consistent in GC test. (#8606) One of the tests ("keep recent lastPinned") failed fairly often because it used two revisions with the same LastPinned timestamp. Sorting a slice is unstable so in a lot of occasions, the given order was flipped and revision 5555 was tried to be deleted, even though it shouldn't even have been considered to be deleted. Note: sort.SliceStable doesn't help because the incoming list's order is already non-consistent. [master] Auto-update dependencies (#8611) Produced via: `./hack/update-deps.sh --upgrade && ./hack/update-codegen.sh` /assign dprotaso vagababov /cc dprotaso vagababov Add route consistency retry to new logging E2E test. (#8324) Handle InitialScale != 1 when creating multiScaler and some tidy-ups (#8612) Now InitialScale has landed, take it into account when calculating initial EBC in multiscaler. Also: - Tidy up some comments. - Fix up some test error messages. - Move mutexes above the things they're guarding. - Use %d and %0.3f for printing numbers, consistently with existing Debug statement at start of same method. Enable HA by default (#8602) * Enable HA by default * Centralize the bucket/replica information for running the HA testing. Add a test to ensure the count of the controller's reconciler stays in sync with the actual controller binary. Remove endpoints from KPA as well. (#8616) * Remove endpoints from KPA as well. Today I noticed that I missed the endpoints code in the KPA itself, when removing that informer. So now really get rid of those. The tests are of course nightmarish :) * remove old cruft Assorted fixes to enable chaos duck. (#8619) * Assorted fixes to enable chaos duck. This lands a handful of fixes that I uncovered preparing to run controlplane chaos testing during our e2e tests. * Drop sleep to 10s --- cmd/controller/main.go | 21 +- cmd/controller/main_test.go | 29 ++ cmd/webhook/main.go | 23 +- config/core/configmaps/gc.yaml | 36 ++- config/core/configmaps/leader-election.yaml | 6 +- go.mod | 8 +- go.sum | 28 +- pkg/autoscaler/metrics/collector_test.go | 17 +- pkg/autoscaler/scaling/autoscaler.go | 8 +- pkg/autoscaler/scaling/multiscaler.go | 20 +- pkg/autoscaler/scaling/multiscaler_test.go | 21 +- pkg/gc/config.go | 51 ++- pkg/gc/config_test.go | 37 ++- pkg/leaderelection/config.go | 34 +- pkg/leaderelection/config_test.go | 38 +-- pkg/reconciler/autoscaling/kpa/controller.go | 14 +- pkg/reconciler/autoscaling/kpa/kpa.go | 25 +- pkg/reconciler/autoscaling/kpa/kpa_test.go | 293 ++++++++---------- pkg/reconciler/autoscaling/kpa/scaler.go | 2 +- pkg/reconciler/gc/v2/gc_test.go | 3 +- test/e2e-common.sh | 6 +- test/e2e-tests.sh | 57 ++-- test/e2e/logging_test.go | 12 +- test/e2e_flags.go | 8 + test/ha/activator_test.go | 8 +- test/ha/autoscalerhpa_test.go | 11 +- test/ha/controller_test.go | 15 +- test/ha/ha.go | 2 +- .../pkg/apis/duck/ducktypes/ducktypes.go | 43 +++ vendor/knative.dev/pkg/apis/duck/register.go | 6 +- .../pkg/apis/duck/v1/addressable_types.go | 10 +- .../pkg/apis/duck/v1/podspec_types.go | 8 +- .../knative.dev/pkg/apis/duck/v1/register.go | 5 +- .../pkg/apis/duck/v1/source_types.go | 11 +- .../pkg/apis/duck/v1/status_types.go | 10 +- vendor/knative.dev/pkg/apis/duck/v1_tests.go | 78 +++++ .../apis/duck/v1beta1/addressable_types.go | 10 +- .../pkg/apis/duck/v1beta1/register.go | 5 +- .../pkg/apis/duck/v1beta1/source_types.go | 11 +- .../pkg/apis/duck/v1beta1/status_types.go | 10 +- .../pkg/apis/duck/v1beta1_tests.go | 56 ++++ vendor/knative.dev/pkg/apis/duck/verify.go | 23 +- vendor/knative.dev/pkg/hash/bucketer.go | 112 +++++++ vendor/knative.dev/pkg/hash/doc.go | 7 +- .../pkg/injection/sharedmain/main.go | 211 +++---------- .../knative.dev/pkg/leaderelection/config.go | 57 ++-- vendor/knative.dev/pkg/test/ha/ha.go | 44 ++- vendor/modules.txt | 9 +- 48 files changed, 880 insertions(+), 679 deletions(-) create mode 100644 cmd/controller/main_test.go create mode 100644 vendor/knative.dev/pkg/apis/duck/ducktypes/ducktypes.go create mode 100644 vendor/knative.dev/pkg/apis/duck/v1_tests.go create mode 100644 vendor/knative.dev/pkg/apis/duck/v1beta1_tests.go create mode 100644 vendor/knative.dev/pkg/hash/bucketer.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index c3bd0d887771..6eb8e97fd6ef 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -27,17 +27,20 @@ import ( "knative.dev/serving/pkg/reconciler/service" // This defines the shared main for injected controllers. + "knative.dev/pkg/injection" "knative.dev/pkg/injection/sharedmain" ) +var ctors = []injection.ControllerConstructor{ + configuration.NewController, + labeler.NewController, + revision.NewController, + route.NewController, + serverlessservice.NewController, + service.NewController, + gc.NewController, +} + func main() { - sharedmain.Main("controller", - configuration.NewController, - labeler.NewController, - revision.NewController, - route.NewController, - serverlessservice.NewController, - service.NewController, - gc.NewController, - ) + sharedmain.Main("controller", ctors...) } diff --git a/cmd/controller/main_test.go b/cmd/controller/main_test.go new file mode 100644 index 000000000000..84e691f182e3 --- /dev/null +++ b/cmd/controller/main_test.go @@ -0,0 +1,29 @@ +/* +Copyright 2020 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "testing" + + "knative.dev/serving/test/ha" +) + +func TestNumController(t *testing.T) { + if got, want := len(ctors), ha.NumControllerReconcilers; got != want { + t.Errorf("Unexpected number of controller = %d, wanted %d. This likely means the constant should be updated.", got, want) + } +} diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index df6ba9c28244..05361ca33706 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -23,7 +23,7 @@ import ( "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/injection/sharedmain" - pkgleaderelection "knative.dev/pkg/leaderelection" + "knative.dev/pkg/leaderelection" "knative.dev/pkg/logging" "knative.dev/pkg/metrics" "knative.dev/pkg/signals" @@ -42,7 +42,6 @@ import ( servingv1 "knative.dev/serving/pkg/apis/serving/v1" servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1" servingv1beta1 "knative.dev/serving/pkg/apis/serving/v1beta1" - "knative.dev/serving/pkg/leaderelection" extravalidation "knative.dev/serving/pkg/webhook" // config validation constructors @@ -152,16 +151,16 @@ func newConfigValidationController(ctx context.Context, cmw configmap.Watcher) * // The configmaps to validate. configmap.Constructors{ - tracingconfig.ConfigName: tracingconfig.NewTracingConfigFromConfigMap, - autoscalerconfig.ConfigName: autoscalerconfig.NewConfigFromConfigMap, - gc.ConfigName: gc.NewConfigFromConfigMapFunc(ctx), - network.ConfigName: network.NewConfigFromConfigMap, - deployment.ConfigName: deployment.NewConfigFromConfigMap, - metrics.ConfigMapName(): metrics.NewObservabilityConfigFromConfigMap, - logging.ConfigMapName(): logging.NewConfigFromConfigMap, - pkgleaderelection.ConfigMapName(): leaderelection.ValidateConfig, - domainconfig.DomainConfigName: domainconfig.NewDomainFromConfigMap, - defaultconfig.DefaultsConfigName: defaultconfig.NewDefaultsConfigFromConfigMap, + tracingconfig.ConfigName: tracingconfig.NewTracingConfigFromConfigMap, + autoscalerconfig.ConfigName: autoscalerconfig.NewConfigFromConfigMap, + gc.ConfigName: gc.NewConfigFromConfigMapFunc(ctx), + network.ConfigName: network.NewConfigFromConfigMap, + deployment.ConfigName: deployment.NewConfigFromConfigMap, + metrics.ConfigMapName(): metrics.NewObservabilityConfigFromConfigMap, + logging.ConfigMapName(): logging.NewConfigFromConfigMap, + leaderelection.ConfigMapName(): leaderelection.NewConfigFromConfigMap, + domainconfig.DomainConfigName: domainconfig.NewDomainFromConfigMap, + defaultconfig.DefaultsConfigName: defaultconfig.NewDefaultsConfigFromConfigMap, }, ) } diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index d8c1e4781a69..21278d8ef7df 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "83219cc3" + knative.dev/example-checksum: "c2432c21" data: _example: | ################################ @@ -47,10 +47,40 @@ data: # resync period (10 hours). stale-revision-timeout: "15h" - # Minimum number of generations of revisions to keep before considering - # them for GC + # Minimum number of generations of non-active revisions to keep before + # considering them for GC. stale-revision-minimum-generations: "20" # To avoid constant updates, we allow an existing annotation to be stale by this # amount before we update the timestamp. stale-revision-lastpinned-debounce: "5h" + + + # --------------------------------------- + # V2 Garbage Collector Settings + # --------------------------------------- + # + # These settings are enabled via the 'responsive-revision-gc' feature flag. + # ALPHA NOTE: This feature is still experimental and under active development. + + # Duration since revision creation before considering it for GC. + gc-retain-since-create-time: "48h" + + # Duration since a route has pointed at the revision + # before considering it for GC. + gc-retain-since-last-active-time: "15h" + + # Minimum number of non-active revisions to retain. + gc-min-stale-revisions: 20 + + # Maximum number of non-active revisions to retain regardless of + # creation or staleness time-bounds. + # + # Instead of allowing revisions to become stale down to gc-min-stale-revisions, + # this mode will keep around revisions up to this maximum limit. A value of 0 + # will immediately delete any non-active revisions. The expected total number + # of revisions is the number of active plus this value. + # + # Set to -1 to disable this setting. Revisions will be eligible for gc as soon + # as they are considered stale as long as there are more than gc-min-stale-revisions. + gc-max-stale-revisions: -1 diff --git a/config/core/configmaps/leader-election.yaml b/config/core/configmaps/leader-election.yaml index e2ac08de71c4..ec90184ec72a 100644 --- a/config/core/configmaps/leader-election.yaml +++ b/config/core/configmaps/leader-election.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "b705abde" + knative.dev/example-checksum: "a255a6cc" data: _example: | ################################ @@ -49,7 +49,3 @@ data: # retryPeriod is how long the leader election client waits between tries of # actions; 2 seconds is the value used by core kubernetes controllers. retryPeriod: "2s" - - # enabledComponents is a comma-delimited list of component names for which - # leader election is enabled. Valid values are: - enabledComponents: "controller,contour-ingress-controller,hpaautoscaler,certcontroller,istiocontroller,net-http01,nscontroller,webhook" diff --git a/go.mod b/go.mod index f4917bfba497..140e07a25e50 100644 --- a/go.mod +++ b/go.mod @@ -38,10 +38,10 @@ require ( k8s.io/client-go v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible k8s.io/code-generator v0.18.0 k8s.io/kube-openapi v0.0.0-20200410145947-bcb3869e6f29 - knative.dev/caching v0.0.0-20200707200344-95a2aaeace0f - knative.dev/networking v0.0.0-20200707203944-725ec013d8a2 - knative.dev/pkg v0.0.0-20200710163519-a0cb3d689532 - knative.dev/test-infra v0.0.0-20200710160019-5b9732bc24f7 + knative.dev/caching v0.0.0-20200713162518-90ce4328c69e + knative.dev/networking v0.0.0-20200713162319-e2731eead7e8 + knative.dev/pkg v0.0.0-20200713194318-a81727701f66 + knative.dev/test-infra v0.0.0-20200713185018-6b52776d44a4 ) replace ( diff --git a/go.sum b/go.sum index 531a2c743ad3..7c674cf10053 100644 --- a/go.sum +++ b/go.sum @@ -38,7 +38,6 @@ cloud.google.com/go/pubsub v1.1.0/go.mod h1:EwwdRX2sKPjnvnqCa270oGRyludottCI76h+ cloud.google.com/go/pubsub v1.2.0 h1:Lpy6hKgdcl7a3WGSfJIFmxmcdjSpP6OmBEfcOv1Y680= cloud.google.com/go/pubsub v1.2.0/go.mod h1:jhfEVHT8odbXTkndysNHCcx0awwzvfOlguIAii9o8iA= cloud.google.com/go/pubsub v1.3.1/go.mod h1:i+ucay31+CNRpDW4Lu78I4xXG+O1r/MAHgjpRVR+TSU= -cloud.google.com/go/pubsub v1.4.0/go.mod h1:LFrqilwgdw4X2cJS9ALgzYmMu+ULyrUN6IHV3CPK4TM= cloud.google.com/go/pubsub v1.5.0/go.mod h1:ZEwJccE3z93Z2HWvstpri00jOg7oO4UZDtKhwDwqF0w= cloud.google.com/go/storage v1.0.0/go.mod h1:IhtSnM/ZTZV8YYJWCY8RULGVqBDmpoyjwiyrjsg+URw= cloud.google.com/go/storage v1.5.0/go.mod h1:tpKbwo567HUNpVclU5sGELwQWBDZ8gh0ZeosJ0Rtdos= @@ -537,7 +536,6 @@ github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/ github.com/google/gofuzz v1.1.0 h1:Hsa8mG0dQ46ij8Sl2AYJDUv1oA9/d6Vk+3LG99Oe02g= github.com/google/gofuzz v1.1.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/licenseclassifier v0.0.0-20190926221455-842c0d70d702/go.mod h1:qsqn2hxC+vURpyBRygGUuinTO42MFRLcsmQ/P8v94+M= -github.com/google/licenseclassifier v0.0.0-20200402202327-879cb1424de0/go.mod h1:qsqn2hxC+vURpyBRygGUuinTO42MFRLcsmQ/P8v94+M= github.com/google/licenseclassifier v0.0.0-20200708223521-3d09a0ea2f39/go.mod h1:qsqn2hxC+vURpyBRygGUuinTO42MFRLcsmQ/P8v94+M= github.com/google/mako v0.0.0-20190821191249-122f8dcef9e3 h1:/o5e44nTD/QEEiWPGSFT3bSqcq3Qg7q27N9bv4gKh5M= github.com/google/mako v0.0.0-20190821191249-122f8dcef9e3/go.mod h1:YzLcVlL+NqWnmUEPuhS1LxDDwGO9WNbVlEXaF4IH35g= @@ -1379,8 +1377,6 @@ golang.org/x/tools v0.0.0-20200515010526-7d3b6ebf133d/go.mod h1:EkVYQZoAsY45+roY golang.org/x/tools v0.0.0-20200527183253-8e7acdbce89d/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200618134242-20370b0cb4b2/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200626171337-aa94e735be7f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= -golang.org/x/tools v0.0.0-20200701000337-a32c0cb1d5b2 h1:xs+dSrelqXhHGIwIftyT5DHxJKH8hbDQnHc5KZ6i/u8= -golang.org/x/tools v0.0.0-20200701000337-a32c0cb1d5b2/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200706234117-b22de6825cf7/go.mod h1:njjCfa9FT2d7l9Bc6FUM5FLjQPp3cFF28FI3qnDFljA= golang.org/x/tools v0.0.0-20200710042808-f1c4188a97a1 h1:rD1FcWVsRaMY+l8biE9jbWP5MS/CJJ/90a9TMkMgNrM= golang.org/x/tools v0.0.0-20200710042808-f1c4188a97a1/go.mod h1:njjCfa9FT2d7l9Bc6FUM5FLjQPp3cFF28FI3qnDFljA= @@ -1467,11 +1463,8 @@ google.golang.org/genproto v0.0.0-20200511104702-f5ebc3bea380/go.mod h1:55QSHmfG google.golang.org/genproto v0.0.0-20200515170657-fc4c6c6a6587/go.mod h1:YsZOwe1myG/8QRHRsmBRE1LrgQY60beZKjly0O1fX9U= google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013/go.mod h1:NbSheEEYHJ7i3ixzK3sjbqSGDJWnxyFXZblF3eUsNvo= google.golang.org/genproto v0.0.0-20200527145253-8367513e4ece/go.mod h1:jDfRM7FcilCzHH/e9qn6dsT145K34l5v+OpcnNgKAAA= -google.golang.org/genproto v0.0.0-20200528110217-3d3490e7e671/go.mod h1:jDfRM7FcilCzHH/e9qn6dsT145K34l5v+OpcnNgKAAA= google.golang.org/genproto v0.0.0-20200618031413-b414f8b61790/go.mod h1:jDfRM7FcilCzHH/e9qn6dsT145K34l5v+OpcnNgKAAA= google.golang.org/genproto v0.0.0-20200626011028-ee7919e894b5/go.mod h1:FWY/as6DDZQgahTzZj3fqbO1CbirC29ZNUFHwi0/+no= -google.golang.org/genproto v0.0.0-20200701001935-0939c5918c31 h1:Of4QP8bfRqzDROen6+s2j/p0jCPgzvQRd9nHiactfn4= -google.golang.org/genproto v0.0.0-20200701001935-0939c5918c31/go.mod h1:FWY/as6DDZQgahTzZj3fqbO1CbirC29ZNUFHwi0/+no= google.golang.org/genproto v0.0.0-20200707001353-8e8330bf89df/go.mod h1:FWY/as6DDZQgahTzZj3fqbO1CbirC29ZNUFHwi0/+no= google.golang.org/genproto v0.0.0-20200710124503-20a17af7bd0e h1:k+p/u26/lVeNEpdxSeUrm7rTvoFckBKaf7gTzgmHyDA= google.golang.org/genproto v0.0.0-20200710124503-20a17af7bd0e/go.mod h1:FWY/as6DDZQgahTzZj3fqbO1CbirC29ZNUFHwi0/+no= @@ -1621,24 +1614,25 @@ k8s.io/legacy-cloud-providers v0.17.4/go.mod h1:FikRNoD64ECjkxO36gkDgJeiQWwyZTuB k8s.io/metrics v0.17.2/go.mod h1:3TkNHET4ROd+NfzNxkjoVfQ0Ob4iZnaHmSEA4vYpwLw= k8s.io/test-infra v0.0.0-20200514184223-ba32c8aae783/go.mod h1:bW6thaPZfL2hW7ecjx2WYwlP9KQLM47/xIJyttkVk5s= k8s.io/test-infra v0.0.0-20200617221206-ea73eaeab7ff/go.mod h1:L3+cRvwftUq8IW1TrHji5m3msnc4uck/7LsE/GR/aZk= -k8s.io/test-infra v0.0.0-20200630233406-1dca6122872e/go.mod h1:L3+cRvwftUq8IW1TrHji5m3msnc4uck/7LsE/GR/aZk= k8s.io/test-infra v0.0.0-20200710134549-5891a1a4cc17/go.mod h1:L3+cRvwftUq8IW1TrHji5m3msnc4uck/7LsE/GR/aZk= k8s.io/utils v0.0.0-20191114184206-e782cd3c129f/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew= k8s.io/utils v0.0.0-20200124190032-861946025e34 h1:HjlUD6M0K3P8nRXmr2B9o4F9dUy9TCj/aEpReeyi6+k= k8s.io/utils v0.0.0-20200124190032-861946025e34/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew= knative.dev/caching v0.0.0-20200116200605-67bca2c83dfa/go.mod h1:dHXFU6CGlLlbzaWc32g80cR92iuBSpsslDNBWI8C7eg= -knative.dev/caching v0.0.0-20200707200344-95a2aaeace0f h1:CwsFW9IKreayHjwzwbnlEtJaiwVYCC3D34AcN4yb6m0= -knative.dev/caching v0.0.0-20200707200344-95a2aaeace0f/go.mod h1:ZQa3DyEIY48qsx5U1ehllwgPHV8rFGzrBB/WonNUzLs= +knative.dev/caching v0.0.0-20200713162518-90ce4328c69e h1:ABjk18hjZYryC5Rs7YNT5PalPwXzfA3COKId0gWXuio= +knative.dev/caching v0.0.0-20200713162518-90ce4328c69e/go.mod h1:7I1DXX8uZX74qggUoUse5ZCaTuMIKyTVBZBpr/cmlaQ= knative.dev/eventing-contrib v0.11.2/go.mod h1:SnXZgSGgMSMLNFTwTnpaOH7hXDzTFtw0J8OmHflNx3g= -knative.dev/networking v0.0.0-20200707203944-725ec013d8a2 h1:Co9j0Q4ZJxwkzVFKUc6AsIXrdiASbaKdHBUROnfiej4= -knative.dev/networking v0.0.0-20200707203944-725ec013d8a2/go.mod h1:e1NL29AarTcgaR240oc4GUzqHtTfTu62JNrUHN3kIG0= +knative.dev/networking v0.0.0-20200713162319-e2731eead7e8 h1:BYQ/DJ1CQ0TQHzrrgdlg3zmL3djM/c4N4utLSEh9Fr8= +knative.dev/networking v0.0.0-20200713162319-e2731eead7e8/go.mod h1:9LCtmPUoygQ+M1ujGZeYcytAF3bDR42rlINsBhge06o= knative.dev/pkg v0.0.0-20200207155214-fef852970f43/go.mod h1:pgODObA1dTyhNoFxPZTTjNWfx6F0aKsKzn+vaT9XO/Q= -knative.dev/pkg v0.0.0-20200707190344-0a8314b44495/go.mod h1:AqAJV6rYi8IGikDjJ/9ZQd9qKdkXVlesVnVjwx62YB8= -knative.dev/pkg v0.0.0-20200710163519-a0cb3d689532 h1:1dSRUstjueNPF06yxGlmeEzzMZpQJnlddA4VehDuTp8= -knative.dev/pkg v0.0.0-20200710163519-a0cb3d689532/go.mod h1:AqAJV6rYi8IGikDjJ/9ZQd9qKdkXVlesVnVjwx62YB8= -knative.dev/test-infra v0.0.0-20200707183444-aed09e56ddc7/go.mod h1:RjYAhXnZqeHw9+B0zsbqSPlae0lCvjekO/nw5ZMpLCs= -knative.dev/test-infra v0.0.0-20200710160019-5b9732bc24f7 h1:fAl3pG2I323tie8kuuNlB88B7RB8WJtCrsXIKuNh1U8= +knative.dev/pkg v0.0.0-20200713031612-b09a159e12c9/go.mod h1:aWPsPIHISvZetAm/2pnz+v6Ro5EYaX704Z/Zd9rTZ4M= +knative.dev/pkg v0.0.0-20200713194318-a81727701f66 h1:H9s47uSb5NCRvnsyIQQpWo5q/cRJ5qEDpm/5pwdnPEg= +knative.dev/pkg v0.0.0-20200713194318-a81727701f66/go.mod h1:2xVLIH5SNUripobZvOEz3w/Ta9xqMkw7QmFIa2cbDFY= knative.dev/test-infra v0.0.0-20200710160019-5b9732bc24f7/go.mod h1:vtT6dLs/iNj8pKcfag8CSVqHKNMgyCFtU/g1pV7Bovs= +knative.dev/test-infra v0.0.0-20200713045417-850e4e37918d h1:Q3LrAYSi+Ii2yZVUiA5Y3Jr4TCU6g/XN9ClVosejpJk= +knative.dev/test-infra v0.0.0-20200713045417-850e4e37918d/go.mod h1:vtT6dLs/iNj8pKcfag8CSVqHKNMgyCFtU/g1pV7Bovs= +knative.dev/test-infra v0.0.0-20200713185018-6b52776d44a4 h1:BYNKY0hC5wsq533k6XbJXi+sb9LNNhM8NQV4mGljR2c= +knative.dev/test-infra v0.0.0-20200713185018-6b52776d44a4/go.mod h1:vtT6dLs/iNj8pKcfag8CSVqHKNMgyCFtU/g1pV7Bovs= modernc.org/cc v1.0.0/go.mod h1:1Sk4//wdnYJiUIxnW8ddKpaOJCF37yAdqYnkxUpaYxw= modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk= modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k= diff --git a/pkg/autoscaler/metrics/collector_test.go b/pkg/autoscaler/metrics/collector_test.go index 5989fb0c5939..149e5c9bc22a 100644 --- a/pkg/autoscaler/metrics/collector_test.go +++ b/pkg/autoscaler/metrics/collector_test.go @@ -154,7 +154,7 @@ func TestMetricCollectorScraper(t *testing.T) { mtp.Channel <- now var gotRPS, gotConcurrency, panicRPS, panicConcurrency float64 // Poll to see that the async loop completed. - wait.PollImmediate(10*time.Millisecond, 100*time.Millisecond, func() (bool, error) { + wait.PollImmediate(10*time.Millisecond, 1*time.Second, func() (bool, error) { gotConcurrency, panicConcurrency, _ = coll.StableAndPanicConcurrency(metricKey, now) gotRPS, panicRPS, _ = coll.StableAndPanicRPS(metricKey, now) return gotConcurrency == wantConcurrency && @@ -163,9 +163,10 @@ func TestMetricCollectorScraper(t *testing.T) { panicRPS == wantPRPS, nil }) - gotConcurrency, panicConcurrency, _ = coll.StableAndPanicConcurrency(metricKey, now) - gotRPS, panicRPS, err := coll.StableAndPanicRPS(metricKey, now) - if err != nil { + if _, _, err := coll.StableAndPanicConcurrency(metricKey, now); err != nil { + t.Errorf("StableAndPanicConcurrency = %v", err) + } + if _, _, err := coll.StableAndPanicRPS(metricKey, now); err != nil { t.Errorf("StableAndPanicRPS = %v", err) } if panicConcurrency != wantPConcurrency { @@ -186,7 +187,7 @@ func TestMetricCollectorScraper(t *testing.T) { mtp.Channel <- now // Wait for async loop to finish. - wait.PollImmediate(10*time.Millisecond, 100*time.Millisecond, func() (bool, error) { + wait.PollImmediate(10*time.Millisecond, 1*time.Second, func() (bool, error) { gotConcurrency, _, _ = coll.StableAndPanicConcurrency(metricKey, now.Add(defaultMetric.Spec.StableWindow).Add(-5*time.Second)) gotRPS, _, _ = coll.StableAndPanicRPS(metricKey, now.Add(defaultMetric.Spec.StableWindow).Add(-5*time.Second)) return gotConcurrency == reportConcurrency*5 && gotRPS == reportRPS*5, nil @@ -200,12 +201,10 @@ func TestMetricCollectorScraper(t *testing.T) { // Deleting the metric should cause a calculation error. coll.Delete(defaultNamespace, defaultName) - _, _, err = coll.StableAndPanicConcurrency(metricKey, now) - if err != ErrNotCollecting { + if _, _, err := coll.StableAndPanicConcurrency(metricKey, now); err != ErrNotCollecting { t.Errorf("StableAndPanicConcurrency() = %v, want %v", err, ErrNotCollecting) } - _, _, err = coll.StableAndPanicRPS(metricKey, now) - if err != ErrNotCollecting { + if _, _, err := coll.StableAndPanicRPS(metricKey, now); err != ErrNotCollecting { t.Errorf("StableAndPanicRPS() = %v, want %v", err, ErrNotCollecting) } } diff --git a/pkg/autoscaler/scaling/autoscaler.go b/pkg/autoscaler/scaling/autoscaler.go index 94d49caa88d7..4cd240a063c8 100644 --- a/pkg/autoscaler/scaling/autoscaler.go +++ b/pkg/autoscaler/scaling/autoscaler.go @@ -242,7 +242,7 @@ func (a *autoscaler) Scale(ctx context.Context, now time.Time) ScaleResult { // Here we compute two numbers: excess burst capacity and number of activators // for subsetting. - // - the excess burst capacity based on panic value, since we don't want to + // - the excess burst capacity is based on panic value, since we don't want to // be making knee-jerk decisions about Activator in the request path. // Negative EBC means that the deployment does not have enough capacity to serve // the desired burst off hand. @@ -252,7 +252,7 @@ func (a *autoscaler) Scale(ctx context.Context, now time.Time) ScaleResult { // the default number. // if tbc > 0, then revision gets number of activators to support total capacity and // tbc additional units. - // if tbc==-1, then revision gets to number of activators to support total capacity. + // if tbc==-1, then revision gets the number of activators needed to support total capacity. // With default target utilization of 0.7, we're overprovisioning number of needed activators // by rate of 1/0.7=1.42. excessBCF := -1. @@ -260,7 +260,7 @@ func (a *autoscaler) Scale(ctx context.Context, now time.Time) ScaleResult { switch { case a.deciderSpec.TargetBurstCapacity == 0: excessBCF = 0 - // numAct stays 1, only needed to scale from 0. + // numAct stays at MinActivators, only needed to scale from 0. case a.deciderSpec.TargetBurstCapacity > 0: // Extra float64 cast disables fused multiply-subtract to force identical behavior on // all platforms. See floating point section in https://golang.org/ref/spec#Operators. @@ -272,7 +272,7 @@ func (a *autoscaler) Scale(ctx context.Context, now time.Time) ScaleResult { numAct = int32(math.Max(MinActivators, math.Ceil(float64(originalReadyPodsCount)*a.deciderSpec.TotalValue/a.deciderSpec.ActivatorCapacity))) } - logger.Debugf("PodCount=%v Total1PodCapacity=%v ObsStableValue=%v ObsPanicValue=%v TargetBC=%v ExcessBC=%v NumActivators=%d", + logger.Debugf("PodCount=%d Total1PodCapacity=%0.3f ObsStableValue=%0.3f ObsPanicValue=%0.3f TargetBC=%0.3f ExcessBC=%0.3f NumActivators=%d", originalReadyPodsCount, a.deciderSpec.TotalValue, observedStableValue, observedPanicValue, a.deciderSpec.TargetBurstCapacity, excessBCF, numAct) diff --git a/pkg/autoscaler/scaling/multiscaler.go b/pkg/autoscaler/scaling/multiscaler.go index 638b94ae6399..0e06342a21cf 100644 --- a/pkg/autoscaler/scaling/multiscaler.go +++ b/pkg/autoscaler/scaling/multiscaler.go @@ -45,7 +45,7 @@ type Decider struct { Status DeciderStatus } -// DeciderSpec is the parameters in which the Revision should scaled. +// DeciderSpec is the parameters by which the Revision should be scaled. type DeciderSpec struct { MaxScaleUpRate float64 MaxScaleDownRate float64 @@ -159,7 +159,7 @@ func (sr *scalerRunner) updateLatestScale(sRes ScaleResult) bool { ret = true } - // If sign has changed -- then we have to update KPA + // If sign has changed -- then we have to update KPA. ret = ret || !sameSign(sr.decider.Status.ExcessBurstCapacity, sRes.ExcessBurstCapacity) // Update with the latest calculation anyway. @@ -167,18 +167,19 @@ func (sr *scalerRunner) updateLatestScale(sRes ScaleResult) bool { return ret } -// MultiScaler maintains a collection of Uniscalers. +// MultiScaler maintains a collection of UniScalers. type MultiScaler struct { - scalers map[types.NamespacedName]*scalerRunner - scalersMutex sync.RWMutex + scalersMutex sync.RWMutex + scalers map[types.NamespacedName]*scalerRunner + scalersStopCh <-chan struct{} uniScalerFactory UniScalerFactory logger *zap.SugaredLogger - watcher func(types.NamespacedName) watcherMutex sync.RWMutex + watcher func(types.NamespacedName) tickProvider func(time.Duration) *time.Ticker } @@ -234,7 +235,7 @@ func (m *MultiScaler) Create(ctx context.Context, decider *Decider) (*Decider, e return scaler.decider, nil } -// Update applied the desired DeciderSpec to a currently running Decider. +// Update applies the desired DeciderSpec to a currently running Decider. func (m *MultiScaler) Update(_ context.Context, decider *Decider) (*Decider, error) { key := types.NamespacedName{Namespace: decider.Namespace, Name: decider.Name} m.scalersMutex.Lock() @@ -324,10 +325,9 @@ func (m *MultiScaler) createScaler(ctx context.Context, decider *Decider) (*scal case -1, 0: d.Status.ExcessBurstCapacity = int32(tbc) default: - // If TBC > Target * InitialScale (currently 1), then we know initial + // If TBC > Target * InitialScale, then we know initial // scale won't be enough to cover TBC and we'll be behind activator. - // TODO(autoscale-wg): fix this when we switch to non "1" initial scale. - d.Status.ExcessBurstCapacity = int32(1*d.Spec.TotalValue - tbc) + d.Status.ExcessBurstCapacity = int32(float64(d.Spec.InitialScale)*d.Spec.TotalValue - tbc) } m.runScalerTicker(ctx, runner) diff --git a/pkg/autoscaler/scaling/multiscaler_test.go b/pkg/autoscaler/scaling/multiscaler_test.go index ac3748b416aa..058e78169935 100644 --- a/pkg/autoscaler/scaling/multiscaler_test.go +++ b/pkg/autoscaler/scaling/multiscaler_test.go @@ -50,7 +50,7 @@ func watchFunc(ctx context.Context, ms *MultiScaler, decider *Decider, desiredSc return } if got, want := m.Status.DesiredScale, int32(desiredScale); got != want { - errCh <- fmt.Errorf("Get() = %v, wanted %v", got, want) + errCh <- fmt.Errorf("DesiredScale = %v, wanted %v", got, want) return } errCh <- nil @@ -114,10 +114,10 @@ func TestMultiScalerScaling(t *testing.T) { t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) } if got, want := d.Status.ExcessBurstCapacity, int32(0); got != want { - t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) + t.Errorf("Decider.Status.ExcessBurstCapacity = %d, want: %d", got, want) } if got, want := d.Status.NumActivators, int32(0); got != want { - t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) + t.Errorf("Decider.Status.NumActivators = %d, want: %d", got, want) } // Verify that we see a "tick" @@ -135,10 +135,10 @@ func TestMultiScalerScaling(t *testing.T) { t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) } if got, want := d.Status.ExcessBurstCapacity, int32(1); got != want { - t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) + t.Errorf("Decider.Status.ExcessBurstCapacity = %d, want: %d", got, want) } if got, want := d.Status.NumActivators, int32(2); got != want { - t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) + t.Errorf("Decider.Status.NumActivators = %d, want: %d", got, want) } // Change number of activators, keeping the other data the same. E.g. CM @@ -158,10 +158,10 @@ func TestMultiScalerScaling(t *testing.T) { t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) } if got, want := d.Status.ExcessBurstCapacity, int32(1); got != want { - t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) + t.Errorf("Decider.Status.ExcessBurstCapacity = %d, want: %d", got, want) } if got, want := d.Status.NumActivators, int32(3); got != want { - t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) + t.Errorf("Decider.Status.NumActivators = %d, want: %d", got, want) } // Verify that subsequent "ticks" don't trigger a callback, since @@ -188,6 +188,7 @@ func TestMultiscalerCreateTBC42(t *testing.T) { ms, _ := createMultiScaler(ctx, TestLogger(t)) decider := newDecider() + decider.Spec.InitialScale = 2 decider.Spec.TargetBurstCapacity = 42 decider.Spec.TotalValue = 25 @@ -202,8 +203,8 @@ func TestMultiscalerCreateTBC42(t *testing.T) { if got, want := d.Status.DesiredScale, int32(-1); got != want { t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) } - if got, want := d.Status.ExcessBurstCapacity, int32(25-42); got != want { - t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) + if got, want := d.Status.ExcessBurstCapacity, int32(50-42); got != want { + t.Errorf("Decider.Status.ExcessBurstCapacity = %d, want: %d", got, want) } if err := ms.Delete(ctx, decider.Namespace, decider.Name); err != nil { t.Errorf("Delete() = %v", err) @@ -230,7 +231,7 @@ func TestMultiscalerCreateTBCMinus1(t *testing.T) { t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) } if got, want := d.Status.ExcessBurstCapacity, int32(-1); got != want { - t.Errorf("Decider.Status.DesiredScale = %d, want: %d", got, want) + t.Errorf("Decider.Status.ExcessBurstCapacity = %d, want: %d", got, want) } if err := ms.Delete(ctx, decider.Namespace, decider.Name); err != nil { t.Errorf("Delete() = %v", err) diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 98d0abe62d25..3a3d1a24c8d3 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -37,22 +37,41 @@ type Config struct { // Timeout since a revision lastPinned before it should be GC'd // This must be longer than the controller resync period StaleRevisionTimeout time.Duration - // Minimum number of generations of revisions to keep before considering for GC + // Minimum number of generations of revisions to keep before considering for GC. StaleRevisionMinimumGenerations int64 - // Maximum number of stale revisions to keep before considering for GC. - // Note that if time-bounds are set, the system may retain more than the max specified. - StaleRevisionMaximumGenerations int64 // Minimum staleness duration before updating lastPinned StaleRevisionLastpinnedDebounce time.Duration + + // Duration from creation when a Revision should be considered active + // and exempt from GC. Note that GCMaxStaleRevision may override this if set. + GCRetainSinceCreateTime time.Duration + // Duration from last active when a Revision should be considered active + // and exempt from GC.Note that GCMaxStaleRevision may override this if set. + GCRetainSinceLastActiveTime time.Duration + // Minimum number of generations of revisions to keep before considering for GC. + // Set -1 to disable minimum and fill up to max. + // Either min or max must be set. + GCMinStaleRevisions int64 + // Maximum number of stale revisions to keep before considering for GC. + // regardless of creation or staleness time-bounds + // Set -1 to disable this setting. + // Either min or max must be set. + GCMaxStaleRevisions int64 } func defaultConfig() *Config { return &Config{ + // V1 GC Settings StaleRevisionCreateDelay: 48 * time.Hour, StaleRevisionTimeout: 15 * time.Hour, StaleRevisionLastpinnedDebounce: 5 * time.Hour, StaleRevisionMinimumGenerations: 20, - StaleRevisionMaximumGenerations: 1000, + + // V2 GC Settings + GCRetainSinceCreateTime: 48 * time.Hour, + GCRetainSinceLastActiveTime: 15 * time.Hour, + GCMinStaleRevisions: 20, + GCMaxStaleRevisions: -1, } } @@ -66,19 +85,35 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf cm.AsDuration("stale-revision-create-delay", &c.StaleRevisionCreateDelay), cm.AsDuration("stale-revision-timeout", &c.StaleRevisionTimeout), cm.AsDuration("stale-revision-lastpinned-debounce", &c.StaleRevisionLastpinnedDebounce), - - cm.AsInt64("stale-revision-maximum-generations", &c.StaleRevisionMaximumGenerations), cm.AsInt64("stale-revision-minimum-generations", &c.StaleRevisionMinimumGenerations), + + cm.AsDuration("gc-retain-since-create-time", &c.GCRetainSinceCreateTime), + cm.AsDuration("gc-retain-since-last-active-time", &c.GCRetainSinceLastActiveTime), + cm.AsInt64("gc-min-stale-revisions", &c.GCMinStaleRevisions), + cm.AsInt64("gc-max-stale-revisions", &c.GCMaxStaleRevisions), ); err != nil { return nil, fmt.Errorf("failed to parse data: %w", err) } + if c.GCMaxStaleRevisions >= 0 && c.GCMinStaleRevisions > c.GCMaxStaleRevisions { + return nil, fmt.Errorf( + "gc-min-stale-revisions(%d) must be <= gc-max-stale-revisions(%d)", + c.GCMinStaleRevisions, c.GCMaxStaleRevisions) + } + if c.GCMinStaleRevisions < 0 { + return nil, fmt.Errorf("gc-min-stale-revisions must be non-negative, was: %d", c.GCMinStaleRevisions) + } + if c.GCMaxStaleRevisions < -1 { + return nil, fmt.Errorf("gc-max-stale-revisions must be >= -1, was: %d", c.GCMaxStaleRevisions) + } + if c.StaleRevisionMinimumGenerations < 0 { return nil, fmt.Errorf("stale-revision-minimum-generations must be non-negative, was: %d", c.StaleRevisionMinimumGenerations) } if c.StaleRevisionTimeout-c.StaleRevisionLastpinnedDebounce < minRevisionTimeout { - logger.Warnf("Got revision timeout of %v, minimum supported value is %v", c.StaleRevisionTimeout, minRevisionTimeout+c.StaleRevisionLastpinnedDebounce) + logger.Warnf("Got revision timeout of %v, minimum supported value is %v", + c.StaleRevisionTimeout, minRevisionTimeout+c.StaleRevisionLastpinnedDebounce) c.StaleRevisionTimeout = minRevisionTimeout + c.StaleRevisionLastpinnedDebounce } return c, nil diff --git a/pkg/gc/config_test.go b/pkg/gc/config_test.go index fc317183ffb9..6b8bb0ce9a64 100644 --- a/pkg/gc/config_test.go +++ b/pkg/gc/config_test.go @@ -50,15 +50,21 @@ func TestOurConfig(t *testing.T) { StaleRevisionCreateDelay: 15 * time.Hour, StaleRevisionTimeout: 14 * time.Hour, StaleRevisionMinimumGenerations: 10, - StaleRevisionMaximumGenerations: 2500, StaleRevisionLastpinnedDebounce: 2*time.Hour + 30*time.Minute + 44*time.Second, + GCRetainSinceCreateTime: 17 * time.Hour, + GCRetainSinceLastActiveTime: 16 * time.Hour, + GCMinStaleRevisions: 5, + GCMaxStaleRevisions: 500, }, data: map[string]string{ "stale-revision-create-delay": "15h", "stale-revision-timeout": "14h", "stale-revision-minimum-generations": "10", - "stale-revision-maximum-generations": "2500", "stale-revision-lastpinned-debounce": "2h30m44s", + "gc-retain-since-create-time": "17h", + "gc-retain-since-last-active-time": "16h", + "gc-min-stale-revisions": "5", + "gc-max-stale-revisions": "500", }, }, { name: "Invalid duration", @@ -74,6 +80,28 @@ func TestOurConfig(t *testing.T) { data: map[string]string{ "stale-revision-minimum-generations": "-1", }, + }, { + name: "Invalid negative minimum generation v2", + fail: true, + want: nil, + data: map[string]string{ + "gc-min-stale-revisions": "-1", + }, + }, { + name: "Invalid negative maximum generation", + fail: true, + want: nil, + data: map[string]string{ + "gc-max-stale-revisions": "-2", + }, + }, { + name: "Invalid max less than min", + fail: true, + want: nil, + data: map[string]string{ + "gc-min-stale-revisions": "20", + "gc-max-stale-revisions": "10", + }, }, { name: "Invalid minimum generation", fail: true, @@ -88,8 +116,11 @@ func TestOurConfig(t *testing.T) { StaleRevisionCreateDelay: 15 * time.Hour, StaleRevisionTimeout: 15 * time.Hour, StaleRevisionMinimumGenerations: 20, - StaleRevisionMaximumGenerations: 1000, StaleRevisionLastpinnedDebounce: 5 * time.Hour, + GCRetainSinceCreateTime: 48 * time.Hour, + GCRetainSinceLastActiveTime: 15 * time.Hour, + GCMinStaleRevisions: 20, + GCMaxStaleRevisions: -1, }, data: map[string]string{ "stale-revision-create-delay": "15h", diff --git a/pkg/leaderelection/config.go b/pkg/leaderelection/config.go index 74244d1f618b..6e9a729bbd8a 100644 --- a/pkg/leaderelection/config.go +++ b/pkg/leaderelection/config.go @@ -17,39 +17,7 @@ limitations under the License. package leaderelection import ( - "fmt" - - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/util/sets" kle "knative.dev/pkg/leaderelection" ) -var ( - validComponents = sets.NewString( - "controller", - "contour-ingress-controller", - "hpaautoscaler", - "certcontroller", - "istiocontroller", - "net-http01", - "nscontroller", - "webhook", - ) -) - -// ValidateConfig enriches the leader election config validation -// with extra validations specific to serving. -func ValidateConfig(configMap *corev1.ConfigMap) (*kle.Config, error) { - config, err := kle.NewConfigFromMap(configMap.Data) - if err != nil { - return nil, err - } - - for component := range config.EnabledComponents { - if !validComponents.Has(component) { - return nil, fmt.Errorf("invalid enabledComponent %q: valid values are %q", component, validComponents.List()) - } - } - - return config, nil -} +var ValidateConfig = kle.NewConfigFromConfigMap diff --git a/pkg/leaderelection/config_test.go b/pkg/leaderelection/config_test.go index e3f5a3163b40..01055de3493d 100644 --- a/pkg/leaderelection/config_test.go +++ b/pkg/leaderelection/config_test.go @@ -24,7 +24,6 @@ import ( "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/util/sets" . "knative.dev/pkg/configmap/testing" kle "knative.dev/pkg/leaderelection" @@ -32,12 +31,11 @@ import ( func okConfig() *kle.Config { return &kle.Config{ - ResourceLock: "leases", - Buckets: 1, - LeaseDuration: 15 * time.Second, - RenewDeadline: 10 * time.Second, - RetryPeriod: 2 * time.Second, - EnabledComponents: sets.NewString("controller"), + ResourceLock: "leases", + Buckets: 1, + LeaseDuration: 15 * time.Second, + RenewDeadline: 10 * time.Second, + RetryPeriod: 2 * time.Second, } } @@ -46,10 +44,9 @@ func okData() map[string]string { // values in this data come from the defaults suggested in the // code: // https://github.com/kubernetes/client-go/blob/kubernetes-1.16.0/tools/leaderelection/leaderelection.go - "leaseDuration": "15s", - "renewDeadline": "10s", - "retryPeriod": "2s", - "enabledComponents": "controller", + "leaseDuration": "15s", + "renewDeadline": "10s", + "retryPeriod": "2s", } } @@ -71,14 +68,6 @@ func TestValidateConfig(t *testing.T) { return data }(), err: errors.New(`failed to parse "renewDeadline": time: invalid duration not a duration`), - }, { - name: "invalid component", - data: func() map[string]string { - data := okData() - data["enabledComponents"] = "controller,frobulator" - return data - }(), - err: errors.New(`invalid enabledComponent "frobulator": valid values are ["certcontroller" "contour-ingress-controller" "controller" "hpaautoscaler" "istiocontroller" "net-http01" "nscontroller" "webhook"]`), }} for _, tc := range cases { @@ -114,12 +103,11 @@ func TestServingConfig(t *testing.T) { }, { name: "Example config", want: &kle.Config{ - ResourceLock: "leases", - Buckets: 1, - LeaseDuration: 15 * time.Second, - RenewDeadline: 10 * time.Second, - RetryPeriod: 2 * time.Second, - EnabledComponents: validComponents, + ResourceLock: "leases", + Buckets: 1, + LeaseDuration: 15 * time.Second, + RenewDeadline: 10 * time.Second, + RetryPeriod: 2 * time.Second, }, data: example, }} { diff --git a/pkg/reconciler/autoscaling/kpa/controller.go b/pkg/reconciler/autoscaling/kpa/controller.go index df45be23ec44..ef164156303f 100644 --- a/pkg/reconciler/autoscaling/kpa/controller.go +++ b/pkg/reconciler/autoscaling/kpa/controller.go @@ -24,7 +24,6 @@ import ( networkingclient "knative.dev/networking/pkg/client/injection/client" sksinformer "knative.dev/networking/pkg/client/injection/informers/networking/v1alpha1/serverlessservice" - endpointsinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/endpoints" podinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod" servingclient "knative.dev/serving/pkg/client/injection/client" "knative.dev/serving/pkg/client/injection/ducks/autoscaling/v1alpha1/podscalable" @@ -32,7 +31,6 @@ import ( painformer "knative.dev/serving/pkg/client/injection/informers/autoscaling/v1alpha1/podautoscaler" pareconciler "knative.dev/serving/pkg/client/injection/reconciler/autoscaling/v1alpha1/podautoscaler" - "knative.dev/networking/pkg/apis/networking" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/kmeta" @@ -62,7 +60,6 @@ func NewController( logger := logging.FromContext(ctx) paInformer := painformer.Get(ctx) sksInformer := sksinformer.Get(ctx) - endpointsInformer := endpointsinformer.Get(ctx) podsInformer := podinformer.Get(ctx) metricInformer := metricinformer.Get(ctx) psInformerFactory := podscalable.Get(ctx) @@ -77,9 +74,8 @@ func NewController( SKSLister: sksInformer.Lister(), MetricLister: metricInformer.Lister(), }, - endpointsLister: endpointsInformer.Lister(), - podsLister: podsInformer.Lister(), - deciders: deciders, + podsLister: podsInformer.Lister(), + deciders: deciders, } impl := pareconciler.NewImpl(ctx, c, autoscaling.KPA, func(impl *controller.Impl) controller.Options { logger.Info("Setting up ConfigMap receivers") @@ -123,9 +119,9 @@ func NewController( sksInformer.Informer().AddEventHandler(handleMatchingControllers) metricInformer.Informer().AddEventHandler(handleMatchingControllers) - // Watch all the private service endpoints. - endpointsInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{ - FilterFunc: pkgreconciler.LabelFilterFunc(networking.ServiceTypeKey, string(networking.ServiceTypePrivate), false), + // Watch the knative pods. + podsInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{ + FilterFunc: pkgreconciler.LabelExistsFilterFunc(serving.RevisionLabelKey), Handler: controller.HandleAll(impl.EnqueueLabelOfNamespaceScopedResource("", serving.RevisionLabelKey)), }) diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index 5050ba63c5a4..4fed753d26c0 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -59,10 +59,9 @@ type podCounts struct { type Reconciler struct { *areconciler.Base - endpointsLister corev1listers.EndpointsLister - podsLister corev1listers.PodLister - deciders resources.Deciders - scaler *scaler + podsLister corev1listers.PodLister + deciders resources.Deciders + scaler *scaler } // Check that our Reconciler implements pareconciler.Interface @@ -127,29 +126,25 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pa *pav1alpha1.PodAutosc if err != nil { return fmt.Errorf("error reconciling SKS: %w", err) } + // Propagate service name. + pa.Status.ServiceName = sks.Status.ServiceName // Compare the desired and observed resources to determine our situation. - // We fetch private endpoints here, since for scaling we're interested in the actual - // state of the deployment. ready, notReady := 0, 0 - - // Propagate service name. - pa.Status.ServiceName = sks.Status.ServiceName + podCounter := resourceutil.NewPodAccessor(c.podsLister, pa.Namespace, pa.Labels[serving.RevisionLabelKey]) // Currently, SKS.IsReady==True when revision has >0 ready pods. if sks.IsReady() { - podEndpointCounter := resourceutil.NewScopedEndpointsCounter(c.endpointsLister, pa.Namespace, sks.Status.PrivateServiceName) - ready, err = podEndpointCounter.ReadyCount() + ready, err = podCounter.ReadyCount() if err != nil { - return fmt.Errorf("error checking endpoints %s: %w", sks.Status.PrivateServiceName, err) + return fmt.Errorf("error getting ready pods %s: %w", sks.Status.PrivateServiceName, err) } - notReady, err = podEndpointCounter.NotReadyCount() + notReady, err = podCounter.NotReadyCount() if err != nil { - return fmt.Errorf("error checking endpoints %s: %w", sks.Status.PrivateServiceName, err) + return fmt.Errorf("error getting not ready pods %s: %w", sks.Status.PrivateServiceName, err) } } - podCounter := resourceutil.NewPodAccessor(c.podsLister, pa.Namespace, pa.Labels[serving.RevisionLabelKey]) pending, terminating, err := podCounter.PendingTerminatingCount() if err != nil { return fmt.Errorf("error checking pods for revision %s: %w", pa.Labels[serving.RevisionLabelKey], err) diff --git a/pkg/reconciler/autoscaling/kpa/kpa_test.go b/pkg/reconciler/autoscaling/kpa/kpa_test.go index 21f1b2d56812..6d08af214a89 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa_test.go +++ b/pkg/reconciler/autoscaling/kpa/kpa_test.go @@ -32,8 +32,8 @@ import ( fakenetworkingclient "knative.dev/networking/pkg/client/injection/client/fake" fakesksinformer "knative.dev/networking/pkg/client/injection/informers/networking/v1alpha1/serverlessservice/fake" fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake" - fakeendpointsinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/endpoints/fake" _ "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/fake" + fakepodsinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/fake" _ "knative.dev/pkg/client/injection/kube/informers/core/v1/service/fake" fakedynamicclient "knative.dev/pkg/injection/clients/dynamicclient/fake" servingclient "knative.dev/serving/pkg/client/injection/client" @@ -274,10 +274,9 @@ func TestReconcile(t *testing.T) { defaultSKS := sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady) defaultMetric := metric(testNamespace, testRevision) - underscaledEndpoints := makeSKSPrivateEndpoints(underscale, testNamespace, testRevision) - overscaledEndpoints := makeSKSPrivateEndpoints(overscale, testNamespace, testRevision) - defaultEndpoints := makeSKSPrivateEndpoints(1, testNamespace, testRevision) - zeroEndpoints := makeSKSPrivateEndpoints(0, testNamespace, testRevision) + underscaledReady := makeReadyPods(underscale, testNamespace, testRevision) + overscaledReady := makeReadyPods(overscale, testNamespace, testRevision) + defaultReady := makeReadyPods(1, testNamespace, testRevision) deciderKey := struct{}{} retryAttempted := false @@ -300,23 +299,22 @@ func TestReconcile(t *testing.T) { }, { Name: "steady state", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment}, defaultReady...), }, { - Name: "no endpoints, with retry", + Name: "status update retry", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, WithPAMetricsService(privateSvc), WithPAStatusService(testRevision), - withScales(1, defaultScale)), + withScales(0, defaultScale)), defaultSKS, metric(testNamespace, testRevision), defaultDeployment, - }, + }, defaultReady...), WithReactors: []clientgotesting.ReactionFunc{ func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { if retryAttempted || !action.Matches("update", "podautoscalers") || action.GetSubresource() != "status" { @@ -327,26 +325,24 @@ func TestReconcile(t *testing.T) { }, }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: kpa(testNamespace, testRevision, markUnknown, WithPAMetricsService(privateSvc), withScales(1, defaultScale), - WithPAStatusService(testRevision), WithObservedGeneration(1), WithObservedGenerationFailure()), + Object: kpa(testNamespace, testRevision, markActive, + markScaleTargetInitialized, + WithPAMetricsService(privateSvc), withScales(1, defaultScale), + WithPAStatusService(testRevision), WithObservedGeneration(1)), }, { - Object: kpa(testNamespace, testRevision, markUnknown, WithPAMetricsService(privateSvc), withScales(1, defaultScale), - WithPAStatusService(testRevision), WithObservedGeneration(1), WithObservedGenerationFailure()), + Object: kpa(testNamespace, testRevision, markActive, + markScaleTargetInitialized, + WithPAMetricsService(privateSvc), withScales(1, defaultScale), + WithPAStatusService(testRevision), WithObservedGeneration(1)), }}, - WantErr: true, - WantEvents: []string{ - Eventf(corev1.EventTypeWarning, "InternalError", - `error checking endpoints test-revision-private: endpoints "test-revision-private" not found`), - }, }, { Name: "failure-creating-metric-object", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment}, defaultReady...), WithReactors: []clientgotesting.ReactionFunc{ InduceFailure("create", "metrics"), }, @@ -361,13 +357,13 @@ func TestReconcile(t *testing.T) { }, { Name: "failure-updating-metric-object", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), - defaultDeployment, defaultEndpoints, + defaultDeployment, metricWithDiffSvc(testNamespace, testRevision), - }, + }, defaultReady...), WithReactors: []clientgotesting.ReactionFunc{ InduceFailure("update", "metrics"), }, @@ -382,13 +378,11 @@ func TestReconcile(t *testing.T) { }, { Name: "create metric", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withScales(1, defaultScale), WithPAStatusService(testRevision)), - defaultSKS, - defaultDeployment, - defaultEndpoints, - }, + defaultSKS, defaultDeployment, + }, defaultReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withScales(1, defaultScale), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1)), @@ -399,14 +393,13 @@ func TestReconcile(t *testing.T) { }, { Name: "scale up deployment", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultSKS, metric(testNamespace, testRevision), deploy(testNamespace, testRevision), - defaultEndpoints, - }, + }, defaultReady...), WantPatches: []clientgotesting.PatchActionImpl{{ ActionImpl: clientgotesting.ActionImpl{ Namespace: testNamespace, @@ -421,14 +414,13 @@ func TestReconcile(t *testing.T) { InduceFailure("patch", "deployments"), }, WantErr: true, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), defaultSKS, metric(testNamespace, testRevision), deploy(testNamespace, testRevision), - defaultEndpoints, - }, + }, defaultReady...), WantPatches: []clientgotesting.PatchActionImpl{{ ActionImpl: clientgotesting.ActionImpl{ Namespace: testNamespace, @@ -440,21 +432,6 @@ func TestReconcile(t *testing.T) { Eventf(corev1.EventTypeWarning, "InternalError", `error scaling target: failed to apply scale 11 to scale target test-revision-deployment: inducing failure for patch deployments`), }, - }, { - Name: "can't read endpoints", - Key: key, - Objects: []runtime.Object{ - kpa(testNamespace, testRevision, markActive, WithPAMetricsService(privateSvc), - withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), - defaultSKS, - metric(testNamespace, testRevision), - defaultDeployment, - }, - WantErr: true, - WantEvents: []string{ - Eventf(corev1.EventTypeWarning, "InternalError", - `error checking endpoints test-revision-private: endpoints "test-revision-private" not found`), - }, }, { Name: "pa activates", Key: key, @@ -465,8 +442,6 @@ func TestReconcile(t *testing.T) { sks(testNamespace, testRevision, WithProxyMode, WithDeployRef(deployName), WithSKSReady), metric(testNamespace, testRevision), defaultDeployment, - // When PA is passive num private endpoints must be 0. - zeroEndpoints, }, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithSKSReady, @@ -479,14 +454,14 @@ func TestReconcile(t *testing.T) { }, { Name: "sks is still not ready", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, WithTraffic, WithPAMetricsService(privateSvc), withScales(0, defaultScale), WithPAStatusService(testRevision)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithPubService, WithPrivateService), metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActivating, withScales(0, defaultScale), WithPAMetricsService(privateSvc), WithPAStatusService(testRevision), WithObservedGeneration(1)), @@ -494,12 +469,12 @@ func TestReconcile(t *testing.T) { }, { Name: "sks becomes ready", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, WithPAMetricsService(privateSvc)), defaultSKS, metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithObservedGeneration(1)), @@ -507,13 +482,13 @@ func TestReconcile(t *testing.T) { }, { Name: "kpa does not become ready without minScale endpoints when reachable", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, withMinScale(2), withScales(1, defaultScale), WithReachabilityReachable, WithPAMetricsService(privateSvc)), defaultSKS, metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActivating, withMinScale(2), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithReachabilityReachable, @@ -522,13 +497,13 @@ func TestReconcile(t *testing.T) { }, { Name: "kpa does not become ready without minScale endpoints when reachability is unknown", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, withMinScale(2), withScales(1, defaultScale), WithPAMetricsService(privateSvc), WithReachabilityUnknown), defaultSKS, metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActivating, withMinScale(2), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithReachabilityUnknown, @@ -537,13 +512,13 @@ func TestReconcile(t *testing.T) { }, { Name: "kpa becomes ready without minScale endpoints when unreachable", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, withMinScale(2), withScales(1, defaultScale), WithPAMetricsService(privateSvc), WithReachabilityUnreachable), defaultSKS, metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withMinScale(2), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithReachabilityUnreachable, @@ -552,14 +527,13 @@ func TestReconcile(t *testing.T) { }, { Name: "kpa becomes ready with minScale endpoints when reachable", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActivating, withMinScale(2), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithReachabilityReachable), defaultSKS, metric(testNamespace, testRevision), defaultDeployment, - makeSKSPrivateEndpoints(2, testNamespace, testRevision), - }, + }, makeReadyPods(2, testNamespace, testRevision)...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withMinScale(2), WithPAMetricsService(privateSvc), withScales(2, defaultScale), WithPAStatusService(testRevision), WithReachabilityReachable, @@ -568,14 +542,13 @@ func TestReconcile(t *testing.T) { }, { Name: "kpa becomes ready with minScale endpoints when reachability is unknown", Key: key, - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActivating, withMinScale(2), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithReachabilityUnknown), defaultSKS, metric(testNamespace, testRevision), defaultDeployment, - makeSKSPrivateEndpoints(2, testNamespace, testRevision), - }, + }, makeReadyPods(2, testNamespace, testRevision)...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withMinScale(2), WithPAMetricsService(privateSvc), withScales(2, defaultScale), WithPAStatusService(testRevision), WithReachabilityUnknown, @@ -702,8 +675,6 @@ func TestReconcile(t *testing.T) { deploy(testNamespace, testRevision, func(d *appsv1.Deployment) { d.Spec.Replicas = ptr.Int32(0) }), - // Should be present, but empty. - zeroEndpoints, }, }, { Name: "steady not serving (scale to zero)", @@ -717,8 +688,6 @@ func TestReconcile(t *testing.T) { sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), metric(testNamespace, testRevision), deploy(testNamespace, testRevision), - // Should be present, but empty. - zeroEndpoints, }, WantPatches: []clientgotesting.PatchActionImpl{{ ActionImpl: clientgotesting.ActionImpl{ @@ -732,13 +701,13 @@ func TestReconcile(t *testing.T) { Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, 0 /* desiredScale */, 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, markOld, withScales(0, 0), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), defaultSKS, metric(testNamespace, testRevision), - deploy(testNamespace, testRevision), defaultEndpoints, - }, + deploy(testNamespace, testRevision), + }, defaultReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, withScales(1, 0), WithPAMetricsService(privateSvc), @@ -755,26 +724,26 @@ func TestReconcile(t *testing.T) { Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, 0 /* desiredScale */, 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withScales(1, 1), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1)), defaultSKS, metric(testNamespace, testRevision), - deploy(testNamespace, testRevision), defaultEndpoints, - }, + deploy(testNamespace, testRevision), + }, defaultReady...), }, { Name: "activation failure", Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, 0 /* desiredScale */, 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActivating, markOld, WithPAStatusService(testRevision), withScales(0, 0), WithPAMetricsService(privateSvc)), defaultSKS, metric(testNamespace, testRevision), - deploy(testNamespace, testRevision), defaultEndpoints, - }, + deploy(testNamespace, testRevision), + }, defaultReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, WithPAMetricsService(privateSvc), WithNoTraffic("TimedOut", "The target could not be activated."), withScales(1, 0), @@ -798,24 +767,24 @@ func TestReconcile(t *testing.T) { Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, unknownScale, 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ - inactiveKPAMinScale(0), underscaledEndpoints, underscaledDeployment, + Objects: append([]runtime.Object{ + inactiveKPAMinScale(0), underscaledDeployment, sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithPubService, WithPrivateService), defaultMetric, - }, + }, underscaledReady...), }, { Name: "want=1, underscaled, PA inactive", // Status -> Activating and Deployment has to be patched. Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, 1, 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ - inactiveKPAMinScale(0), underscaledEndpoints, underscaledDeployment, + Objects: append([]runtime.Object{ + inactiveKPAMinScale(0), underscaledDeployment, sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithPubService, WithPrivateService), defaultMetric, - }, + }, underscaledReady...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: activatingKPAMinScale(0), }}, @@ -832,10 +801,10 @@ func TestReconcile(t *testing.T) { Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, 2 /*autoscaler desired scale*/, 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ - activatingKPAMinScale(underscale), underscaledEndpoints, underscaledDeployment, + Objects: append([]runtime.Object{ + activatingKPAMinScale(underscale), underscaledDeployment, defaultSKS, defaultMetric, - }, + }, underscaledReady...), WantPatches: []clientgotesting.PatchActionImpl{ minScalePatch, }, @@ -845,10 +814,10 @@ func TestReconcile(t *testing.T) { Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, defaultScale, 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ - activeKPAMinScale(underscale, defaultScale), underscaledEndpoints, underscaledDeployment, + Objects: append([]runtime.Object{ + activeKPAMinScale(underscale, defaultScale), underscaledDeployment, defaultSKS, defaultMetric, - }, + }, underscaledReady...), WantPatches: []clientgotesting.PatchActionImpl{ minScalePatch, }, @@ -861,11 +830,11 @@ func TestReconcile(t *testing.T) { Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, 0 /*wantScale*/, 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ - inactiveKPAMinScale(overscale), overscaledEndpoints, overscaledDeployment, + Objects: append([]runtime.Object{ + inactiveKPAMinScale(overscale), overscaledDeployment, sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), defaultMetric, - }, + }, overscaledReady...), WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), }}, @@ -881,11 +850,11 @@ func TestReconcile(t *testing.T) { Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, 1 /*wantScale*/, 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ - inactiveKPAMinScale(overscale), overscaledEndpoints, overscaledDeployment, + Objects: append([]runtime.Object{ + inactiveKPAMinScale(overscale), overscaledDeployment, sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), defaultMetric, - }, + }, overscaledReady...), WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady), }}, @@ -902,10 +871,10 @@ func TestReconcile(t *testing.T) { Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, overscale, /*want more than minScale*/ 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ - activeKPAMinScale(overscale, overscale), overscaledEndpoints, overscaledDeployment, + Objects: append([]runtime.Object{ + activeKPAMinScale(overscale, overscale), overscaledDeployment, defaultSKS, defaultMetric, - }, + }, overscaledReady...), }, { Name: "over maxScale, need to scale down, PA active", // No-op. @@ -913,10 +882,10 @@ func TestReconcile(t *testing.T) { Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, 1, /*less than minScale*/ 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ - activeKPAMinScale(overscale, overscale), overscaledEndpoints, overscaledDeployment, + Objects: append([]runtime.Object{ + activeKPAMinScale(overscale, overscale), overscaledDeployment, defaultSKS, defaultMetric, - }, + }, overscaledReady...), WantPatches: []clientgotesting.PatchActionImpl{ minScalePatch, }, @@ -929,40 +898,40 @@ func TestReconcile(t *testing.T) { Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, unknownScale, /* desiredScale */ 0 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markInactive, WithPAMetricsService(privateSvc), withScales(0, -1), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithPubService, WithPrivateService), metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), }, { Name: "steady not enough capacity", Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -42 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady), metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), }, { Name: "steady, proxy mode, many activators requested", Key: key, Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -42 /* ebc */, 1982 /*numActivators*/)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady, WithNumActivators(4)), metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady, WithNumActivators(1982)), @@ -973,14 +942,14 @@ func TestReconcile(t *testing.T) { Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -18 /* ebc */, scaling.MinActivators+1)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, WithNumActivators(2)), metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithSKSReady, WithDeployRef(deployName), WithProxyMode, WithNumActivators(scaling.MinActivators+1)), @@ -991,14 +960,14 @@ func TestReconcile(t *testing.T) { Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, defaultScale, /* desiredScale */ 1 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, WithPAMetricsService(privateSvc), withScales(1, defaultScale), WithPAStatusService(testRevision), WithObservedGeneration(1)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithSKSReady, WithProxyMode, WithNumActivators(3)), metric(testNamespace, testRevision), - defaultDeployment, defaultEndpoints, - }, + defaultDeployment, + }, defaultReady...), WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: sks(testNamespace, testRevision, WithSKSReady, WithDeployRef(deployName), WithNumActivators(2)), @@ -1009,13 +978,13 @@ func TestReconcile(t *testing.T) { Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -42 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActivating, withScales(defaultScale, defaultScale), WithReachabilityReachable, withMinScale(defaultScale), withInitialScale(20), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc)), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady, WithPubService, WithPrivateService), - defaultMetric, makeSKSPrivateEndpoints(defaultScale, testNamespace, testRevision), defaultDeployment, - }, + defaultMetric, defaultDeployment, + }, makeReadyPods(defaultScale, testNamespace, testRevision)...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActivating, withScales(defaultScale, 20), WithReachabilityReachable, withMinScale(defaultScale), withInitialScale(20), WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), @@ -1033,17 +1002,17 @@ func TestReconcile(t *testing.T) { Ctx: context.WithValue(context.Background(), deciderKey, decider(testNamespace, testRevision, defaultScale, /* desiredScale */ -42 /* ebc */, scaling.MinActivators)), - Objects: []runtime.Object{ + Objects: append([]runtime.Object{ kpa(testNamespace, testRevision, markActivating, withScales(20, defaultScale), withInitialScale(20), WithReachabilityReachable, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), ), sks(testNamespace, testRevision, WithDeployRef(deployName), WithProxyMode, WithSKSReady, WithPubService, WithPrivateService), - defaultMetric, makeSKSPrivateEndpoints(20, testNamespace, testRevision), + defaultMetric, deploy(testNamespace, testRevision, func(d *appsv1.Deployment) { d.Spec.Replicas = ptr.Int32(defaultScale) }), - }, + }, makeReadyPods(20, testNamespace, testRevision)...), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: kpa(testNamespace, testRevision, markActive, markScaleTargetInitialized, withScales(20, 20), withInitialScale(20), WithReachabilityReachable, WithPAStatusService(testRevision), WithPAMetricsService(privateSvc), WithObservedGeneration(1), @@ -1086,10 +1055,9 @@ func TestReconcile(t *testing.T) { SKSLister: listers.GetServerlessServiceLister(), MetricLister: listers.GetMetricLister(), }, - endpointsLister: listers.GetEndpointsLister(), - podsLister: listers.GetPodsLister(), - deciders: fakeDeciders, - scaler: scaler, + podsLister: listers.GetPodsLister(), + deciders: fakeDeciders, + scaler: scaler, } return pareconciler.NewReconciler(ctx, logging.FromContext(ctx), servingclient.Get(ctx), listers.GetPodAutoscalerLister(), @@ -1234,8 +1202,9 @@ func TestReconcileDeciderCreatesAndDeletes(t *testing.T) { rev := newTestRevision(testNamespace, testRevision) fakeservingclient.Get(ctx).ServingV1().Revisions(testNamespace).Create(rev) - ep := makeSKSPrivateEndpoints(1, testNamespace, testRevision) - fakekubeclient.Get(ctx).CoreV1().Endpoints(testNamespace).Create(ep) + pods := makeReadyPods(1, testNamespace, testRevision) + fakekubeclient.Get(ctx).CoreV1().Pods(testNamespace).Create(pods[0].(*corev1.Pod)) + fakepodsinformer.Get(ctx).Informer().GetIndexer().Add(pods[0].(*corev1.Pod)) newDeployment(t, fakedynamicclient.Get(ctx), testRevision+"-deployment", 3) @@ -1295,9 +1264,9 @@ func TestUpdate(t *testing.T) { newDeployment(t, fakedynamicclient.Get(ctx), testRevision+"-deployment", 3) - ep := makeSKSPrivateEndpoints(1, testNamespace, testRevision) - fakekubeclient.Get(ctx).CoreV1().Endpoints(testNamespace).Create(ep) - fakeendpointsinformer.Get(ctx).Informer().GetIndexer().Add(ep) + pods := makeReadyPods(1, testNamespace, testRevision) + fakekubeclient.Get(ctx).CoreV1().Pods(testNamespace).Create(pods[0].(*corev1.Pod)) + fakepodsinformer.Get(ctx).Informer().GetIndexer().Add(pods[0].(*corev1.Pod)) kpa := revisionresources.MakePA(rev) kpa.SetDefaults(context.Background()) @@ -1635,28 +1604,26 @@ func newTestRevision(namespace, name string) *v1.Revision { } } -func makeSKSPrivateEndpoints(num int, ns, n string) *corev1.Endpoints { - eps := &corev1.Endpoints{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: ns, - Name: n + "-private", - }, - } +func makeReadyPods(num int, ns, n string) []runtime.Object { + r := make([]runtime.Object, num) for i := 0; i < num; i++ { - eps = addEndpoint(eps) - } - return eps -} - -func addEndpoint(ep *corev1.Endpoints) *corev1.Endpoints { - if ep.Subsets == nil { - ep.Subsets = []corev1.EndpointSubset{{ - Addresses: []corev1.EndpointAddress{}, - }} + p := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: n + strconv.Itoa(i), + Namespace: ns, + Labels: map[string]string{serving.RevisionLabelKey: n}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + } + r[i] = p } - - ep.Subsets[0].Addresses = append(ep.Subsets[0].Addresses, corev1.EndpointAddress{IP: "127.0.0.1"}) - return ep + return r } func withMinScale(minScale int) PodAutoscalerOption { diff --git a/pkg/reconciler/autoscaling/kpa/scaler.go b/pkg/reconciler/autoscaling/kpa/scaler.go index afc5a81a2edc..4b75a7dacbb6 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler.go +++ b/pkg/reconciler/autoscaling/kpa/scaler.go @@ -319,7 +319,7 @@ func (ks *scaler) scale(ctx context.Context, pa *pav1alpha1.PodAutoscaler, sks * min, max := pa.ScaleBounds() initialScale := kparesources.GetInitialScale(config.FromContext(ctx).Autoscaler, pa) if initialScale > 1 { - // Ignore initial scale if minScale >= initialScale + // Ignore initial scale if minScale >= initialScale. if min < initialScale { logger.Debugf("Adjusting min to meet the initial scale: %d -> %d", min, initialScale) } diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index d48a745a4537..9a77c167722f 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -54,6 +54,7 @@ var revisionSpec = v1.RevisionSpec{ func TestCollect(t *testing.T) { now := time.Now() + nineMinutesAgo := now.Add(-9 * time.Minute) tenMinutesAgo := now.Add(-10 * time.Minute) old := now.Add(-11 * time.Minute) @@ -164,7 +165,7 @@ func TestCollect(t *testing.T) { rev("keep-recent-last-pinned", "foo", 5555, MarkRevisionReady, WithRevName("5555"), WithCreationTimestamp(older), - WithLastPinned(tenMinutesAgo)), + WithLastPinned(nineMinutesAgo)), rev("keep-recent-last-pinned", "foo", 5556, MarkRevisionReady, WithRevName("5556"), WithCreationTimestamp(old), diff --git a/test/e2e-common.sh b/test/e2e-common.sh index ab5a6e30cb1a..c7ca252a92c7 100644 --- a/test/e2e-common.sh +++ b/test/e2e-common.sh @@ -512,6 +512,9 @@ function test_setup() { # Clean up kail so it doesn't interfere with job shutting down add_trap "kill $kail_pid || true" EXIT + echo ">> Waiting for Serving components to be running..." + wait_until_pods_running ${SYSTEM_NAMESPACE} || return 1 + local TEST_CONFIG_DIR=${TEST_DIR}/config echo ">> Creating test resources (${TEST_CONFIG_DIR}/)" ko apply ${KO_FLAGS} -f ${TEST_CONFIG_DIR}/ || return 1 @@ -525,9 +528,6 @@ function test_setup() { echo ">> Uploading test images..." ${REPO_ROOT_DIR}/test/upload-test-images.sh || return 1 - echo ">> Waiting for Serving components to be running..." - wait_until_pods_running ${SYSTEM_NAMESPACE} || return 1 - echo ">> Waiting for Cert Manager components to be running..." wait_until_pods_running cert-manager || return 1 diff --git a/test/e2e-tests.sh b/test/e2e-tests.sh index cd8a336934c2..8d1835f227d3 100755 --- a/test/e2e-tests.sh +++ b/test/e2e-tests.sh @@ -49,24 +49,19 @@ function wait_for_leader_controller() { return 1 } -function enable_tag_header_based_routing() { - echo -n "Enabling Tag Header Based Routing" - kubectl patch cm config-network -n "${SYSTEM_NAMESPACE}" -p '{"data":{"tagHeaderBasedRouting":"Enabled"}}' +function toggle_feature() { + local FEATURE="$1" + local STATE="$2" + local CONFIG="${3:-config-features}" + echo -n "Setting feature ${FEATURE} to ${STATE}" + kubectl patch cm "${CONFIG}" -n "${SYSTEM_NAMESPACE}" -p '{"data":{"'${FEATURE}'":"'${STATE}'"}}' + # We don't have a good mechanism for positive handoff so sleep :( + echo "Waiting 10s for change to get picked up." + sleep 10 } -function disable_tag_header_based_routing() { - echo -n "Disabling Tag Header Based Routing" - kubectl patch cm config-network -n "${SYSTEM_NAMESPACE}" -p '{"data":{"tagHeaderBasedRouting":"Disabled"}}' -} - -function enable_multi_container_feature() { - echo -n "Enabling Multi Container Feature Flag" - kubectl patch cm config-features -n "${SYSTEM_NAMESPACE}" -p '{"data":{"multi-container":"Enabled"}}' -} - -function disable_multi_container_feature() { - echo -n "Disabling Multi Container Feature Flag" - kubectl patch cm config-features -n "${SYSTEM_NAMESPACE}" -p '{"data":{"multi-container":"Disabled"}}' +function toggle_network_feature() { + toggle_feature "$1" "$2" config-network } # Script entry point. @@ -102,20 +97,27 @@ if (( HTTPS )); then add_trap "turn_off_auto_tls" SIGKILL SIGTERM SIGQUIT fi + +# Keep this in sync with test/ha/ha.go +readonly REPLICAS=3 +readonly BUCKETS=10 + + # Enable allow-zero-initial-scale before running e2e tests (for test/e2e/initial_scale_test.go) kubectl -n ${SYSTEM_NAMESPACE} patch configmap/config-autoscaler --type=merge --patch='{"data":{"allow-zero-initial-scale":"true"}}' || failed=1 add_trap "kubectl -n ${SYSTEM_NAMESPACE} patch configmap/config-autoscaler --type=merge --patch='{\"data\":{\"allow-zero-initial-scale\":\"false\"}}'" SIGKILL SIGTERM SIGQUIT +# Keep the bucket count in sync with test/ha/ha.go kubectl -n "${SYSTEM_NAMESPACE}" patch configmap/config-leader-election --type=merge \ - --patch='{"data":{"enabledComponents":"controller,hpaautoscaler,webhook", "buckets": "10"}}' || failed=1 -add_trap "kubectl get cm config-leader-election -n ${SYSTEM_NAMESPACE} -oyaml | sed '/.*enabledComponents.*/d' | kubectl replace -f -" SIGKILL SIGTERM SIGQUIT + --patch='{"data":{"buckets": "'${BUCKETS}'"}}' || failed=1 +add_trap "kubectl get cm config-leader-election -n ${SYSTEM_NAMESPACE} -oyaml | sed '/.*buckets.*/d' | kubectl replace -f -" SIGKILL SIGTERM SIGQUIT # Save activator HPA original values for later use. hpa_spec=$(echo '{"spec": {'$(kubectl get hpa activator -n "knative-serving" -ojsonpath='"minReplicas": {.spec.minReplicas}, "maxReplicas": {.spec.maxReplicas}')'}}') kubectl patch hpa activator -n "${SYSTEM_NAMESPACE}" \ --type "merge" \ - --patch '{"spec": {"minReplicas": 2, "maxReplicas": 2}}' || failed=1 + --patch '{"spec": {"minReplicas": '${REPLICAS}', "maxReplicas": '${REPLICAS}'}}' || failed=1 add_trap "kubectl patch hpa activator -n ${SYSTEM_NAMESPACE} \ --type 'merge' \ --patch $hpa_spec" SIGKILL SIGTERM SIGQUIT @@ -126,7 +128,7 @@ for deployment in controller autoscaler-hpa webhook; do # Give it time to kill the pods. sleep 5 # Scale up components for HA tests - kubectl -n "${SYSTEM_NAMESPACE}" scale deployment "$deployment" --replicas=2 || failed=1 + kubectl -n "${SYSTEM_NAMESPACE}" scale deployment "$deployment" --replicas="${REPLICAS}" || failed=1 done add_trap "for deployment in controller autoscaler-hpa webhook; do \ kubectl -n ${SYSTEM_NAMESPACE} scale deployment $deployment --replicas=0; \ @@ -165,15 +167,15 @@ if (( HTTPS )); then turn_off_auto_tls fi -enable_tag_header_based_routing -add_trap "disable_tag_header_based_routing" SIGKILL SIGTERM SIGQUIT +toggle_network_feature tagHeaderBasedRouting Enabled +add_trap "toggle_network_feature tagHeaderBasedRouting Disabled" SIGKILL SIGTERM SIGQUIT go_test_e2e -timeout=2m ./test/e2e/tagheader || failed=1 -disable_tag_header_based_routing +toggle_network_feature tagHeaderBasedRouting Disabled -enable_multi_container_feature -add_trap "disable_multi_container_feature" SIGKILL SIGTERM SIGQUIT +toggle_feature multi-container Enabled +add_trap "toggle_feature multi-container Disabled" SIGKILL SIGTERM SIGQUIT go_test_e2e -timeout=2m ./test/e2e/multicontainer || failed=1 -disable_multi_container_feature +toggle_feature multi-container Disabled # Certificate conformance tests must be run separately # because they need cert-manager specific configurations. @@ -200,7 +202,8 @@ fi # Run HA tests separately as they're stopping core Knative Serving pods # Define short -spoofinterval to ensure frequent probing while stopping pods -go_test_e2e -timeout=15m -failfast -parallel=1 ./test/ha -spoofinterval="10ms" || failed=1 +go_test_e2e -timeout=15m -failfast -parallel=1 ./test/ha \ + -replicas="${REPLICAS:-1}" -buckets="${BUCKETS:-1}" -spoofinterval="10ms" || failed=1 (( failed )) && fail_test diff --git a/test/e2e/logging_test.go b/test/e2e/logging_test.go index 3f5d3ec25fbc..4392a4861a0e 100644 --- a/test/e2e/logging_test.go +++ b/test/e2e/logging_test.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "knative.dev/pkg/system" + pkgTest "knative.dev/pkg/test" "knative.dev/pkg/test/logstream" "knative.dev/serving/pkg/apis/autoscaling" "knative.dev/serving/pkg/network" @@ -63,9 +64,16 @@ func TestRequestLogs(t *testing.T) { t.Fatalf("Failed to create initial Service: %v: %v", names.Service, err) } - _, err = sendRequest(t, clients, test.ServingFlags.ResolvableDomain, resources.Route.Status.URL.URL()) + _, err = pkgTest.WaitForEndpointState( + clients.KubeClient, + t.Logf, + resources.Route.Status.URL.URL(), + v1test.RetryingRouteInconsistency(pkgTest.MatchesAllOf(pkgTest.IsStatusOK, pkgTest.MatchesBody(test.HelloWorldText))), + "WaitForEndpointToServeText", + test.ServingFlags.ResolvableDomain, + test.AddRootCAtoTransport(t.Logf, clients, test.ServingFlags.Https)) if err != nil { - t.Fatal("Unexpected error when sending request to helloworld:", err) + t.Fatalf("The endpoint didn't serve the expected text %q: %v", test.HelloWorldText, err) } pod, err := theOnlyPod(clients, resources.Revision.Namespace, resources.Revision.Name) diff --git a/test/e2e_flags.go b/test/e2e_flags.go index 153e3e328123..0fb1feaa8f1a 100644 --- a/test/e2e_flags.go +++ b/test/e2e_flags.go @@ -35,6 +35,8 @@ type ServingEnvironmentFlags struct { IngressClass string // Indicates the class of Ingress provider to test. CertificateClass string // Indicates the class of Certificate provider to test. SystemNamespace string // Indicates the system namespace, in which Knative Serving is installed. + Buckets int // The number of reconciler buckets configured. + Replicas int // The number of controlplane replicas being run. } func initializeServingFlags() *ServingEnvironmentFlags { @@ -51,5 +53,11 @@ func initializeServingFlags() *ServingEnvironmentFlags { flag.StringVar(&f.CertificateClass, "certificateClass", network.CertManagerCertificateClassName, "Set this flag to the certificate class to test against.") + flag.IntVar(&f.Buckets, "buckets", 1, + "Set this flag to the number of reconciler buckets configured.") + + flag.IntVar(&f.Replicas, "replicas", 1, + "Set this flag to the number of controlplane replicas being run.") + return &f } diff --git a/test/ha/activator_test.go b/test/ha/activator_test.go index f18e0c57e7f4..4ecc17297e7f 100644 --- a/test/ha/activator_test.go +++ b/test/ha/activator_test.go @@ -23,6 +23,7 @@ import ( "testing" "time" + "knative.dev/networking/pkg/apis/networking" "knative.dev/pkg/ptr" "knative.dev/pkg/system" "knative.dev/pkg/test/logstream" @@ -64,8 +65,8 @@ func testActivatorHA(t *testing.T, gracePeriod *int64, slo float64) { podDeleteOptions := &metav1.DeleteOptions{GracePeriodSeconds: gracePeriod} - if err := pkgTest.WaitForDeploymentScale(clients.KubeClient, activatorDeploymentName, system.Namespace(), haReplicas); err != nil { - t.Fatalf("Deployment %s not scaled to %d: %v", activatorDeploymentName, haReplicas, err) + if err := pkgTest.WaitForDeploymentScale(clients.KubeClient, activatorDeploymentName, system.Namespace(), test.ServingFlags.Replicas); err != nil { + t.Fatalf("Deployment %s not scaled to %d: %v", activatorDeploymentName, test.ServingFlags.Replicas, err) } activators, err := clients.KubeClient.Kube.CoreV1().Pods(system.Namespace()).List(metav1.ListOptions{ LabelSelector: activatorLabel, @@ -125,7 +126,8 @@ func testActivatorHA(t *testing.T, gracePeriod *int64, slo float64) { if err := pkgTest.WaitForPodDeleted(clients.KubeClient, activator.Name, system.Namespace()); err != nil { t.Fatalf("Did not observe %s to actually be deleted: %v", activator.Name, err) } - if err := pkgTest.WaitForServiceEndpoints(clients.KubeClient, resourcesScaleToZero.Revision.Name, test.ServingNamespace, haReplicas); err != nil { + // Check for the endpoint to appear in the activator's endpoints, since this revision may pick a subset of those endpoints. + if err := pkgTest.WaitForServiceEndpoints(clients.KubeClient, networking.ActivatorServiceName, system.Namespace(), test.ServingFlags.Replicas); err != nil { t.Fatalf("Deployment %s failed to scale up: %v", activatorDeploymentName, err) } if gracePeriod != nil && *gracePeriod == 0 { diff --git a/test/ha/autoscalerhpa_test.go b/test/ha/autoscalerhpa_test.go index 2206c281886d..8e912d0701c6 100644 --- a/test/ha/autoscalerhpa_test.go +++ b/test/ha/autoscalerhpa_test.go @@ -21,6 +21,7 @@ package ha import ( "testing" + apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -42,12 +43,12 @@ func TestAutoscalerHPAHANewRevision(t *testing.T) { cancel := logstream.Start(t) defer cancel() - if err := pkgTest.WaitForDeploymentScale(clients.KubeClient, autoscalerHPADeploymentName, system.Namespace(), haReplicas); err != nil { - t.Fatalf("Deployment %s not scaled to %d: %v", autoscalerHPADeploymentName, haReplicas, err) + if err := pkgTest.WaitForDeploymentScale(clients.KubeClient, autoscalerHPADeploymentName, system.Namespace(), test.ServingFlags.Replicas); err != nil { + t.Fatalf("Deployment %s not scaled to %d: %v", autoscalerHPADeploymentName, test.ServingFlags.Replicas, err) } // TODO(mattmoor): Once we switch to the new sharded leader election, we should use more than a single bucket here, but the test is still interesting. - leaders, err := pkgHa.WaitForNewLeaders(t, clients.KubeClient, autoscalerHPADeploymentName, system.Namespace(), sets.NewString(), 1 /* numBuckets */) + leaders, err := pkgHa.WaitForNewLeaders(t, clients.KubeClient, autoscalerHPADeploymentName, system.Namespace(), sets.NewString(), test.ServingFlags.Buckets) if err != nil { t.Fatal("Failed to get leader:", err) } @@ -64,7 +65,7 @@ func TestAutoscalerHPAHANewRevision(t *testing.T) { for _, leader := range leaders.List() { if err := clients.KubeClient.Kube.CoreV1().Pods(system.Namespace()).Delete(leader, - &metav1.DeleteOptions{}); err != nil { + &metav1.DeleteOptions{}); err != nil && !apierrs.IsNotFound(err) { t.Fatalf("Failed to delete pod %s: %v", leader, err) } @@ -74,7 +75,7 @@ func TestAutoscalerHPAHANewRevision(t *testing.T) { } // Wait for all of the old leaders to go away, and then for the right number to be back. - if _, err := pkgHa.WaitForNewLeaders(t, clients.KubeClient, autoscalerHPADeploymentName, system.Namespace(), leaders, 1 /* numBuckets */); err != nil { + if _, err := pkgHa.WaitForNewLeaders(t, clients.KubeClient, autoscalerHPADeploymentName, system.Namespace(), leaders, test.ServingFlags.Buckets); err != nil { t.Fatal("Failed to find new leader:", err) } diff --git a/test/ha/controller_test.go b/test/ha/controller_test.go index 73b662ea5b25..2d52ad2dbc76 100644 --- a/test/ha/controller_test.go +++ b/test/ha/controller_test.go @@ -21,6 +21,7 @@ package ha import ( "testing" + apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -32,19 +33,21 @@ import ( "knative.dev/serving/test/e2e" ) -const controllerDeploymentName = "controller" +const ( + controllerDeploymentName = "controller" +) func TestControllerHA(t *testing.T) { clients := e2e.Setup(t) cancel := logstream.Start(t) defer cancel() - if err := pkgTest.WaitForDeploymentScale(clients.KubeClient, controllerDeploymentName, system.Namespace(), haReplicas); err != nil { - t.Fatalf("Deployment %s not scaled to %d: %v", controllerDeploymentName, haReplicas, err) + if err := pkgTest.WaitForDeploymentScale(clients.KubeClient, controllerDeploymentName, system.Namespace(), test.ServingFlags.Replicas); err != nil { + t.Fatalf("Deployment %s not scaled to %d: %v", controllerDeploymentName, test.ServingFlags.Replicas, err) } // TODO(mattmoor): Once we switch to the new sharded leader election, we should use more than a single bucket here, but the test is still interesting. - leaders, err := pkgHa.WaitForNewLeaders(t, clients.KubeClient, controllerDeploymentName, system.Namespace(), sets.NewString(), 1 /* numBuckets */) + leaders, err := pkgHa.WaitForNewLeaders(t, clients.KubeClient, controllerDeploymentName, system.Namespace(), sets.NewString(), NumControllerReconcilers*test.ServingFlags.Buckets) if err != nil { t.Fatal("Failed to get leader:", err) } @@ -55,7 +58,7 @@ func TestControllerHA(t *testing.T) { for _, leader := range leaders.List() { if err := clients.KubeClient.Kube.CoreV1().Pods(system.Namespace()).Delete(leader, - &metav1.DeleteOptions{}); err != nil { + &metav1.DeleteOptions{}); err != nil && !apierrs.IsNotFound(err) { t.Fatalf("Failed to delete pod %s: %v", leader, err) } @@ -65,7 +68,7 @@ func TestControllerHA(t *testing.T) { } // Wait for all of the old leaders to go away, and then for the right number to be back. - if _, err := pkgHa.WaitForNewLeaders(t, clients.KubeClient, controllerDeploymentName, system.Namespace(), leaders, 1 /* numBuckets */); err != nil { + if _, err := pkgHa.WaitForNewLeaders(t, clients.KubeClient, controllerDeploymentName, system.Namespace(), leaders, NumControllerReconcilers*test.ServingFlags.Buckets); err != nil { t.Fatal("Failed to find new leader:", err) } diff --git a/test/ha/ha.go b/test/ha/ha.go index e5cea896dfe5..2d5fdde38a91 100644 --- a/test/ha/ha.go +++ b/test/ha/ha.go @@ -32,7 +32,7 @@ import ( ) const ( - haReplicas = 2 + NumControllerReconcilers = 7 // Keep in sync with ./cmd/controller/main.go ) func createPizzaPlanetService(t *testing.T, fopt ...rtesting.ServiceOption) (test.ResourceNames, *v1test.ResourceObjects) { diff --git a/vendor/knative.dev/pkg/apis/duck/ducktypes/ducktypes.go b/vendor/knative.dev/pkg/apis/duck/ducktypes/ducktypes.go new file mode 100644 index 000000000000..6f99e0882393 --- /dev/null +++ b/vendor/knative.dev/pkg/apis/duck/ducktypes/ducktypes.go @@ -0,0 +1,43 @@ +/* +Copyright 2020 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package ducktypes + +import ( + "knative.dev/pkg/apis" +) + +// Implementable is implemented by the Fooable duck type that consumers +// are expected to embed as a `.status.fooable` field. +type Implementable interface { + // GetFullType returns an instance of a full resource wrapping + // an instance of this Implementable that can populate its fields + // to verify json roundtripping. + GetFullType() Populatable +} + +// Populatable is implemented by a skeleton resource wrapping an Implementable +// duck type. It will generally have TypeMeta, ObjectMeta, and a Status field +// wrapping a Fooable field. +type Populatable interface { + apis.Listable + + // Populate fills in all possible fields, so that we can verify that + // they roundtrip properly through JSON. + Populate() +} + +const GroupName = "duck.knative.dev" diff --git a/vendor/knative.dev/pkg/apis/duck/register.go b/vendor/knative.dev/pkg/apis/duck/register.go index d84cd49d18b0..93cf55d7fcc1 100644 --- a/vendor/knative.dev/pkg/apis/duck/register.go +++ b/vendor/knative.dev/pkg/apis/duck/register.go @@ -16,8 +16,12 @@ limitations under the License. package duck +import ( + "knative.dev/pkg/apis/duck/ducktypes" +) + const ( - GroupName = "duck.knative.dev" + GroupName = ducktypes.GroupName // AddressableDuckVersionLabel is the label we use to declare // that a type conforms to the Addressable duck type. diff --git a/vendor/knative.dev/pkg/apis/duck/v1/addressable_types.go b/vendor/knative.dev/pkg/apis/duck/v1/addressable_types.go index 579e6976ee85..3cfcac331476 100644 --- a/vendor/knative.dev/pkg/apis/duck/v1/addressable_types.go +++ b/vendor/knative.dev/pkg/apis/duck/v1/addressable_types.go @@ -24,7 +24,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "knative.dev/pkg/apis" - "knative.dev/pkg/apis/duck" + "knative.dev/pkg/apis/duck/ducktypes" ) // +genduck @@ -40,8 +40,6 @@ type Addressable struct { } var ( - // Addressable is an Implementable "duck type". - _ duck.Implementable = (*Addressable)(nil) // Addressable is a Convertible type. _ apis.Convertible = (*Addressable)(nil) ) @@ -66,13 +64,11 @@ type AddressStatus struct { } var ( - // Verify AddressableType resources meet duck contracts. - _ duck.Populatable = (*AddressableType)(nil) - _ apis.Listable = (*AddressableType)(nil) + _ apis.Listable = (*AddressableType)(nil) ) // GetFullType implements duck.Implementable -func (*Addressable) GetFullType() duck.Populatable { +func (*Addressable) GetFullType() ducktypes.Populatable { return &AddressableType{} } diff --git a/vendor/knative.dev/pkg/apis/duck/v1/podspec_types.go b/vendor/knative.dev/pkg/apis/duck/v1/podspec_types.go index 0dd9ec338685..92ed7f1c4ccd 100644 --- a/vendor/knative.dev/pkg/apis/duck/v1/podspec_types.go +++ b/vendor/knative.dev/pkg/apis/duck/v1/podspec_types.go @@ -22,7 +22,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "knative.dev/pkg/apis" - "knative.dev/pkg/apis/duck" + "knative.dev/pkg/apis/duck/ducktypes" ) // +genduck @@ -50,13 +50,11 @@ type WithPodSpec struct { // Assert that we implement the interfaces necessary to // use duck.VerifyType. var ( - _ duck.Populatable = (*WithPod)(nil) - _ duck.Implementable = (*PodSpecable)(nil) - _ apis.Listable = (*WithPod)(nil) + _ apis.Listable = (*WithPod)(nil) ) // GetFullType implements duck.Implementable -func (*PodSpecable) GetFullType() duck.Populatable { +func (*PodSpecable) GetFullType() ducktypes.Populatable { return &WithPod{} } diff --git a/vendor/knative.dev/pkg/apis/duck/v1/register.go b/vendor/knative.dev/pkg/apis/duck/v1/register.go index e3af46d6f8d6..69bb11578383 100644 --- a/vendor/knative.dev/pkg/apis/duck/v1/register.go +++ b/vendor/knative.dev/pkg/apis/duck/v1/register.go @@ -20,11 +20,12 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "knative.dev/pkg/apis/duck" + + "knative.dev/pkg/apis/duck/ducktypes" ) // SchemeGroupVersion is group version used to register these objects -var SchemeGroupVersion = schema.GroupVersion{Group: duck.GroupName, Version: "v1"} +var SchemeGroupVersion = schema.GroupVersion{Group: ducktypes.GroupName, Version: "v1"} // Kind takes an unqualified kind and returns back a Group qualified GroupKind func Kind(kind string) schema.GroupKind { diff --git a/vendor/knative.dev/pkg/apis/duck/v1/source_types.go b/vendor/knative.dev/pkg/apis/duck/v1/source_types.go index 3b378ca3d48f..03c4d40b9b5a 100644 --- a/vendor/knative.dev/pkg/apis/duck/v1/source_types.go +++ b/vendor/knative.dev/pkg/apis/duck/v1/source_types.go @@ -24,12 +24,9 @@ import ( "k8s.io/apimachinery/pkg/runtime" "knative.dev/pkg/apis" - "knative.dev/pkg/apis/duck" + "knative.dev/pkg/apis/duck/ducktypes" ) -// Source is an Implementable "duck type". -var _ duck.Implementable = (*Source)(nil) - // +genduck // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -113,9 +110,7 @@ func (ss *SourceStatus) IsReady() bool { } var ( - // Verify Source resources meet duck contracts. - _ duck.Populatable = (*Source)(nil) - _ apis.Listable = (*Source)(nil) + _ apis.Listable = (*Source)(nil) ) const ( @@ -125,7 +120,7 @@ const ( ) // GetFullType implements duck.Implementable -func (*Source) GetFullType() duck.Populatable { +func (*Source) GetFullType() ducktypes.Populatable { return &Source{} } diff --git a/vendor/knative.dev/pkg/apis/duck/v1/status_types.go b/vendor/knative.dev/pkg/apis/duck/v1/status_types.go index aab246b1f90a..f25212e576db 100644 --- a/vendor/knative.dev/pkg/apis/duck/v1/status_types.go +++ b/vendor/knative.dev/pkg/apis/duck/v1/status_types.go @@ -20,7 +20,7 @@ import ( "context" "knative.dev/pkg/apis" - "knative.dev/pkg/apis/duck" + "knative.dev/pkg/apis/duck/ducktypes" "knative.dev/pkg/kmeta" ) @@ -29,9 +29,6 @@ import ( // Conditions is a simple wrapper around apis.Conditions to implement duck.Implementable. type Conditions apis.Conditions -// Conditions is an Implementable "duck type". -var _ duck.Implementable = (*Conditions)(nil) - // Status shows how we expect folks to embed Conditions in // their Status field. // WARNING: Adding fields to this struct will add them to all Knative resources. @@ -66,14 +63,11 @@ func (s *Status) SetConditions(c apis.Conditions) { s.Conditions = Conditions(c) } -// In order for Conditions to be Implementable, KResource must be Populatable. -var _ duck.Populatable = (*KResource)(nil) - // Ensure KResource satisfies apis.Listable var _ apis.Listable = (*KResource)(nil) // GetFullType implements duck.Implementable -func (*Conditions) GetFullType() duck.Populatable { +func (*Conditions) GetFullType() ducktypes.Populatable { return &KResource{} } diff --git a/vendor/knative.dev/pkg/apis/duck/v1_tests.go b/vendor/knative.dev/pkg/apis/duck/v1_tests.go new file mode 100644 index 000000000000..ba56a994cade --- /dev/null +++ b/vendor/knative.dev/pkg/apis/duck/v1_tests.go @@ -0,0 +1,78 @@ +/* +Copyright 2020 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package duck + +import ( + "testing" + + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" + + v1 "knative.dev/pkg/apis/duck/v1" +) + +// Conditions is an Implementable "duck type". +var _ Implementable = (*v1.Conditions)(nil) + +// In order for Conditions to be Implementable, KResource must be Populatable. +var _ Populatable = (*v1.KResource)(nil) + +// Source is an Implementable "duck type". +var _ Implementable = (*v1.Source)(nil) + +// Verify Source resources meet duck contracts. +var _ Populatable = (*v1.Source)(nil) + +var _ Populatable = (*v1.WithPod)(nil) +var _ Implementable = (*v1.PodSpecable)(nil) + +func TestV1TypesImplements(t *testing.T) { + testCases := []struct { + instance interface{} + iface Implementable + }{ + {instance: &v1.AddressableType{}, iface: &v1.Addressable{}}, + {instance: &v1.KResource{}, iface: &v1.Conditions{}}, + } + for _, tc := range testCases { + if err := VerifyType(tc.instance, tc.iface); err != nil { + t.Error(err) + } + } +} + +func TestV1ImplementsPodSpecable(t *testing.T) { + instances := []interface{}{ + &v1.WithPod{}, + &appsv1.ReplicaSet{}, + &appsv1.Deployment{}, + &appsv1.StatefulSet{}, + &appsv1.DaemonSet{}, + &batchv1.Job{}, + } + for _, instance := range instances { + if err := VerifyType(instance, &v1.PodSpecable{}); err != nil { + t.Error(err) + } + } +} + +// Addressable is an Implementable "duck type". +var _ Implementable = (*v1.Addressable)(nil) + +// Verify AddressableType resources meet duck contracts. +var _ Populatable = (*v1.AddressableType)(nil) diff --git a/vendor/knative.dev/pkg/apis/duck/v1beta1/addressable_types.go b/vendor/knative.dev/pkg/apis/duck/v1beta1/addressable_types.go index 3e3f8286763f..d8984d8ed245 100644 --- a/vendor/knative.dev/pkg/apis/duck/v1beta1/addressable_types.go +++ b/vendor/knative.dev/pkg/apis/duck/v1beta1/addressable_types.go @@ -24,7 +24,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "knative.dev/pkg/apis" - "knative.dev/pkg/apis/duck" + "knative.dev/pkg/apis/duck/ducktypes" v1 "knative.dev/pkg/apis/duck/v1" ) @@ -41,8 +41,6 @@ type Addressable struct { } var ( - // Addressable is an Implementable "duck type". - _ duck.Implementable = (*Addressable)(nil) // Addressable is a Convertible type. _ apis.Convertible = (*Addressable)(nil) ) @@ -67,13 +65,11 @@ type AddressStatus struct { } var ( - // Verify AddressableType resources meet duck contracts. - _ duck.Populatable = (*AddressableType)(nil) - _ apis.Listable = (*AddressableType)(nil) + _ apis.Listable = (*AddressableType)(nil) ) // GetFullType implements duck.Implementable -func (*Addressable) GetFullType() duck.Populatable { +func (*Addressable) GetFullType() ducktypes.Populatable { return &AddressableType{} } diff --git a/vendor/knative.dev/pkg/apis/duck/v1beta1/register.go b/vendor/knative.dev/pkg/apis/duck/v1beta1/register.go index ca8388ad4843..324639c065bd 100644 --- a/vendor/knative.dev/pkg/apis/duck/v1beta1/register.go +++ b/vendor/knative.dev/pkg/apis/duck/v1beta1/register.go @@ -20,11 +20,12 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "knative.dev/pkg/apis/duck" + + "knative.dev/pkg/apis/duck/ducktypes" ) // SchemeGroupVersion is group version used to register these objects -var SchemeGroupVersion = schema.GroupVersion{Group: duck.GroupName, Version: "v1beta1"} +var SchemeGroupVersion = schema.GroupVersion{Group: ducktypes.GroupName, Version: "v1beta1"} // Kind takes an unqualified kind and returns back a Group qualified GroupKind func Kind(kind string) schema.GroupKind { diff --git a/vendor/knative.dev/pkg/apis/duck/v1beta1/source_types.go b/vendor/knative.dev/pkg/apis/duck/v1beta1/source_types.go index 5853a1ae4108..1a9be0d6c377 100644 --- a/vendor/knative.dev/pkg/apis/duck/v1beta1/source_types.go +++ b/vendor/knative.dev/pkg/apis/duck/v1beta1/source_types.go @@ -24,12 +24,9 @@ import ( "k8s.io/apimachinery/pkg/runtime" "knative.dev/pkg/apis" - "knative.dev/pkg/apis/duck" + "knative.dev/pkg/apis/duck/ducktypes" ) -// Source is an Implementable "duck type". -var _ duck.Implementable = (*Source)(nil) - // +genduck // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -98,9 +95,7 @@ func (ss *SourceStatus) IsReady() bool { } var ( - // Verify Source resources meet duck contracts. - _ duck.Populatable = (*Source)(nil) - _ apis.Listable = (*Source)(nil) + _ apis.Listable = (*Source)(nil) ) const ( @@ -110,7 +105,7 @@ const ( ) // GetFullType implements duck.Implementable -func (*Source) GetFullType() duck.Populatable { +func (*Source) GetFullType() ducktypes.Populatable { return &Source{} } diff --git a/vendor/knative.dev/pkg/apis/duck/v1beta1/status_types.go b/vendor/knative.dev/pkg/apis/duck/v1beta1/status_types.go index 107592e85dd2..8054c53bee39 100644 --- a/vendor/knative.dev/pkg/apis/duck/v1beta1/status_types.go +++ b/vendor/knative.dev/pkg/apis/duck/v1beta1/status_types.go @@ -25,7 +25,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "knative.dev/pkg/apis" - "knative.dev/pkg/apis/duck" + "knative.dev/pkg/apis/duck/ducktypes" "knative.dev/pkg/kmeta" ) @@ -34,9 +34,6 @@ import ( // Conditions is a simple wrapper around apis.Conditions to implement duck.Implementable. type Conditions apis.Conditions -// Conditions is an Implementable "duck type". -var _ duck.Implementable = (*Conditions)(nil) - // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // KResource is a skeleton type wrapping Conditions in the manner we expect @@ -84,14 +81,11 @@ func (s *Status) SetConditions(c apis.Conditions) { s.Conditions = Conditions(c) } -// In order for Conditions to be Implementable, KResource must be Populatable. -var _ duck.Populatable = (*KResource)(nil) - // Ensure KResource satisfies apis.Listable var _ apis.Listable = (*KResource)(nil) // GetFullType implements duck.Implementable -func (*Conditions) GetFullType() duck.Populatable { +func (*Conditions) GetFullType() ducktypes.Populatable { return &KResource{} } diff --git a/vendor/knative.dev/pkg/apis/duck/v1beta1_tests.go b/vendor/knative.dev/pkg/apis/duck/v1beta1_tests.go new file mode 100644 index 000000000000..23731d8c79e3 --- /dev/null +++ b/vendor/knative.dev/pkg/apis/duck/v1beta1_tests.go @@ -0,0 +1,56 @@ +/* +Copyright 2020 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package duck + +import ( + "testing" + + "knative.dev/pkg/apis/duck/v1beta1" +) + +// v1beta1.Conditions is an Implementable "duck type". +var _ Implementable = (*v1beta1.Conditions)(nil) + +// In order for v1beta1.Conditions to be Implementable, v1beta1.KResource must be Populatable. +var _ Populatable = (*v1beta1.KResource)(nil) + +// v1beta1.Source is an Implementable "duck type". +var _ Implementable = (*v1beta1.Source)(nil) + +// Verify v1beta1.Source resources meet duck contracts. +var _ Populatable = (*v1beta1.Source)(nil) + +// Addressable is an Implementable "duck type". +var _ Implementable = (*v1beta1.Addressable)(nil) + +// Verify AddressableType resources meet duck contracts. +var _ Populatable = (*v1beta1.AddressableType)(nil) + +func TestV1Beta1TypesImplements(t *testing.T) { + testCases := []struct { + instance interface{} + iface Implementable + }{ + {instance: &v1beta1.AddressableType{}, iface: &v1beta1.Addressable{}}, + {instance: &v1beta1.KResource{}, iface: &v1beta1.Conditions{}}, + } + for _, tc := range testCases { + if err := VerifyType(tc.instance, tc.iface); err != nil { + t.Error(err) + } + } +} diff --git a/vendor/knative.dev/pkg/apis/duck/verify.go b/vendor/knative.dev/pkg/apis/duck/verify.go index 3f42330fff2b..cbcc51117cef 100644 --- a/vendor/knative.dev/pkg/apis/duck/verify.go +++ b/vendor/knative.dev/pkg/apis/duck/verify.go @@ -20,29 +20,12 @@ import ( "encoding/json" "fmt" - "knative.dev/pkg/apis" + "knative.dev/pkg/apis/duck/ducktypes" "knative.dev/pkg/kmp" ) -// Implementable is implemented by the Fooable duck type that consumers -// are expected to embed as a `.status.fooable` field. -type Implementable interface { - // GetFullType returns an instance of a full resource wrapping - // an instance of this Implementable that can populate its fields - // to verify json roundtripping. - GetFullType() Populatable -} - -// Populatable is implemented by a skeleton resource wrapping an Implementable -// duck type. It will generally have TypeMeta, ObjectMeta, and a Status field -// wrapping a Fooable field. -type Populatable interface { - apis.Listable - - // Populate fills in all possible fields, so that we can verify that - // they roundtrip properly through JSON. - Populate() -} +type Implementable = ducktypes.Implementable +type Populatable = ducktypes.Populatable // VerifyType verifies that a particular concrete resource properly implements // the provided Implementable duck type. It is expected that under the resource diff --git a/vendor/knative.dev/pkg/hash/bucketer.go b/vendor/knative.dev/pkg/hash/bucketer.go new file mode 100644 index 000000000000..45f1100ab554 --- /dev/null +++ b/vendor/knative.dev/pkg/hash/bucketer.go @@ -0,0 +1,112 @@ +/* +Copyright 2020 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// This file contains the utilities to make bucketing decisions. + +package hash + +import ( + "sync" + + lru "github.com/hashicorp/golang-lru" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" + "knative.dev/pkg/reconciler" +) + +var _ reconciler.Bucket = (*Bucket)(nil) + +// BucketSet answers to what bucket does key X belong in a +// consistent manner (consistent as in consistent hashing). +type BucketSet struct { + // Stores the cached lookups. cache is internally thread safe. + cache *lru.Cache + + // mu guards buckets. + mu sync.RWMutex + // All the bucket names. Needed for building hash universe. + // `name` must be in this set. + buckets sets.String +} + +// Bucket implements reconciler.Bucket and wraps around BuketSet +// for bucketing functions. +type Bucket struct { + name string + buckets *BucketSet +} + +// Scientifically inferred preferred cache size. +const cacheSize = 4096 + +func newCache() *lru.Cache { + c, _ := lru.New(cacheSize) + return c +} + +// NewBucketSet creates a new bucket set with the given universe +// of bucket names. +func NewBucketSet(bucketList sets.String) *BucketSet { + return &BucketSet{ + cache: newCache(), + buckets: bucketList, + } +} + +// NewBucket creates a new bucket. +func NewBucket(name string, bl *BucketSet) *Bucket { + return &Bucket{ + name: name, + buckets: bl, + } +} + +// Name implements Bucket. +func (b *Bucket) Name() string { + return b.name +} + +// Has returns true if this bucket owns the key and +// implements reconciler.Bucket interface. +func (b *Bucket) Has(nn types.NamespacedName) bool { + return b.buckets.Owner(nn.String()) == b.name +} + +// Owner returns the owner of the key. +// Owner will cache the results for faster lookup. +func (b *BucketSet) Owner(key string) string { + if v, ok := b.cache.Get(key); ok { + return v.(string) + } + b.mu.RLock() + defer b.mu.RUnlock() + l := ChooseSubset(b.buckets, 1 /*single query wanted*/, key) + ret := l.UnsortedList()[0] + b.cache.Add(key, ret) + return ret +} + +// Update updates the universe of buckets. +func (b *BucketSet) Update(newB sets.String) { + b.mu.Lock() + defer b.mu.Unlock() + // In theory we can iterate over the map and + // purge only the keys that moved to a new shard. + // But this might be more expensive than re-build + // the cache as reconciliations happen. + b.cache.Purge() + b.buckets = newB +} diff --git a/vendor/knative.dev/pkg/hash/doc.go b/vendor/knative.dev/pkg/hash/doc.go index 36569b84a246..5cb01faa6372 100644 --- a/vendor/knative.dev/pkg/hash/doc.go +++ b/vendor/knative.dev/pkg/hash/doc.go @@ -17,6 +17,11 @@ limitations under the License. // Package hash contains various Knative specific hashing utilities. // // - ChooseSubset is a consistent hashing/mapping function providing -// a consistent selection of N keys from M (N<=M) keys for a given +// a consistent selection of N keys from M (N<=M) keys for a given // target. +// - BucketSet is a bucketer library which uses ChooseSubset under the +// the hood in order to implement consistent mapping between keys and +// set of buckets, indentified by unique names. Compared to basic bucket +// implementtion which just does hash%num_buckets, when the number of +// buckets change only a small subset of keys are supposed to migrate. package hash diff --git a/vendor/knative.dev/pkg/injection/sharedmain/main.go b/vendor/knative.dev/pkg/injection/sharedmain/main.go index 6c4acf71ec4b..676e47de8240 100644 --- a/vendor/knative.dev/pkg/injection/sharedmain/main.go +++ b/vendor/knative.dev/pkg/injection/sharedmain/main.go @@ -33,14 +33,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/apimachinery/pkg/watch" - "k8s.io/client-go/kubernetes/scheme" - typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" - "k8s.io/client-go/tools/leaderelection" - "k8s.io/client-go/tools/leaderelection/resourcelock" - "k8s.io/client-go/tools/record" "go.uber.org/zap" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -48,7 +42,7 @@ import ( "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/injection" - kle "knative.dev/pkg/leaderelection" + "knative.dev/pkg/leaderelection" "knative.dev/pkg/logging" "knative.dev/pkg/metrics" "knative.dev/pkg/profiling" @@ -108,110 +102,65 @@ func GetLoggingConfig(ctx context.Context) (*logging.Config, error) { } // GetLeaderElectionConfig gets the leader election config. -func GetLeaderElectionConfig(ctx context.Context) (*kle.Config, error) { - leaderElectionConfigMap, err := kubeclient.Get(ctx).CoreV1().ConfigMaps(system.Namespace()).Get(kle.ConfigMapName(), metav1.GetOptions{}) +func GetLeaderElectionConfig(ctx context.Context) (*leaderelection.Config, error) { + leaderElectionConfigMap, err := kubeclient.Get(ctx).CoreV1().ConfigMaps(system.Namespace()).Get(leaderelection.ConfigMapName(), metav1.GetOptions{}) if apierrors.IsNotFound(err) { - return kle.NewConfigFromConfigMap(nil) + return leaderelection.NewConfigFromConfigMap(nil) } else if err != nil { return nil, err } - return kle.NewConfigFromConfigMap(leaderElectionConfigMap) + return leaderelection.NewConfigFromConfigMap(leaderElectionConfigMap) } -// Main runs the generic main flow for non-webhook controllers with a new -// context. Use WebhookMainWith* if you need to serve webhooks. +// Main runs the generic main flow with a new context. +// If any of the contructed controllers are AdmissionControllers or Conversion webhooks, +// then a webhook is started to serve them. func Main(component string, ctors ...injection.ControllerConstructor) { // Set up signals so we handle the first shutdown signal gracefully. MainWithContext(signals.NewContext(), component, ctors...) } -// MainWithContext runs the generic main flow for non-webhook controllers. Use -// WebhookMainWithContext if you need to serve webhooks. -func MainWithContext(ctx context.Context, component string, ctors ...injection.ControllerConstructor) { - MainWithConfig(ctx, component, ParseAndGetConfigOrDie(), ctors...) -} - -// MainWithConfig runs the generic main flow for non-webhook controllers. Use -// WebhookMainWithConfig if you need to serve webhooks. -func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, ctors ...injection.ControllerConstructor) { - log.Printf("Registering %d clients", len(injection.Default.GetClients())) - log.Printf("Registering %d informer factories", len(injection.Default.GetInformerFactories())) - log.Printf("Registering %d informers", len(injection.Default.GetInformers())) - log.Printf("Registering %d controllers", len(ctors)) - - MemStatsOrDie(ctx) - - // Adjust our client's rate limits based on the number of controllers we are running. - cfg.QPS = float32(len(ctors)) * rest.DefaultQPS - cfg.Burst = len(ctors) * rest.DefaultBurst - ctx = injection.WithConfig(ctx, cfg) - - ctx, informers := injection.Default.SetupInformers(ctx, cfg) +// Legacy aliases for back-compat. +var ( + WebhookMainWithContext = MainWithContext + WebhookMainWithConfig = MainWithConfig +) - logger, atomicLevel := SetupLoggerOrDie(ctx, component) - defer flush(logger) - ctx = logging.WithLogger(ctx, logger) - profilingHandler := profiling.NewHandler(logger, false) - profilingServer := profiling.NewServer(profilingHandler) - eg, egCtx := errgroup.WithContext(ctx) - eg.Go(profilingServer.ListenAndServe) - go func() { - // This will block until either a signal arrives or one of the grouped functions - // returns an error. - <-egCtx.Done() - - profilingServer.Shutdown(context.Background()) - if err := eg.Wait(); err != nil && err != http.ErrServerClosed { - logger.Errorw("Error while running server", zap.Error(err)) - } - }() - CheckK8sClientMinimumVersionOrDie(ctx, logger) +// MainWithContext runs the generic main flow for controllers and +// webhooks. Use MainWithContext if you do not need to serve webhooks. +func MainWithContext(ctx context.Context, component string, ctors ...injection.ControllerConstructor) { - run := func(ctx context.Context) { - cmw := SetupConfigMapWatchOrDie(ctx, logger) - controllers, _ := ControllersAndWebhooksFromCtors(ctx, cmw, ctors...) - WatchLoggingConfigOrDie(ctx, cmw, logger, atomicLevel, component) - WatchObservabilityConfigOrDie(ctx, cmw, profilingHandler, logger, component) + // TODO(mattmoor): Remove this once HA is stable. + disableHighAvailability := flag.Bool("disable-ha", false, + "Whether to disable high-availability functionality for this component. This flag will be deprecated "+ + "and removed when we have promoted this feature to stable, so do not pass it without filing an "+ + "issue upstream!") - logger.Info("Starting configuration manager...") - if err := cmw.Start(ctx.Done()); err != nil { - logger.Fatalw("Failed to start configuration manager", zap.Error(err)) - } - logger.Info("Starting informers...") - if err := controller.StartInformers(ctx.Done(), informers...); err != nil { - logger.Fatalw("Failed to start informers", zap.Error(err)) - } - logger.Info("Starting controllers...") - go controller.StartAll(ctx, controllers...) + // HACK: This parses flags, so the above should be set once this runs. + cfg := ParseAndGetConfigOrDie() - <-ctx.Done() + if *disableHighAvailability { + ctx = WithHADisabled(ctx) } - // Set up leader election config - leaderElectionConfig, err := GetLeaderElectionConfig(ctx) - if err != nil { - logger.Fatalw("Error loading leader election configuration", zap.Error(err)) - } - leConfig := leaderElectionConfig.GetComponentConfig(component) + MainWithConfig(ctx, component, cfg, ctors...) +} - if !leConfig.LeaderElect { - logger.Infof("%v will not run in leader-elected mode", component) - run(ctx) - } else { - RunLeaderElected(ctx, logger, run, leConfig) - } +type haDisabledKey struct{} + +// WithHADisabled signals to MainWithConfig that it should not set up an appropriate leader elector for this component. +func WithHADisabled(ctx context.Context) context.Context { + return context.WithValue(ctx, haDisabledKey{}, struct{}{}) } -// WebhookMainWithContext runs the generic main flow for controllers and -// webhooks. Use MainWithContext if you do not need to serve webhooks. -func WebhookMainWithContext(ctx context.Context, component string, ctors ...injection.ControllerConstructor) { - WebhookMainWithConfig(ctx, component, ParseAndGetConfigOrDie(), ctors...) +// IsHADisabled checks the context for the desired to disabled leader elector. +func IsHADisabled(ctx context.Context) bool { + return ctx.Value(haDisabledKey{}) != nil } -// WebhookMainWithConfig runs the generic main flow for controllers and webhooks -// with the given config. Use MainWithConfig if you do not need to serve -// webhooks. -func WebhookMainWithConfig(ctx context.Context, component string, cfg *rest.Config, ctors ...injection.ControllerConstructor) { +// MainWithConfig runs the generic main flow for controllers and webhooks +// with the given config. +func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, ctors ...injection.ControllerConstructor) { log.Printf("Registering %d clients", len(injection.Default.GetClients())) log.Printf("Registering %d informer factories", len(injection.Default.GetInformerFactories())) log.Printf("Registering %d informers", len(injection.Default.GetInformers())) @@ -238,10 +187,11 @@ func WebhookMainWithConfig(ctx context.Context, component string, cfg *rest.Conf if err != nil { logger.Fatalf("Error loading leader election configuration: %v", err) } - leConfig := leaderElectionConfig.GetComponentConfig(component) - if leConfig.LeaderElect { + + if !IsHADisabled(ctx) { // Signal that we are executing in a context with leader election. - ctx = kle.WithDynamicLeaderElectorBuilder(ctx, kubeclient.Get(ctx), leConfig) + ctx = leaderelection.WithDynamicLeaderElectorBuilder(ctx, kubeclient.Get(ctx), + leaderElectionConfig.GetComponentConfig(component)) } controllers, webhooks := ControllersAndWebhooksFromCtors(ctx, cmw, ctors...) @@ -251,6 +201,14 @@ func WebhookMainWithConfig(ctx context.Context, component string, cfg *rest.Conf eg, egCtx := errgroup.WithContext(ctx) eg.Go(profilingServer.ListenAndServe) + // Many of the webhooks rely on configuration, e.g. configurable defaults, feature flags. + // So make sure that we have synchonized our configuration state before launching the + // webhooks, so that things are properly initialized. + logger.Info("Starting configuration manager...") + if err := cmw.Start(ctx.Done()); err != nil { + logger.Fatalw("Failed to start configuration manager", zap.Error(err)) + } + // If we have one or more admission controllers, then start the webhook // and pass them in. var wh *webhook.Webhook @@ -267,10 +225,6 @@ func WebhookMainWithConfig(ctx context.Context, component string, cfg *rest.Conf }) } - logger.Info("Starting configuration manager...") - if err := cmw.Start(ctx.Done()); err != nil { - logger.Fatalw("Failed to start configuration manager", zap.Error(err)) - } logger.Info("Starting informers...") if err := controller.StartInformers(ctx.Done(), informers...); err != nil { logger.Fatalw("Failed to start informers", zap.Error(err)) @@ -414,7 +368,7 @@ func ControllersAndWebhooksFromCtors(ctx context.Context, // Check whether the context has been infused with a leader elector builder. // If it has, then every reconciler we plan to start MUST implement LeaderAware. - leEnabled := kle.HasLeaderElection(ctx) + leEnabled := leaderelection.HasLeaderElection(ctx) controllers := make([]*controller.Impl, 0, len(ctors)) webhooks := make([]interface{}, 0) @@ -437,66 +391,3 @@ func ControllersAndWebhooksFromCtors(ctx context.Context, return controllers, webhooks } - -// RunLeaderElected runs the given function in leader elected mode. The function -// will be run only once the leader election lock is obtained. -func RunLeaderElected(ctx context.Context, logger *zap.SugaredLogger, run func(context.Context), leConfig kle.ComponentConfig) { - recorder := controller.GetEventRecorder(ctx) - if recorder == nil { - // Create event broadcaster - logger.Debug("Creating event broadcaster") - eventBroadcaster := record.NewBroadcaster() - watches := []watch.Interface{ - eventBroadcaster.StartLogging(logger.Named("event-broadcaster").Infof), - eventBroadcaster.StartRecordingToSink( - &typedcorev1.EventSinkImpl{Interface: kubeclient.Get(ctx).CoreV1().Events(system.Namespace())}), - } - recorder = eventBroadcaster.NewRecorder( - scheme.Scheme, corev1.EventSource{Component: leConfig.Component}) - go func() { - <-ctx.Done() - for _, w := range watches { - w.Stop() - } - }() - } - - // Create a unique identifier so that two controllers on the same host don't - // race. - id, err := kle.UniqueID() - if err != nil { - logger.Fatalw("Failed to get unique ID for leader election", zap.Error(err)) - } - logger.Infof("%v will run in leader-elected mode with id %v", leConfig.Component, id) - - // rl is the resource used to hold the leader election lock. - rl, err := resourcelock.New(leConfig.ResourceLock, - system.Namespace(), // use namespace we are running in - leConfig.Component, // component is used as the resource name - kubeclient.Get(ctx).CoreV1(), - kubeclient.Get(ctx).CoordinationV1(), - resourcelock.ResourceLockConfig{ - Identity: id, - EventRecorder: recorder, - }) - if err != nil { - logger.Fatalw("Error creating lock", zap.Error(err)) - } - - // Execute the `run` function when we have the lock. - leaderelection.RunOrDie(ctx, leaderelection.LeaderElectionConfig{ - Lock: rl, - LeaseDuration: leConfig.LeaseDuration, - RenewDeadline: leConfig.RenewDeadline, - RetryPeriod: leConfig.RetryPeriod, - Callbacks: leaderelection.LeaderCallbacks{ - OnStartedLeading: run, - OnStoppedLeading: func() { - logger.Fatal("Leader election lost") - }, - }, - ReleaseOnCancel: true, - // TODO: use health check watchdog, knative/pkg#1048 - Name: leConfig.Component, - }) -} diff --git a/vendor/knative.dev/pkg/leaderelection/config.go b/vendor/knative.dev/pkg/leaderelection/config.go index 880dd8c2b988..b694c4fd206b 100644 --- a/vendor/knative.dev/pkg/leaderelection/config.go +++ b/vendor/knative.dev/pkg/leaderelection/config.go @@ -52,11 +52,6 @@ func NewConfigFromMap(data map[string]string) (*Config, error) { cm.AsDuration("retryPeriod", &config.RetryPeriod), cm.AsUint32("buckets", &config.Buckets), - - // enabledComponents are not validated here, because they are dependent on - // the component. Components should provide additional validation for this - // field. - cm.AsStringSet("enabledComponents", &config.EnabledComponents), ); err != nil { return nil, err } @@ -84,45 +79,42 @@ func NewConfigFromConfigMap(configMap *corev1.ConfigMap) (*Config, error) { // contained within a single namespace. Typically these will correspond to a // single source repository, viz: serving or eventing. type Config struct { - ResourceLock string - Buckets uint32 - LeaseDuration time.Duration - RenewDeadline time.Duration - RetryPeriod time.Duration + ResourceLock string + Buckets uint32 + LeaseDuration time.Duration + RenewDeadline time.Duration + RetryPeriod time.Duration + + // This field is deprecated and will be removed once downstream + // repositories have removed their validation of it. + // TODO(https://github.com/knative/pkg/issues/1478): Remove this field. EnabledComponents sets.String } func (c *Config) GetComponentConfig(name string) ComponentConfig { - if c.EnabledComponents.Has(name) { - return ComponentConfig{ - Component: name, - LeaderElect: true, - Buckets: c.Buckets, - ResourceLock: c.ResourceLock, - LeaseDuration: c.LeaseDuration, - RenewDeadline: c.RenewDeadline, - RetryPeriod: c.RetryPeriod, - } + return ComponentConfig{ + Component: name, + Buckets: c.Buckets, + ResourceLock: c.ResourceLock, + LeaseDuration: c.LeaseDuration, + RenewDeadline: c.RenewDeadline, + RetryPeriod: c.RetryPeriod, } - - return defaultComponentConfig(name) } func defaultConfig() *Config { return &Config{ - ResourceLock: "leases", - Buckets: 1, - LeaseDuration: 15 * time.Second, - RenewDeadline: 10 * time.Second, - RetryPeriod: 2 * time.Second, - EnabledComponents: sets.NewString(), + ResourceLock: "leases", + Buckets: 1, + LeaseDuration: 15 * time.Second, + RenewDeadline: 10 * time.Second, + RetryPeriod: 2 * time.Second, } } // ComponentConfig represents the leader election config for a single component. type ComponentConfig struct { Component string - LeaderElect bool Buckets uint32 ResourceLock string LeaseDuration time.Duration @@ -165,13 +157,6 @@ func newStatefulSetConfig() (*statefulSetConfig, error) { return ssc, nil } -func defaultComponentConfig(name string) ComponentConfig { - return ComponentConfig{ - Component: name, - LeaderElect: false, - } -} - // ConfigMapName returns the name of the configmap to read for leader election // settings. func ConfigMapName() string { diff --git a/vendor/knative.dev/pkg/test/ha/ha.go b/vendor/knative.dev/pkg/test/ha/ha.go index c1b5c045b438..2859cc872854 100644 --- a/vendor/knative.dev/pkg/test/ha/ha.go +++ b/vendor/knative.dev/pkg/test/ha/ha.go @@ -26,17 +26,36 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" + "knative.dev/pkg/test" "knative.dev/pkg/test/logging" ) +func countingRFind(wr rune, wc int) func(rune) bool { + cnt := 0 + return func(r rune) bool { + if r == wr { + cnt++ + } + return cnt == wc + } +} + +func extractDeployment(pod string) string { + if x := strings.LastIndexFunc(pod, countingRFind('-', 2)); x != -1 { + return pod[:x] + } + return "" +} + // GetLeaders collects all of the leader pods from the specified deployment. +// GetLeaders will return duplicate pods by design. func GetLeaders(t *testing.T, client *test.KubeClient, deploymentName, namespace string) ([]string, error) { leases, err := client.Kube.CoordinationV1().Leases(namespace).List(metav1.ListOptions{}) if err != nil { return nil, fmt.Errorf("error getting leases for deployment %q: %w", deploymentName, err) } - var pods []string + ret := make([]string, 0, len(leases.Items)) for _, lease := range leases.Items { if lease.Spec.HolderIdentity == nil { continue @@ -44,16 +63,12 @@ func GetLeaders(t *testing.T, client *test.KubeClient, deploymentName, namespace pod := strings.SplitN(*lease.Spec.HolderIdentity, "_", 2)[0] // Deconstruct the pod name and look for the deployment. This won't work for very long deployment names. - parts := strings.Split(pod, "-") - if len(parts) < 3 { - continue - } - if strings.Join(parts[:len(parts)-2], "-") != deploymentName { + if extractDeployment(pod) != deploymentName { continue } - pods = append(pods, pod) + ret = append(ret, pod) } - return pods, nil + return ret, nil } // WaitForNewLeaders waits until the collection of current leaders consists of "n" leaders @@ -64,19 +79,20 @@ func WaitForNewLeaders(t *testing.T, client *test.KubeClient, deploymentName, na var leaders sets.String err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { - currentLeaders, err := GetLeaders(t, client, deploymentName, namespace) + currLeaders, err := GetLeaders(t, client, deploymentName, namespace) if err != nil { return false, err } - if len(currentLeaders) < n { - t.Logf("WaitForNewLeaders[%s] not enough leaders, got: %d, want: %d", deploymentName, len(currentLeaders), n) + if len(currLeaders) < n { + t.Logf("WaitForNewLeaders[%s] not enough leaders, got: %d, want: %d", deploymentName, len(currLeaders), n) return false, nil } - if previousLeaders.HasAny(currentLeaders...) { - t.Logf("WaitForNewLeaders[%s] still see intersection: %v", deploymentName, previousLeaders.Intersection(sets.NewString(currentLeaders...))) + l := sets.NewString(currLeaders...) + if previousLeaders.HasAny(currLeaders...) { + t.Logf("WaitForNewLeaders[%s] still see intersection: %v", deploymentName, previousLeaders.Intersection(l)) return false, nil } - leaders = sets.NewString(currentLeaders...) + leaders = l return true, nil }) return leaders, err diff --git a/vendor/modules.txt b/vendor/modules.txt index 39360dfb3d1d..156e0ad39d18 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -987,7 +987,7 @@ k8s.io/utils/buffer k8s.io/utils/integer k8s.io/utils/pointer k8s.io/utils/trace -# knative.dev/caching v0.0.0-20200707200344-95a2aaeace0f +# knative.dev/caching v0.0.0-20200713162518-90ce4328c69e ## explicit knative.dev/caching/config knative.dev/caching/pkg/apis/caching @@ -1008,7 +1008,7 @@ knative.dev/caching/pkg/client/injection/informers/caching/v1alpha1/image/fake knative.dev/caching/pkg/client/injection/informers/factory knative.dev/caching/pkg/client/injection/informers/factory/fake knative.dev/caching/pkg/client/listers/caching/v1alpha1 -# knative.dev/networking v0.0.0-20200707203944-725ec013d8a2 +# knative.dev/networking v0.0.0-20200713162319-e2731eead7e8 ## explicit knative.dev/networking/pkg/apis/networking knative.dev/networking/pkg/apis/networking/v1alpha1 @@ -1034,12 +1034,13 @@ knative.dev/networking/pkg/client/injection/informers/networking/v1alpha1/server knative.dev/networking/pkg/client/injection/reconciler/networking/v1alpha1/serverlessservice knative.dev/networking/pkg/client/istio/listers/networking/v1alpha3 knative.dev/networking/pkg/client/listers/networking/v1alpha1 -# knative.dev/pkg v0.0.0-20200710163519-a0cb3d689532 +# knative.dev/pkg v0.0.0-20200713194318-a81727701f66 ## explicit knative.dev/pkg/apiextensions/storageversion knative.dev/pkg/apiextensions/storageversion/cmd/migrate knative.dev/pkg/apis knative.dev/pkg/apis/duck +knative.dev/pkg/apis/duck/ducktypes knative.dev/pkg/apis/duck/v1 knative.dev/pkg/apis/duck/v1alpha1 knative.dev/pkg/apis/duck/v1beta1 @@ -1154,7 +1155,7 @@ knative.dev/pkg/webhook/resourcesemantics/conversion knative.dev/pkg/webhook/resourcesemantics/defaulting knative.dev/pkg/webhook/resourcesemantics/validation knative.dev/pkg/websocket -# knative.dev/test-infra v0.0.0-20200710160019-5b9732bc24f7 +# knative.dev/test-infra v0.0.0-20200713185018-6b52776d44a4 ## explicit knative.dev/test-infra/scripts # sigs.k8s.io/yaml v1.2.0 From c6fc36631cf5440ef57f0373e351deef297b0ef0 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 13 Jul 2020 20:35:40 -0700 Subject: [PATCH 03/37] Use the new GC settings --- pkg/reconciler/gc/v2/gc.go | 16 ++++++++-------- pkg/reconciler/gc/v2/gc_test.go | 23 +++++++++-------------- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 1fbfaf1b5a9e..cf4df91e7ae5 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -30,6 +30,7 @@ import ( v1 "knative.dev/serving/pkg/apis/serving/v1" clientset "knative.dev/serving/pkg/client/clientset/versioned" listers "knative.dev/serving/pkg/client/listers/serving/v1" + "knative.dev/serving/pkg/gc" configns "knative.dev/serving/pkg/reconciler/gc/config" ) @@ -48,7 +49,7 @@ func Collect( return err } - gcSkipOffset := cfg.StaleRevisionMinimumGenerations + gcSkipOffset := cfg.GCMinStaleRevisions if gcSkipOffset >= int64(len(revs)) { return nil @@ -61,7 +62,7 @@ func Collect( }) for _, rev := range revs[gcSkipOffset:] { - if isRevisionStale(ctx, rev, config) { + if isRevisionStale(ctx, cfg, rev, config) { err := client.ServingV1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}) if err != nil { logger.With(zap.Error(err)).Errorf("Failed to delete stale revision %q", rev.Name) @@ -72,17 +73,16 @@ func Collect( return nil } -func isRevisionStale(ctx context.Context, rev *v1.Revision, config *v1.Configuration) bool { +func isRevisionStale(ctx context.Context, cfg *gc.Config, rev *v1.Revision, config *v1.Configuration) bool { if config.Status.LatestReadyRevisionName == rev.Name { return false } - cfg := configns.FromContext(ctx).RevisionGC logger := logging.FromContext(ctx) curTime := time.Now() createTime := rev.ObjectMeta.CreationTimestamp - if createTime.Add(cfg.StaleRevisionCreateDelay).After(curTime) { - // Revision was created sooner than staleRevisionCreateDelay. Ignore it. + if createTime.Add(cfg.GCRetainSinceCreateTime).After(curTime) { + // Revision was created sooner than GCRetainSinceCreateTime. Ignore it. return false } @@ -91,12 +91,12 @@ func isRevisionStale(ctx context.Context, rev *v1.Revision, config *v1.Configura // TODO(whaught): this is carried over from v1, but I'm not sure why we can't delete a ready revision // that isn't referenced? Maybe because of labeler failure - can we replace this with 'pending' routing state check? if lastActive.Equal(createTime.Time) { - // Revision was never active and it's not ready after staleRevisionCreateDelay. + // Revision was never active and it's not ready after GCRetainSinceCreateTime. // It usually happens when ksvc was deployed with wrong configuration. return !rev.Status.GetCondition(v1.RevisionConditionReady).IsTrue() } - ret := lastActive.Add(cfg.StaleRevisionTimeout).Before(curTime) + ret := lastActive.Add(cfg.GCRetainSinceLastActiveTime).Before(curTime) if ret { logger.Infof("Detected stale revision %v with creation time %v and last active time %v.", rev.ObjectMeta.Name, rev.ObjectMeta.CreationTimestamp, lastActive) diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index 9a77c167722f..cbd62195b444 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -212,9 +212,9 @@ func TestCollect(t *testing.T) { cfgMap := &config.Config{ RevisionGC: &gcconfig.Config{ - StaleRevisionCreateDelay: 5 * time.Minute, - StaleRevisionTimeout: 5 * time.Minute, - StaleRevisionMinimumGenerations: 2, + GCRetainSinceCreateTime: 5 * time.Minute, + GCRetainSinceLastActiveTime: 5 * time.Minute, + GCMinStaleRevisions: 2, }, } @@ -348,20 +348,15 @@ func TestIsRevisionStale(t *testing.T) { want: false, }} - cfgStore := testConfigStore{ - config: &config.Config{ - RevisionGC: &gcconfig.Config{ - StaleRevisionCreateDelay: 5 * time.Minute, - StaleRevisionTimeout: 5 * time.Minute, - StaleRevisionMinimumGenerations: 2, - }, - }, + cfg := &gcconfig.Config{ + GCRetainSinceCreateTime: 5 * time.Minute, + GCRetainSinceLastActiveTime: 5 * time.Minute, + GCMinStaleRevisions: 2, } - ctx := cfgStore.ToContext(context.Background()) for _, test := range tests { t.Run(test.name, func(t *testing.T) { - cfg := &v1.Configuration{ + config := &v1.Configuration{ Status: v1.ConfigurationStatus{ ConfigurationStatusFields: v1.ConfigurationStatusFields{ LatestReadyRevisionName: test.latestRev, @@ -369,7 +364,7 @@ func TestIsRevisionStale(t *testing.T) { }, } - got := isRevisionStale(ctx, test.rev, cfg) + got := isRevisionStale(context.Background(), cfg, test.rev, config) if got != test.want { t.Errorf("IsRevisionStale want %v got %v", test.want, got) From 0a65256ccebacf6a588e650e69293e425b432555 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 13 Jul 2020 20:58:25 -0700 Subject: [PATCH 04/37] actually use the new settings --- pkg/reconciler/gc/v2/gc.go | 22 +++++++++++++++++----- pkg/reconciler/gc/v2/gc_test.go | 13 ------------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index cf4df91e7ae5..50880f01e046 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -49,9 +49,8 @@ func Collect( return err } - gcSkipOffset := cfg.GCMinStaleRevisions - - if gcSkipOffset >= int64(len(revs)) { + minStale, maxStale := int(cfg.GCMinStaleRevisions), int(cfg.GCMaxStaleRevisions) + if l := len(revs); l <= minStale || l <= maxStale { return nil } @@ -61,8 +60,17 @@ func Collect( return a.After(b) }) - for _, rev := range revs[gcSkipOffset:] { + numStale := 0 + for total, rev := range revs { + if isRevisionActive(rev, config) { + continue + } + if isRevisionStale(ctx, cfg, rev, config) { + numStale++ + } + + if total > maxStale || numStale > minStale { err := client.ServingV1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}) if err != nil { logger.With(zap.Error(err)).Errorf("Failed to delete stale revision %q", rev.Name) @@ -73,10 +81,14 @@ func Collect( return nil } -func isRevisionStale(ctx context.Context, cfg *gc.Config, rev *v1.Revision, config *v1.Configuration) bool { +func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool { if config.Status.LatestReadyRevisionName == rev.Name { return false } + return rev.GetRoutingState() != v1.RoutingStateReserve +} + +func isRevisionStale(ctx context.Context, cfg *gc.Config, rev *v1.Revision, config *v1.Configuration) bool { logger := logging.FromContext(ctx) curTime := time.Now() createTime := rev.ObjectMeta.CreationTimestamp diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index cbd62195b444..e05e1e839957 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -333,19 +333,6 @@ func TestIsRevisionStale(t *testing.T) { }, }, want: false, - }, { - name: "stale latest ready revision", - rev: &v1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - CreationTimestamp: metav1.NewTime(staleTime), - Annotations: map[string]string{ - "serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()), - }, - }, - }, - latestRev: "myrev", - want: false, }} cfg := &gcconfig.Config{ From aab20cfceaa840d70f457cc9e39469f51d099e71 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 13 Jul 2020 21:25:31 -0700 Subject: [PATCH 05/37] simplify --- pkg/reconciler/gc/v2/gc.go | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 50880f01e046..65acb0926927 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -66,14 +66,17 @@ func Collect( continue } - if isRevisionStale(ctx, cfg, rev, config) { + if isRevisionStale(cfg, rev, logger) { numStale++ } if total > maxStale || numStale > minStale { - err := client.ServingV1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}) + err := client.ServingV1().Revisions(rev.Namespace).Delete( + rev.Name, &metav1.DeleteOptions{}) if err != nil { - logger.With(zap.Error(err)).Errorf("Failed to delete stale revision %q", rev.Name) + logger.With( + zap.Error(err)).Errorf( + "Failed to delete stale revision %q", rev.Name) continue } } @@ -88,32 +91,20 @@ func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool { return rev.GetRoutingState() != v1.RoutingStateReserve } -func isRevisionStale(ctx context.Context, cfg *gc.Config, rev *v1.Revision, config *v1.Configuration) bool { - logger := logging.FromContext(ctx) +func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger) bool { curTime := time.Now() createTime := rev.ObjectMeta.CreationTimestamp - if createTime.Add(cfg.GCRetainSinceCreateTime).After(curTime) { // Revision was created sooner than GCRetainSinceCreateTime. Ignore it. return false } - lastActive := revisionLastActiveTime(rev) - - // TODO(whaught): this is carried over from v1, but I'm not sure why we can't delete a ready revision - // that isn't referenced? Maybe because of labeler failure - can we replace this with 'pending' routing state check? - if lastActive.Equal(createTime.Time) { - // Revision was never active and it's not ready after GCRetainSinceCreateTime. - // It usually happens when ksvc was deployed with wrong configuration. - return !rev.Status.GetCondition(v1.RevisionConditionReady).IsTrue() - } - - ret := lastActive.Add(cfg.GCRetainSinceLastActiveTime).Before(curTime) - if ret { + if a := revisionLastActiveTime(rev); a.Add(cfg.GCRetainSinceLastActiveTime).Before(curTime) { logger.Infof("Detected stale revision %v with creation time %v and last active time %v.", - rev.ObjectMeta.Name, rev.ObjectMeta.CreationTimestamp, lastActive) + rev.ObjectMeta.Name, rev.ObjectMeta.CreationTimestamp, a) + return true } - return ret + return false } // revisionLastActiveTime returns if present: From 2a7afbbe87b5a58c59720f756f8b568fcfad0e6b Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 13 Jul 2020 21:32:30 -0700 Subject: [PATCH 06/37] now it compiles --- pkg/reconciler/gc/v2/gc_test.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index e05e1e839957..72fc467cb2c0 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -39,6 +39,7 @@ import ( _ "knative.dev/serving/pkg/client/injection/informers/serving/v1/configuration/fake" _ "knative.dev/serving/pkg/client/injection/informers/serving/v1/revision/fake" + . "knative.dev/pkg/logging/testing" . "knative.dev/pkg/reconciler/testing" . "knative.dev/serving/pkg/testing/v1" ) @@ -343,15 +344,7 @@ func TestIsRevisionStale(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - config := &v1.Configuration{ - Status: v1.ConfigurationStatus{ - ConfigurationStatusFields: v1.ConfigurationStatusFields{ - LatestReadyRevisionName: test.latestRev, - }, - }, - } - - got := isRevisionStale(context.Background(), cfg, test.rev, config) + got := isRevisionStale(cfg, test.rev, TestLogger(t)) if got != test.want { t.Errorf("IsRevisionStale want %v got %v", test.want, got) From 16d6668edb1ab4de73deb175bf63ad33ec0ec88e Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Tue, 14 Jul 2020 13:54:05 -0700 Subject: [PATCH 07/37] fix config references --- pkg/reconciler/gc/v2/gc.go | 6 +++--- pkg/reconciler/gc/v2/gc_test.go | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 65acb0926927..420714929a9c 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -49,7 +49,7 @@ func Collect( return err } - minStale, maxStale := int(cfg.GCMinStaleRevisions), int(cfg.GCMaxStaleRevisions) + minStale, maxStale := int(cfg.MinStaleRevisions), int(cfg.MaxStaleRevisions) if l := len(revs); l <= minStale || l <= maxStale { return nil } @@ -94,12 +94,12 @@ func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool { func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger) bool { curTime := time.Now() createTime := rev.ObjectMeta.CreationTimestamp - if createTime.Add(cfg.GCRetainSinceCreateTime).After(curTime) { + if createTime.Add(cfg.RetainSinceCreateTime).After(curTime) { // Revision was created sooner than GCRetainSinceCreateTime. Ignore it. return false } - if a := revisionLastActiveTime(rev); a.Add(cfg.GCRetainSinceLastActiveTime).Before(curTime) { + if a := revisionLastActiveTime(rev); a.Add(cfg.RetainSinceLastActiveTime).Before(curTime) { logger.Infof("Detected stale revision %v with creation time %v and last active time %v.", rev.ObjectMeta.Name, rev.ObjectMeta.CreationTimestamp, a) return true diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index 72fc467cb2c0..113fba349473 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -213,9 +213,9 @@ func TestCollect(t *testing.T) { cfgMap := &config.Config{ RevisionGC: &gcconfig.Config{ - GCRetainSinceCreateTime: 5 * time.Minute, - GCRetainSinceLastActiveTime: 5 * time.Minute, - GCMinStaleRevisions: 2, + RetainSinceCreateTime: 5 * time.Minute, + RetainSinceLastActiveTime: 5 * time.Minute, + MinStaleRevisions: 2, }, } @@ -337,9 +337,9 @@ func TestIsRevisionStale(t *testing.T) { }} cfg := &gcconfig.Config{ - GCRetainSinceCreateTime: 5 * time.Minute, - GCRetainSinceLastActiveTime: 5 * time.Minute, - GCMinStaleRevisions: 2, + RetainSinceCreateTime: 5 * time.Minute, + RetainSinceLastActiveTime: 5 * time.Minute, + MinStaleRevisions: 2, } for _, test := range tests { From 562512ab63067d57b9e2b7e8dea4e0471865fd2d Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Tue, 14 Jul 2020 19:11:42 -0700 Subject: [PATCH 08/37] test with min settings --- pkg/reconciler/gc/v2/gc.go | 10 +- pkg/reconciler/gc/v2/gc_test.go | 192 +++++++++++++++----------------- 2 files changed, 94 insertions(+), 108 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 420714929a9c..b67f35e11975 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -70,7 +70,7 @@ func Collect( numStale++ } - if total > maxStale || numStale > minStale { + if numStale > minStale || maxStale != -1 && total >= maxStale { err := client.ServingV1().Revisions(rev.Namespace).Delete( rev.Name, &metav1.DeleteOptions{}) if err != nil { @@ -86,8 +86,10 @@ func Collect( func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool { if config.Status.LatestReadyRevisionName == rev.Name { - return false + return true // never delete latest ready, even if config is not active. } + // Anything that the labeler hasn't explicitly labelled as inactive. + // Revisions which do not yet have any annotation are not eligible for deletion. return rev.GetRoutingState() != v1.RoutingStateReserve } @@ -99,9 +101,9 @@ func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger return false } - if a := revisionLastActiveTime(rev); a.Add(cfg.RetainSinceLastActiveTime).Before(curTime) { + if active := revisionLastActiveTime(rev); active.Add(cfg.RetainSinceLastActiveTime).Before(curTime) { logger.Infof("Detected stale revision %v with creation time %v and last active time %v.", - rev.ObjectMeta.Name, rev.ObjectMeta.CreationTimestamp, a) + rev.ObjectMeta.Name, createTime, active) return true } return false diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index 113fba349473..edc9e481a56d 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -53,10 +53,19 @@ var revisionSpec = v1.RevisionSpec{ TimeoutSeconds: ptr.Int64(60), } -func TestCollect(t *testing.T) { +func TestCollectMin(t *testing.T) { + cfgMap := &config.Config{ + RevisionGC: &gcconfig.Config{ + RetainSinceCreateTime: 5 * time.Minute, + RetainSinceLastActiveTime: 5 * time.Minute, + MinStaleRevisions: 1, + MaxStaleRevisions: -1, // assert no changes to min case + }, + } + now := time.Now() - nineMinutesAgo := now.Add(-9 * time.Minute) - tenMinutesAgo := now.Add(-10 * time.Minute) + //nineMinutesAgo := now.Add(-9 * time.Minute) + //tenMinutesAgo := now.Add(-10 * time.Minute) old := now.Add(-11 * time.Minute) older := now.Add(-12 * time.Minute) @@ -68,24 +77,27 @@ func TestCollect(t *testing.T) { revs []*v1.Revision wantDeletes []clientgotesting.DeleteActionImpl }{{ - name: "delete oldest, keep two lastPinned", + name: "delete oldest, keep one recent, one active", cfg: cfg("keep-two", "foo", 5556, WithLatestCreated("5556"), WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ + // Stale, oldest should be deleted rev("keep-two", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithCreationTimestamp(oldest), - WithLastPinned(oldest)), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(oldest)), + // Stale, but MinStaleRevisions is 1 rev("keep-two", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(older)), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Actively referenced by Configuration rev("keep-two", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(old)), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), }, wantDeletes: []clientgotesting.DeleteActionImpl{{ ActionImpl: clientgotesting.ActionImpl{ @@ -100,23 +112,23 @@ func TestCollect(t *testing.T) { Name: "5554", }}, }, { - name: "delete oldest, keep two routingStateModified", - cfg: cfg("keep-two", "foo", 5556, - WithLatestCreated("5556"), - WithLatestReady("5556"), - WithConfigObservedGen), + name: "no latest ready, one active", + cfg: cfg("keep-two", "foo", 5556, WithConfigObservedGen), revs: []*v1.Revision{ + // Stale, oldest should be deleted rev("keep-two", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithCreationTimestamp(oldest), + WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(oldest)), + // Stale, but MinStaleRevisions is 1 rev("keep-two", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), + WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(older)), + // Actively referenced by Configuration rev("keep-two", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), + WithRoutingState(v1.RoutingStateActive), WithRoutingStateModified(old)), }, wantDeletes: []clientgotesting.DeleteActionImpl{{ @@ -132,7 +144,7 @@ func TestCollect(t *testing.T) { Name: "5554", }}, }, { - name: "keep oldest when no lastPinned", + name: "keep oldest when none Reserved", cfg: cfg("keep-no-last-pinned", "foo", 5556, WithLatestCreated("5556"), WithLatestReady("5556"), @@ -141,84 +153,37 @@ func TestCollect(t *testing.T) { // No lastPinned so we will keep this. rev("keep-no-last-pinned", "foo", 5554, MarkRevisionReady, WithRevName("5554"), + WithRoutingState(v1.RoutingStatePending), WithCreationTimestamp(oldest)), rev("keep-no-last-pinned", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(tenMinutesAgo)), + WithRoutingState(v1.RoutingStateUnset), + WithCreationTimestamp(older)), rev("keep-no-last-pinned", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(tenMinutesAgo)), + WithRoutingState(v1.RoutingStateActive), + WithCreationTimestamp(old)), }, }, { - name: "keep recent lastPinned", - cfg: cfg("keep-recent-last-pinned", "foo", 5556, - WithLatestCreated("5556"), - WithLatestReady("5556"), - WithConfigObservedGen), + name: "none stale", + cfg: cfg("keep-no-last-pinned", "foo", 5556, WithConfigObservedGen), revs: []*v1.Revision{ - rev("keep-recent-last-pinned", "foo", 5554, MarkRevisionReady, - WithRevName("5554"), - WithCreationTimestamp(oldest), - // This is an indication that things are still routing here. - WithLastPinned(now)), - rev("keep-recent-last-pinned", "foo", 5555, MarkRevisionReady, - WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(nineMinutesAgo)), - rev("keep-recent-last-pinned", "foo", 5556, MarkRevisionReady, - WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(tenMinutesAgo)), - }, - }, { - name: "keep LatestReadyRevision", - cfg: cfg("keep-two", "foo", 5556, - WithLatestReady("5554"), - // This comes after 'WithLatestReady' so the - // Configuration's 'Ready' Status is 'Unknown' - WithLatestCreated("5556"), - WithConfigObservedGen), - revs: []*v1.Revision{ - // Create a revision where the LatestReady is 5554, but LatestCreated is 5556. - // We should keep LatestReady even if it is old. - rev("keep-two", "foo", 5554, MarkRevisionReady, + // No lastPinned so we will keep this. + rev("keep-no-last-pinned", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithCreationTimestamp(oldest), - WithLastPinned(oldest)), - rev("keep-two", "foo", 5555, // Not Ready + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(now)), + rev("keep-no-last-pinned", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(older)), - rev("keep-two", "foo", 5556, // Not Ready + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(now)), + rev("keep-no-last-pinned", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(old)), - }, - }, { - name: "keep stale revision because of minimum generations", - cfg: cfg("keep-all", "foo", 5554, - // Don't set the latest ready revision here - // since those by default are always retained - WithLatestCreated("keep-all"), - WithConfigObservedGen), - revs: []*v1.Revision{ - rev("keep-all", "foo", 5554, - WithRevName("keep-all"), - WithCreationTimestamp(oldest), - WithLastPinned(tenMinutesAgo)), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(now)), }, }} - cfgMap := &config.Config{ - RevisionGC: &gcconfig.Config{ - RetainSinceCreateTime: 5 * time.Minute, - RetainSinceLastActiveTime: 5 * time.Minute, - MinStaleRevisions: 2, - }, - } - for _, test := range table { t.Run(test.name, func(t *testing.T) { ctx, _ := SetupFakeContext(t) @@ -268,16 +233,7 @@ func TestIsRevisionStale(t *testing.T) { latestRev string want bool }{{ - name: "fresh revision that was never pinned", - rev: &v1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - CreationTimestamp: metav1.NewTime(curTime), - }, - }, - want: false, - }, { - name: "stale revision that was never pinned w/ Ready status", + name: "stale create time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "myrev", @@ -287,18 +243,18 @@ func TestIsRevisionStale(t *testing.T) { Status: duckv1.Status{ Conditions: duckv1.Conditions{{ Type: v1.RevisionConditionReady, - Status: "True", + Status: "Unknown", }}, }, }, }, - want: false, + want: true, }, { - name: "stale revision that was never pinned w/o Ready status", + name: "fresh create time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "myrev", - CreationTimestamp: metav1.NewTime(staleTime), + CreationTimestamp: metav1.NewTime(curTime), }, Status: v1.RevisionStatus{ Status: duckv1.Status{ @@ -309,12 +265,13 @@ func TestIsRevisionStale(t *testing.T) { }, }, }, - want: true, + want: false, }, { - name: "stale revision that was previously pinned", + name: "stale pinned time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", + Name: "myrev", + // We technically don't expect create time to ever be ahead of pinned CreationTimestamp: metav1.NewTime(staleTime), Annotations: map[string]string{ "serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()), @@ -323,10 +280,11 @@ func TestIsRevisionStale(t *testing.T) { }, want: true, }, { - name: "fresh revision that was previously pinned", + name: "fresh pinned time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", + Name: "myrev", + // We technically don't expect create time to ever be ahead of pinned CreationTimestamp: metav1.NewTime(staleTime), Annotations: map[string]string{ "serving.knative.dev/lastPinned": fmt.Sprintf("%d", curTime.Unix()), @@ -334,6 +292,32 @@ func TestIsRevisionStale(t *testing.T) { }, }, want: false, + }, { + name: "stale revisionStateModified", + rev: &v1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrev", + // We technically don't expect create time to ever be ahead of pinned + CreationTimestamp: metav1.NewTime(staleTime), + Annotations: map[string]string{ + "serving.knative.dev/routingStateModified": staleTime.UTC().Format(time.RFC3339), + }, + }, + }, + want: true, + }, { + name: "fresh revisionStateModified", + rev: &v1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrev", + // We technically don't expect create time to ever be ahead of pinned + CreationTimestamp: metav1.NewTime(staleTime), + Annotations: map[string]string{ + "serving.knative.dev/routingStateModified": curTime.UTC().Format(time.RFC3339), + }, + }, + }, + want: false, }} cfg := &gcconfig.Config{ @@ -373,8 +357,8 @@ func cfg(name, namespace string, generation int64, co ...ConfigOption) *v1.Confi return c } -func rev(name, namespace string, generation int64, ro ...RevisionOption) *v1.Revision { - config := cfg(name, namespace, generation) +func rev(configName, namespace string, generation int64, ro ...RevisionOption) *v1.Revision { + config := cfg(configName, namespace, generation) rev := resources.MakeRevision(config) rev.SetDefaults(context.Background()) From 639a4fc6c126ad45d1f3d351e0dc24668fbe63d1 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Tue, 14 Jul 2020 20:40:58 -0700 Subject: [PATCH 09/37] max tests --- pkg/gc/config.go | 4 + pkg/reconciler/gc/v2/gc_test.go | 155 +++++++++++++++++++++++++++++++- 2 files changed, 156 insertions(+), 3 deletions(-) diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 5504f16ef201..23f9e3d6a398 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -68,6 +68,10 @@ func defaultConfig() *Config { StaleRevisionMinimumGenerations: 20, // V2 GC Settings + // TODO(whaught): consider an infinity sentinel value for use with max mode. + // Document in example why you'd want that to fill to max if you want to do it by count. + // Max stale is really just max? + // TODO(whaught): validate positive RetainSinceCreateTime: 48 * time.Hour, RetainSinceLastActiveTime: 15 * time.Hour, MinStaleRevisions: 20, diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index edc9e481a56d..19948ca53d7f 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -64,9 +64,6 @@ func TestCollectMin(t *testing.T) { } now := time.Now() - //nineMinutesAgo := now.Add(-9 * time.Minute) - //tenMinutesAgo := now.Add(-10 * time.Minute) - old := now.Add(-11 * time.Minute) older := now.Add(-12 * time.Minute) oldest := now.Add(-13 * time.Minute) @@ -223,6 +220,158 @@ func TestCollectMin(t *testing.T) { } } +func TestCollectMax(t *testing.T) { + cfgMap := &config.Config{ + RevisionGC: &gcconfig.Config{ + RetainSinceCreateTime: 1 * time.Hour, + RetainSinceLastActiveTime: 1 * time.Hour, + MinStaleRevisions: 1, + MaxStaleRevisions: 3, + }, + } + + now := time.Now() + old := now.Add(-11 * time.Minute) + older := now.Add(-12 * time.Minute) + oldest := now.Add(-13 * time.Minute) + + table := []struct { + name string + cfg *v1.Configuration + revs []*v1.Revision + wantDeletes []clientgotesting.DeleteActionImpl + }{{ + name: "at max", + cfg: cfg("keep-two", "foo", 5556, + WithLatestCreated("5556"), + WithLatestReady("5556"), + WithConfigObservedGen), + revs: []*v1.Revision{ + // Under max + rev("keep-two", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Under max + rev("keep-two", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Actively referenced by Configuration + rev("keep-two", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), + }, + }, { + name: "delete oldest, keep three max", + cfg: cfg("keep-two", "foo", 5556, + WithLatestCreated("5556"), + WithLatestReady("5556"), + WithConfigObservedGen), + revs: []*v1.Revision{ + // Stale and over the max + rev("keep-two", "foo", 5554, MarkRevisionReady, + WithRevName("5553"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(oldest)), + // Stale but under max + rev("keep-two", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Stale but under max + rev("keep-two", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Actively referenced by Configuration + rev("keep-two", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), + }, + wantDeletes: []clientgotesting.DeleteActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: "foo", + Verb: "delete", + Resource: schema.GroupVersionResource{ + Group: "serving.knative.dev", + Version: "v1", + Resource: "revisions", + }, + }, + Name: "5553", + }}, + }, { + name: "over max, all active", + cfg: cfg("keep-two", "foo", 5556, + WithLatestCreated("5556"), + WithLatestReady("5556"), + WithConfigObservedGen), + revs: []*v1.Revision{ + // Stale and over the max + rev("keep-two", "foo", 5554, MarkRevisionReady, + WithRevName("5553"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(oldest)), + // Stale but under max + rev("keep-two", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(older)), + // Stale but under max + rev("keep-two", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(older)), + // Actively referenced by Configuration + rev("keep-two", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), + }, + }} + + for _, test := range table { + t.Run(test.name, func(t *testing.T) { + ctx, _ := SetupFakeContext(t) + ctx = config.ToContext(ctx, cfgMap) + client := fakeservingclient.Get(ctx) + + ri := fakerevisioninformer.Get(ctx) + for _, rev := range test.revs { + ri.Informer().GetIndexer().Add(rev) + } + + recorderList := ActionRecorderList{client} + + Collect(ctx, client, ri.Lister(), test.cfg) + + actions, err := recorderList.ActionsByVerb() + if err != nil { + t.Errorf("Error capturing actions by verb: %q", err) + } + + for i, want := range test.wantDeletes { + if i >= len(actions.Deletes) { + t.Errorf("Missing delete: %#v", want) + continue + } + got := actions.Deletes[i] + if got.GetName() != want.GetName() { + t.Errorf("Unexpected delete[%d]: %#v", i, got) + } + } + if got, want := len(actions.Deletes), len(test.wantDeletes); got > want { + for _, extra := range actions.Deletes[want:] { + t.Errorf("Extra delete: %s/%s", extra.GetNamespace(), extra.GetName()) + } + } + }) + } +} + func TestIsRevisionStale(t *testing.T) { curTime := time.Now() staleTime := curTime.Add(-10 * time.Minute) From 668dd00057a08b545742fae5b5fd364f9da23c63 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Tue, 14 Jul 2020 20:59:23 -0700 Subject: [PATCH 10/37] change name of max to max-non-active --- config/core/configmaps/gc.yaml | 2 +- pkg/gc/config.go | 23 ++++++++++------------- pkg/gc/config_test.go | 12 ++++++------ pkg/reconciler/gc/v2/gc.go | 9 +++++---- pkg/reconciler/gc/v2/gc_test.go | 4 ++-- 5 files changed, 24 insertions(+), 26 deletions(-) diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index 2014659f7214..9ec0aceaf956 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -83,4 +83,4 @@ data: # # Set to -1 to disable this setting. Revisions will be eligible for gc as soon # as they are considered stale as long as there are more than min-stale-revisions. - max-stale-revisions: "-1" + max-non-active-revisions: "-1" diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 23f9e3d6a398..b72f78563ec3 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -48,15 +48,12 @@ type Config struct { // Duration from last active when a Revision should be considered active // and exempt from GC.Note that GCMaxStaleRevision may override this if set. RetainSinceLastActiveTime time.Duration - // Minimum number of generations of revisions to keep before considering for GC. - // Set -1 to disable minimum and fill up to max. - // Either min or max must be set. + // Minimum number of stale revisions to keep before considering for GC. MinStaleRevisions int64 - // Maximum number of stale revisions to keep before considering for GC. + // Maximum number of non-active revisions to keep before considering for GC. // regardless of creation or staleness time-bounds // Set -1 to disable this setting. - // Either min or max must be set. - MaxStaleRevisions int64 + MaxNonActiveRevisions int64 } func defaultConfig() *Config { @@ -70,12 +67,12 @@ func defaultConfig() *Config { // V2 GC Settings // TODO(whaught): consider an infinity sentinel value for use with max mode. // Document in example why you'd want that to fill to max if you want to do it by count. - // Max stale is really just max? + // Max stale is really max non-active? Staleness is time-defined. // TODO(whaught): validate positive RetainSinceCreateTime: 48 * time.Hour, RetainSinceLastActiveTime: 15 * time.Hour, MinStaleRevisions: 20, - MaxStaleRevisions: -1, + MaxNonActiveRevisions: 1000, } } @@ -94,21 +91,21 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf cm.AsDuration("retain-since-create-time", &c.RetainSinceCreateTime), cm.AsDuration("retain-since-last-active-time", &c.RetainSinceLastActiveTime), cm.AsInt64("min-stale-revisions", &c.MinStaleRevisions), - cm.AsInt64("max-stale-revisions", &c.MaxStaleRevisions), + cm.AsInt64("max-non-active-revisions", &c.MaxNonActiveRevisions), ); err != nil { return nil, fmt.Errorf("failed to parse data: %w", err) } - if c.MaxStaleRevisions >= 0 && c.MinStaleRevisions > c.MaxStaleRevisions { + if c.MaxNonActiveRevisions >= 0 && c.MinStaleRevisions > c.MaxNonActiveRevisions { return nil, fmt.Errorf( "min-stale-revisions(%d) must be <= max-stale-revisions(%d)", - c.MinStaleRevisions, c.MaxStaleRevisions) + c.MinStaleRevisions, c.MaxNonActiveRevisions) } if c.MinStaleRevisions < 0 { return nil, fmt.Errorf("min-stale-revisions must be non-negative, was: %d", c.MinStaleRevisions) } - if c.MaxStaleRevisions < -1 { - return nil, fmt.Errorf("max-stale-revisions must be >= -1, was: %d", c.MaxStaleRevisions) + if c.MaxNonActiveRevisions < -1 { + return nil, fmt.Errorf("max-stale-revisions must be >= -1, was: %d", c.MaxNonActiveRevisions) } if c.StaleRevisionMinimumGenerations < 0 { diff --git a/pkg/gc/config_test.go b/pkg/gc/config_test.go index 46a0f460ea0e..f21ba7299ef2 100644 --- a/pkg/gc/config_test.go +++ b/pkg/gc/config_test.go @@ -54,7 +54,7 @@ func TestOurConfig(t *testing.T) { RetainSinceCreateTime: 17 * time.Hour, RetainSinceLastActiveTime: 16 * time.Hour, MinStaleRevisions: 5, - MaxStaleRevisions: 500, + MaxNonActiveRevisions: 500, }, data: map[string]string{ "stale-revision-create-delay": "15h", @@ -64,7 +64,7 @@ func TestOurConfig(t *testing.T) { "retain-since-create-time": "17h", "retain-since-last-active-time": "16h", "min-stale-revisions": "5", - "max-stale-revisions": "500", + "max-non-active-revisions": "500", }, }, { name: "Invalid duration", @@ -92,15 +92,15 @@ func TestOurConfig(t *testing.T) { fail: true, want: nil, data: map[string]string{ - "max-stale-revisions": "-2", + "max-non-active-revisions": "-2", }, }, { name: "Invalid max less than min", fail: true, want: nil, data: map[string]string{ - "min-stale-revisions": "20", - "max-stale-revisions": "10", + "min-stale-revisions": "20", + "max-non-active-revisions": "10", }, }, { name: "Invalid minimum generation", @@ -120,7 +120,7 @@ func TestOurConfig(t *testing.T) { RetainSinceCreateTime: 48 * time.Hour, RetainSinceLastActiveTime: 15 * time.Hour, MinStaleRevisions: 20, - MaxStaleRevisions: -1, + MaxNonActiveRevisions: 1000, }, data: map[string]string{ "stale-revision-create-delay": "15h", diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index b67f35e11975..f1d525cf7713 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -49,7 +49,7 @@ func Collect( return err } - minStale, maxStale := int(cfg.MinStaleRevisions), int(cfg.MaxStaleRevisions) + minStale, maxStale := int(cfg.MinStaleRevisions), int(cfg.MaxNonActiveRevisions) if l := len(revs); l <= minStale || l <= maxStale { return nil } @@ -60,17 +60,18 @@ func Collect( return a.After(b) }) - numStale := 0 - for total, rev := range revs { + numStale, nonactive := 0, 0 + for _, rev := range revs { if isRevisionActive(rev, config) { continue } + nonactive++ if isRevisionStale(cfg, rev, logger) { numStale++ } - if numStale > minStale || maxStale != -1 && total >= maxStale { + if numStale > minStale || maxStale != -1 && nonactive >= maxStale { err := client.ServingV1().Revisions(rev.Namespace).Delete( rev.Name, &metav1.DeleteOptions{}) if err != nil { diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index 19948ca53d7f..ea21cf2ba1c3 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -59,7 +59,7 @@ func TestCollectMin(t *testing.T) { RetainSinceCreateTime: 5 * time.Minute, RetainSinceLastActiveTime: 5 * time.Minute, MinStaleRevisions: 1, - MaxStaleRevisions: -1, // assert no changes to min case + MaxNonActiveRevisions: -1, // assert no changes to min case }, } @@ -226,7 +226,7 @@ func TestCollectMax(t *testing.T) { RetainSinceCreateTime: 1 * time.Hour, RetainSinceLastActiveTime: 1 * time.Hour, MinStaleRevisions: 1, - MaxStaleRevisions: 3, + MaxNonActiveRevisions: 3, }, } From 80d7b90240b00460e82c2659d0b12749e64d7a1f Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Tue, 14 Jul 2020 21:19:54 -0700 Subject: [PATCH 11/37] comments and validate positive --- config/core/configmaps/gc.yaml | 24 +++++++++++++++--------- pkg/gc/config.go | 34 +++++++++++++++++++++------------- 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index 9ec0aceaf956..b08f8d3bfcc8 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -64,23 +64,29 @@ data: # ALPHA NOTE: This feature is still experimental and under active development. # Duration since revision creation before considering it for GC. + # + # Accepts special value 'forever' if max-non-active-revisions is set. retain-since-create-time: "48h" - # Duration since a route has pointed at the revision - # before considering it for GC. + # Duration since a route has pointed at the revision before considering it for GC. + # + # Accepts special value 'forever' if max-non-active-revisions is set. retain-since-last-active-time: "15h" - # Minimum number of non-active revisions to retain. + # Minimum number of stale revisions to retain. + # + # A stale revision is one whose creation time is past retain-since-create-time + # or was actively referenced by a route beyond retain-since-last-active-time min-stale-revisions: "20" # Maximum number of non-active revisions to retain regardless of # creation or staleness time-bounds. # - # Instead of allowing revisions to become stale down to min-stale-revisions, - # this mode will keep around revisions up to this maximum limit. A value of 0 - # will immediately delete any non-active revisions. The expected total number - # of revisions is the number of active plus this value. - # + # A value of 0 will immediately delete any non-active revisions. # Set to -1 to disable this setting. Revisions will be eligible for gc as soon # as they are considered stale as long as there are more than min-stale-revisions. - max-non-active-revisions: "-1" + # To allow revisions to simply grow up to the max and be kept, set staleness timeouts + # to 'forever'. + # + # Setting 'forever' retention with no maximum disables garbage collection entirely. + max-non-active-revisions: "1000" diff --git a/pkg/gc/config.go b/pkg/gc/config.go index b72f78563ec3..dfe95e23d609 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -29,6 +29,7 @@ import ( const ( ConfigName = "config-gc" + Forever = time.Duration(-1) ) type Config struct { @@ -65,9 +66,7 @@ func defaultConfig() *Config { StaleRevisionMinimumGenerations: 20, // V2 GC Settings - // TODO(whaught): consider an infinity sentinel value for use with max mode. - // Document in example why you'd want that to fill to max if you want to do it by count. - // Max stale is really max non-active? Staleness is time-defined. + // TODO(whaught): consider 'forever' sentinel value for use with max mode. // TODO(whaught): validate positive RetainSinceCreateTime: 48 * time.Hour, RetainSinceLastActiveTime: 15 * time.Hour, @@ -88,6 +87,7 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf cm.AsDuration("stale-revision-lastpinned-debounce", &c.StaleRevisionLastpinnedDebounce), cm.AsInt64("stale-revision-minimum-generations", &c.StaleRevisionMinimumGenerations), + // v2 settings cm.AsDuration("retain-since-create-time", &c.RetainSinceCreateTime), cm.AsDuration("retain-since-last-active-time", &c.RetainSinceLastActiveTime), cm.AsInt64("min-stale-revisions", &c.MinStaleRevisions), @@ -96,6 +96,24 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf return nil, fmt.Errorf("failed to parse data: %w", err) } + if c.StaleRevisionMinimumGenerations < 0 { + return nil, fmt.Errorf("stale-revision-minimum-generations must be non-negative, was: %d", c.StaleRevisionMinimumGenerations) + } + if c.StaleRevisionTimeout-c.StaleRevisionLastpinnedDebounce < minRevisionTimeout { + logger.Warnf("Got revision timeout of %v, minimum supported value is %v", + c.StaleRevisionTimeout, minRevisionTimeout+c.StaleRevisionLastpinnedDebounce) + c.StaleRevisionTimeout = minRevisionTimeout + c.StaleRevisionLastpinnedDebounce + } + + // validate V2 settings + if c.RetainSinceCreateTime < 0 { + return nil, fmt.Errorf( + "min-stale-revisions(%d) must be non-negative", c.RetainSinceCreateTime) + } + if c.RetainSinceLastActiveTime < 0 { + return nil, fmt.Errorf( + "retain-since-last-active-time(%d) must be non-negative", c.RetainSinceLastActiveTime) + } if c.MaxNonActiveRevisions >= 0 && c.MinStaleRevisions > c.MaxNonActiveRevisions { return nil, fmt.Errorf( "min-stale-revisions(%d) must be <= max-stale-revisions(%d)", @@ -107,16 +125,6 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf if c.MaxNonActiveRevisions < -1 { return nil, fmt.Errorf("max-stale-revisions must be >= -1, was: %d", c.MaxNonActiveRevisions) } - - if c.StaleRevisionMinimumGenerations < 0 { - return nil, fmt.Errorf("stale-revision-minimum-generations must be non-negative, was: %d", c.StaleRevisionMinimumGenerations) - } - - if c.StaleRevisionTimeout-c.StaleRevisionLastpinnedDebounce < minRevisionTimeout { - logger.Warnf("Got revision timeout of %v, minimum supported value is %v", - c.StaleRevisionTimeout, minRevisionTimeout+c.StaleRevisionLastpinnedDebounce) - c.StaleRevisionTimeout = minRevisionTimeout + c.StaleRevisionLastpinnedDebounce - } return c, nil } } From ab2ea1cfd6e8c24fa392356018c5e4fc148a41ef Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Tue, 14 Jul 2020 22:36:03 -0700 Subject: [PATCH 12/37] parse forever constant --- config/core/configmaps/gc.yaml | 2 +- pkg/gc/config.go | 33 +++++++++++++++++++++++++-------- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index b08f8d3bfcc8..62bf27b2223c 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "c675b489" + knative.dev/example-checksum: "0baba3a7" data: _example: | ################################ diff --git a/pkg/gc/config.go b/pkg/gc/config.go index dfe95e23d609..8da18c72dc5c 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -19,6 +19,7 @@ package gc import ( "context" "fmt" + "strings" "time" corev1 "k8s.io/api/core/v1" @@ -30,6 +31,8 @@ import ( const ( ConfigName = "config-gc" Forever = time.Duration(-1) + + foreverString = "forever" ) type Config struct { @@ -81,6 +84,7 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf return func(configMap *corev1.ConfigMap) (*Config, error) { c := defaultConfig() + var retainCreate, retainActive string if err := cm.Parse(configMap.Data, cm.AsDuration("stale-revision-create-delay", &c.StaleRevisionCreateDelay), cm.AsDuration("stale-revision-timeout", &c.StaleRevisionTimeout), @@ -88,8 +92,8 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf cm.AsInt64("stale-revision-minimum-generations", &c.StaleRevisionMinimumGenerations), // v2 settings - cm.AsDuration("retain-since-create-time", &c.RetainSinceCreateTime), - cm.AsDuration("retain-since-last-active-time", &c.RetainSinceLastActiveTime), + cm.AsString("retain-since-create-time", &retainCreate), + cm.AsString("retain-since-last-active-time", &retainActive), cm.AsInt64("min-stale-revisions", &c.MinStaleRevisions), cm.AsInt64("max-non-active-revisions", &c.MaxNonActiveRevisions), ); err != nil { @@ -106,13 +110,11 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf } // validate V2 settings - if c.RetainSinceCreateTime < 0 { - return nil, fmt.Errorf( - "min-stale-revisions(%d) must be non-negative", c.RetainSinceCreateTime) + if err := parseForeverOrDuration(retainCreate, &c.RetainSinceCreateTime); err != nil { + return nil, fmt.Errorf("failed to parse min-stale-revisions: %w", err) } - if c.RetainSinceLastActiveTime < 0 { - return nil, fmt.Errorf( - "retain-since-last-active-time(%d) must be non-negative", c.RetainSinceLastActiveTime) + if err := parseForeverOrDuration(retainActive, &c.RetainSinceLastActiveTime); err != nil { + return nil, fmt.Errorf("failed to parse retain-since-last-active-time: %w", err) } if c.MaxNonActiveRevisions >= 0 && c.MinStaleRevisions > c.MaxNonActiveRevisions { return nil, fmt.Errorf( @@ -128,3 +130,18 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf return c, nil } } + +func parseForeverOrDuration(val string, toSet *time.Duration) error { + if val == "" { + // keep default value + } else if strings.EqualFold(val, foreverString) { + *toSet = Forever + } else if parsed, err := time.ParseDuration(val); err != nil { + return err + } else if parsed < 0 { + return fmt.Errorf("must be non-negative") + } else { + *toSet = parsed + } + return nil +} From 6066241c4ebec57c908fe560e8a0c18d26fdfaef Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Tue, 14 Jul 2020 23:33:38 -0700 Subject: [PATCH 13/37] fixing the unit test, better examples --- config/core/configmaps/gc.yaml | 55 +++++++++++++++++++++------------- pkg/gc/config_test.go | 47 +++++++++++++++++++++++++++-- 2 files changed, 78 insertions(+), 24 deletions(-) diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index 62bf27b2223c..b35bb843bb49 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "0baba3a7" + knative.dev/example-checksum: "a1cd0b95" data: _example: | ################################ @@ -60,33 +60,46 @@ data: # V2 Garbage Collector Settings # --------------------------------------- # - # These settings are enabled via the 'responsive-revision-gc' feature flag. + # These settings are enabled via the "responsive-revision-gc" feature flag. # ALPHA NOTE: This feature is still experimental and under active development. - - # Duration since revision creation before considering it for GC. # - # Accepts special value 'forever' if max-non-active-revisions is set. + # Staleness + # * Revisions are considered stale if they were created after more than + # "retain-since-create-time" or last referenced by a route more than + # "retain-since-last-active-time" ago. + # * Either timeout may be set to "forever" to turn off time-based GC. + # * Individual revisions may be marked with the annotation + # "knative.dev/no-gc":"true" to be permanently considered active. + # + # Example config to immediately collect any inactive revision: + # min-stale-revisions: "0" + # retain-since-create-time: "0" + # retain-since-last-active-time: "0" + # + # + # Example config to always keep around the last ten revisions: + # retain-since-create-time: "forever" + # max-non-active-revisions: "10" + # + # + # Example config to keep recently deployed or active revisions, + # always maintain the last two in case of rollback, and prevent + # burst activity from exploding the count of old revisions: + # retain-since-create-time: "48h" + # retain-since-last-active-time: "15h" + # min-stale-revisions: "2" + # max-non-active-revisions: "1000" + + + # Duration since creation before considering a revision for GC or "forever". retain-since-create-time: "48h" - # Duration since a route has pointed at the revision before considering it for GC. - # - # Accepts special value 'forever' if max-non-active-revisions is set. + # Duration since active before considering a revision for GC or "forever". retain-since-last-active-time: "15h" # Minimum number of stale revisions to retain. - # - # A stale revision is one whose creation time is past retain-since-create-time - # or was actively referenced by a route beyond retain-since-last-active-time min-stale-revisions: "20" - # Maximum number of non-active revisions to retain regardless of - # creation or staleness time-bounds. - # - # A value of 0 will immediately delete any non-active revisions. - # Set to -1 to disable this setting. Revisions will be eligible for gc as soon - # as they are considered stale as long as there are more than min-stale-revisions. - # To allow revisions to simply grow up to the max and be kept, set staleness timeouts - # to 'forever'. - # - # Setting 'forever' retention with no maximum disables garbage collection entirely. + # Maximum number of non-active revisions to retain. + # Set to -1 to disable any maximum limit. max-non-active-revisions: "1000" diff --git a/pkg/gc/config_test.go b/pkg/gc/config_test.go index f21ba7299ef2..b2492e6316dd 100644 --- a/pkg/gc/config_test.go +++ b/pkg/gc/config_test.go @@ -81,7 +81,14 @@ func TestOurConfig(t *testing.T) { "stale-revision-minimum-generations": "-1", }, }, { - name: "Invalid negative minimum generation v2", + name: "Invalid minimum generation", + fail: true, + want: nil, + data: map[string]string{ + "stale-revision-minimum-generations": "invalid", + }, + }, { + name: "Invalid negative min stale", fail: true, want: nil, data: map[string]string{ @@ -103,11 +110,45 @@ func TestOurConfig(t *testing.T) { "max-non-active-revisions": "10", }, }, { - name: "Invalid minimum generation", + name: "Unparsable create duration", fail: true, want: nil, data: map[string]string{ - "stale-revision-minimum-generations": "invalid", + "retain-since-create-time": "invalid", + }, + }, { + name: "Negative create duration", + fail: true, + want: nil, + data: map[string]string{ + "retain-since-create-time": "-1d", + }, + }, { + name: "Negative last-active duration", + fail: true, + want: nil, + data: map[string]string{ + "retain-since-last-active-time": "-1d", + }, + }, { + name: "create delay forever", + want: func() *Config { + d := defaultConfig() + d.RetainSinceCreateTime = Forever + return d + }(), + data: map[string]string{ + "retain-since-create-time": "forever", + }, + }, { + name: "last-active forever", + want: func() *Config { + d := defaultConfig() + d.RetainSinceLastActiveTime = Forever + return d + }(), + data: map[string]string{ + "retain-since-last-active-time": "forever", }, }, { name: "Below minimum timeout", From e3343a2d209a42cd3796bb63d82eb1ec37fb9cbf Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Tue, 14 Jul 2020 23:53:42 -0700 Subject: [PATCH 14/37] include disabled setinel --- config/core/configmaps/gc.yaml | 14 +++++---- pkg/gc/config.go | 35 +++++++++++++++------- pkg/gc/config_test.go | 55 +++++++++++++++++++--------------- 3 files changed, 64 insertions(+), 40 deletions(-) diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index b35bb843bb49..6049b0921cc7 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "a1cd0b95" + knative.dev/example-checksum: "6a3fcd16" data: _example: | ################################ @@ -78,9 +78,13 @@ data: # # # Example config to always keep around the last ten revisions: - # retain-since-create-time: "forever" + # retain-since-create-time: "disabled" # max-non-active-revisions: "10" # + # Example disable all GC: + # retain-since-create-time: "disabled" + # max-non-active-revisions: "disabled" + # # # Example config to keep recently deployed or active revisions, # always maintain the last two in case of rollback, and prevent @@ -91,15 +95,15 @@ data: # max-non-active-revisions: "1000" - # Duration since creation before considering a revision for GC or "forever". + # Duration since creation before considering a revision for GC or "disabled". retain-since-create-time: "48h" - # Duration since active before considering a revision for GC or "forever". + # Duration since active before considering a revision for GC or "disabled". retain-since-last-active-time: "15h" # Minimum number of stale revisions to retain. min-stale-revisions: "20" # Maximum number of non-active revisions to retain. - # Set to -1 to disable any maximum limit. + # Set to "disabled" to disable any maximum limit. max-non-active-revisions: "1000" diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 8da18c72dc5c..7168535393c1 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -19,6 +19,7 @@ package gc import ( "context" "fmt" + "strconv" "strings" "time" @@ -31,8 +32,9 @@ import ( const ( ConfigName = "config-gc" Forever = time.Duration(-1) + Infinity = -1 - foreverString = "forever" + disabled = "disabled" ) type Config struct { @@ -84,7 +86,7 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf return func(configMap *corev1.ConfigMap) (*Config, error) { c := defaultConfig() - var retainCreate, retainActive string + var retainCreate, retainActive, max string if err := cm.Parse(configMap.Data, cm.AsDuration("stale-revision-create-delay", &c.StaleRevisionCreateDelay), cm.AsDuration("stale-revision-timeout", &c.StaleRevisionTimeout), @@ -95,13 +97,15 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf cm.AsString("retain-since-create-time", &retainCreate), cm.AsString("retain-since-last-active-time", &retainActive), cm.AsInt64("min-stale-revisions", &c.MinStaleRevisions), - cm.AsInt64("max-non-active-revisions", &c.MaxNonActiveRevisions), + cm.AsString("max-non-active-revisions", &max), ); err != nil { return nil, fmt.Errorf("failed to parse data: %w", err) } if c.StaleRevisionMinimumGenerations < 0 { - return nil, fmt.Errorf("stale-revision-minimum-generations must be non-negative, was: %d", c.StaleRevisionMinimumGenerations) + return nil, fmt.Errorf( + "stale-revision-minimum-generations must be non-negative, was: %d", + c.StaleRevisionMinimumGenerations) } if c.StaleRevisionTimeout-c.StaleRevisionLastpinnedDebounce < minRevisionTimeout { logger.Warnf("Got revision timeout of %v, minimum supported value is %v", @@ -116,16 +120,25 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf if err := parseForeverOrDuration(retainActive, &c.RetainSinceLastActiveTime); err != nil { return nil, fmt.Errorf("failed to parse retain-since-last-active-time: %w", err) } - if c.MaxNonActiveRevisions >= 0 && c.MinStaleRevisions > c.MaxNonActiveRevisions { - return nil, fmt.Errorf( - "min-stale-revisions(%d) must be <= max-stale-revisions(%d)", - c.MinStaleRevisions, c.MaxNonActiveRevisions) + + if max == "" { + // keep default value + } else if strings.EqualFold(max, disabled) { + c.MaxNonActiveRevisions = Infinity + } else if parsed, err := strconv.ParseInt(max, 10, 64); err != nil { + return nil, fmt.Errorf("failed to parse max-stale-revisions, was: %d", c.MaxNonActiveRevisions) + } else { + if parsed < 0 { + return nil, fmt.Errorf("max-stale-revisions must non-negative or %q, was: %d", disabled, parsed) + } + c.MaxNonActiveRevisions = parsed } + if c.MinStaleRevisions < 0 { return nil, fmt.Errorf("min-stale-revisions must be non-negative, was: %d", c.MinStaleRevisions) } - if c.MaxNonActiveRevisions < -1 { - return nil, fmt.Errorf("max-stale-revisions must be >= -1, was: %d", c.MaxNonActiveRevisions) + if c.MaxNonActiveRevisions >= 0 && c.MinStaleRevisions > c.MaxNonActiveRevisions { + return nil, fmt.Errorf("min-stale-revisions(%d) must be <= max-stale-revisions(%d)", c.MinStaleRevisions, c.MaxNonActiveRevisions) } return c, nil } @@ -134,7 +147,7 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf func parseForeverOrDuration(val string, toSet *time.Duration) error { if val == "" { // keep default value - } else if strings.EqualFold(val, foreverString) { + } else if strings.EqualFold(val, disabled) { *toSet = Forever } else if parsed, err := time.ParseDuration(val); err != nil { return err diff --git a/pkg/gc/config_test.go b/pkg/gc/config_test.go index b2492e6316dd..417c159aa04e 100644 --- a/pkg/gc/config_test.go +++ b/pkg/gc/config_test.go @@ -35,17 +35,17 @@ func TestOurConfig(t *testing.T) { want *Config data map[string]string }{{ - name: "Actual config", + name: "actual config", fail: false, want: defaultConfig(), data: actual.Data, }, { - name: "Example config", + name: "example config", fail: false, want: defaultConfig(), data: example.Data, }, { - name: "With value overrides", + name: "with value overrides", want: &Config{ StaleRevisionCreateDelay: 15 * time.Hour, StaleRevisionTimeout: 14 * time.Hour, @@ -67,91 +67,98 @@ func TestOurConfig(t *testing.T) { "max-non-active-revisions": "500", }, }, { - name: "Invalid duration", + name: "invalid duration", fail: true, - want: nil, data: map[string]string{ "stale-revision-create-delay": "invalid", }, }, { - name: "Invalid negative minimum generation", + name: "invalid negative minimum generation", fail: true, - want: nil, data: map[string]string{ "stale-revision-minimum-generations": "-1", }, }, { - name: "Invalid minimum generation", + name: "invalid minimum generation", fail: true, - want: nil, data: map[string]string{ "stale-revision-minimum-generations": "invalid", }, }, { name: "Invalid negative min stale", fail: true, - want: nil, data: map[string]string{ "min-stale-revisions": "-1", }, }, { name: "Invalid negative maximum generation", fail: true, - want: nil, data: map[string]string{ "max-non-active-revisions": "-2", }, }, { - name: "Invalid max less than min", + name: "invalid max less than min", fail: true, - want: nil, data: map[string]string{ "min-stale-revisions": "20", "max-non-active-revisions": "10", }, }, { - name: "Unparsable create duration", + name: "unparsable create duration", fail: true, - want: nil, data: map[string]string{ "retain-since-create-time": "invalid", }, }, { - name: "Negative create duration", + name: "negative create duration", fail: true, - want: nil, data: map[string]string{ "retain-since-create-time": "-1d", }, }, { - name: "Negative last-active duration", + name: "negative last-active duration", fail: true, - want: nil, data: map[string]string{ "retain-since-last-active-time": "-1d", }, }, { - name: "create delay forever", + name: "create delay disabled", want: func() *Config { d := defaultConfig() d.RetainSinceCreateTime = Forever return d }(), data: map[string]string{ - "retain-since-create-time": "forever", + "retain-since-create-time": disabled, }, }, { - name: "last-active forever", + name: "last-active disabled", want: func() *Config { d := defaultConfig() d.RetainSinceLastActiveTime = Forever return d }(), data: map[string]string{ - "retain-since-last-active-time": "forever", + "retain-since-last-active-time": disabled, }, }, { - name: "Below minimum timeout", + name: "max-non-active unparsable", + fail: true, + data: map[string]string{ + "max-non-active-revisions": "invalid", + }, + }, { + name: "max-non-active disabled", + want: func() *Config { + d := defaultConfig() + d.MaxNonActiveRevisions = Infinity + return d + }(), + data: map[string]string{ + "max-non-active-revisions": disabled, + }, + }, { + name: "below minimum timeout", fail: false, want: &Config{ StaleRevisionCreateDelay: 15 * time.Hour, From 2c1334ffdeb4acf82a4e7e265374ffd6919b5fc2 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Wed, 15 Jul 2020 15:21:08 -0700 Subject: [PATCH 15/37] merge in settings --- config/core/configmaps/gc.yaml | 19 ++++++++------- pkg/gc/config.go | 42 +++++++++++++++++++--------------- pkg/gc/config_test.go | 4 ++-- 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index 6049b0921cc7..7584e366623c 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "6a3fcd16" + knative.dev/example-checksum: "e7f87361" data: _example: | ################################ @@ -28,7 +28,6 @@ data: # EXAMPLE CONFIGURATION # # # ################################ - # This block is not actually functional configuration, # but serves to illustrate the available configuration # options and document them in a way that is accessible @@ -63,20 +62,22 @@ data: # These settings are enabled via the "responsive-revision-gc" feature flag. # ALPHA NOTE: This feature is still experimental and under active development. # - # Staleness + # Stale # * Revisions are considered stale if they were created after more than - # "retain-since-create-time" or last referenced by a route more than - # "retain-since-last-active-time" ago. + # "retain-since-create-time" or last referenced by a route more than + # "retain-since-last-active-time" ago. # * Either timeout may be set to "forever" to turn off time-based GC. + # Active + # * Revisions which are referenced by a Route are considered active and + # are exempted from GC. # * Individual revisions may be marked with the annotation - # "knative.dev/no-gc":"true" to be permanently considered active. + # "knative.dev/no-gc":"true" to be permanently considered active. # # Example config to immediately collect any inactive revision: # min-stale-revisions: "0" # retain-since-create-time: "0" # retain-since-last-active-time: "0" # - # # Example config to always keep around the last ten revisions: # retain-since-create-time: "disabled" # max-non-active-revisions: "10" @@ -85,7 +86,6 @@ data: # retain-since-create-time: "disabled" # max-non-active-revisions: "disabled" # - # # Example config to keep recently deployed or active revisions, # always maintain the last two in case of rollback, and prevent # burst activity from exploding the count of old revisions: @@ -94,7 +94,6 @@ data: # min-stale-revisions: "2" # max-non-active-revisions: "1000" - # Duration since creation before considering a revision for GC or "disabled". retain-since-create-time: "48h" @@ -106,4 +105,4 @@ data: # Maximum number of non-active revisions to retain. # Set to "disabled" to disable any maximum limit. - max-non-active-revisions: "1000" + max-non-active-revisions: "1000" \ No newline at end of file diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 7168535393c1..c460bba98d2b 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -50,15 +50,17 @@ type Config struct { // Duration from creation when a Revision should be considered active // and exempt from GC. Note that GCMaxStaleRevision may override this if set. + // Set Forever (-1) to disable/ignore duration and always consider active. RetainSinceCreateTime time.Duration // Duration from last active when a Revision should be considered active // and exempt from GC.Note that GCMaxStaleRevision may override this if set. + // Set Forever (-1) to disable/ignore duration and always consider active. RetainSinceLastActiveTime time.Duration // Minimum number of stale revisions to keep before considering for GC. MinStaleRevisions int64 // Maximum number of non-active revisions to keep before considering for GC. // regardless of creation or staleness time-bounds - // Set -1 to disable this setting. + // Set Infinity (-1) to disable/ignore max. MaxNonActiveRevisions int64 } @@ -71,8 +73,6 @@ func defaultConfig() *Config { StaleRevisionMinimumGenerations: 20, // V2 GC Settings - // TODO(whaught): consider 'forever' sentinel value for use with max mode. - // TODO(whaught): validate positive RetainSinceCreateTime: 48 * time.Hour, RetainSinceLastActiveTime: 15 * time.Hour, MinStaleRevisions: 20, @@ -114,26 +114,15 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf } // validate V2 settings - if err := parseForeverOrDuration(retainCreate, &c.RetainSinceCreateTime); err != nil { + if err := parseDisabledOrDuration(retainCreate, &c.RetainSinceCreateTime); err != nil { return nil, fmt.Errorf("failed to parse min-stale-revisions: %w", err) } - if err := parseForeverOrDuration(retainActive, &c.RetainSinceLastActiveTime); err != nil { + if err := parseDisabledOrDuration(retainActive, &c.RetainSinceLastActiveTime); err != nil { return nil, fmt.Errorf("failed to parse retain-since-last-active-time: %w", err) } - - if max == "" { - // keep default value - } else if strings.EqualFold(max, disabled) { - c.MaxNonActiveRevisions = Infinity - } else if parsed, err := strconv.ParseInt(max, 10, 64); err != nil { - return nil, fmt.Errorf("failed to parse max-stale-revisions, was: %d", c.MaxNonActiveRevisions) - } else { - if parsed < 0 { - return nil, fmt.Errorf("max-stale-revisions must non-negative or %q, was: %d", disabled, parsed) - } - c.MaxNonActiveRevisions = parsed + if err := parseDisabledOrInt64(max, &c.MaxNonActiveRevisions); err != nil { + return nil, fmt.Errorf("failed to parse max-stale-revisions: %w", err) } - if c.MinStaleRevisions < 0 { return nil, fmt.Errorf("min-stale-revisions must be non-negative, was: %d", c.MinStaleRevisions) } @@ -144,7 +133,22 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf } } -func parseForeverOrDuration(val string, toSet *time.Duration) error { +func parseDisabledOrInt64(val string, toSet *int64) error { + if val == "" { + // keep default value + } else if strings.EqualFold(val, disabled) { + *toSet = Infinity + } else if parsed, err := strconv.ParseInt(val, 10, 64); err != nil { + return err + } else if parsed < 0 { + return fmt.Errorf("must non-negative or %q, was: %d", disabled, parsed) + } else { + *toSet = parsed + } + return nil +} + +func parseDisabledOrDuration(val string, toSet *time.Duration) error { if val == "" { // keep default value } else if strings.EqualFold(val, disabled) { diff --git a/pkg/gc/config_test.go b/pkg/gc/config_test.go index 417c159aa04e..a7f8b91992d5 100644 --- a/pkg/gc/config_test.go +++ b/pkg/gc/config_test.go @@ -113,13 +113,13 @@ func TestOurConfig(t *testing.T) { name: "negative create duration", fail: true, data: map[string]string{ - "retain-since-create-time": "-1d", + "retain-since-create-time": "-1h", }, }, { name: "negative last-active duration", fail: true, data: map[string]string{ - "retain-since-last-active-time": "-1d", + "retain-since-last-active-time": "-1h", }, }, { name: "create delay disabled", From 2ac77b3fa90c16964cf6ec95dad8057061e2351a Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Wed, 15 Jul 2020 16:25:43 -0700 Subject: [PATCH 16/37] disabled logic --- config/core/configmaps/gc.yaml | 13 ++++++++----- pkg/gc/config.go | 8 ++++---- pkg/reconciler/gc/v2/gc.go | 21 ++++++++++++++------- 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index 7584e366623c..1589a2747e81 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "e7f87361" + knative.dev/example-checksum: "31700824" data: _example: | ################################ @@ -28,6 +28,7 @@ data: # EXAMPLE CONFIGURATION # # # ################################ + # This block is not actually functional configuration, # but serves to illustrate the available configuration # options and document them in a way that is accessible @@ -66,7 +67,7 @@ data: # * Revisions are considered stale if they were created after more than # "retain-since-create-time" or last referenced by a route more than # "retain-since-last-active-time" ago. - # * Either timeout may be set to "forever" to turn off time-based GC. + # * Either timeout may be set to "disabled" to turn off time-based GC. # Active # * Revisions which are referenced by a Route are considered active and # are exempted from GC. @@ -80,10 +81,12 @@ data: # # Example config to always keep around the last ten revisions: # retain-since-create-time: "disabled" + # retain-since-last-active-time: "disabled" # max-non-active-revisions: "10" # # Example disable all GC: # retain-since-create-time: "disabled" + # retain-since-last-active-time: "disabled" # max-non-active-revisions: "disabled" # # Example config to keep recently deployed or active revisions, @@ -103,6 +106,6 @@ data: # Minimum number of stale revisions to retain. min-stale-revisions: "20" - # Maximum number of non-active revisions to retain. - # Set to "disabled" to disable any maximum limit. - max-non-active-revisions: "1000" \ No newline at end of file + # Maximum number of non-active revisions to retain + # or "disabled" to disable any maximum limit. + max-non-active-revisions: "1000" diff --git a/pkg/gc/config.go b/pkg/gc/config.go index c460bba98d2b..1c0feb789a13 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -31,7 +31,7 @@ import ( const ( ConfigName = "config-gc" - Forever = time.Duration(-1) + Disabled = time.Duration(-1) Infinity = -1 disabled = "disabled" @@ -50,11 +50,11 @@ type Config struct { // Duration from creation when a Revision should be considered active // and exempt from GC. Note that GCMaxStaleRevision may override this if set. - // Set Forever (-1) to disable/ignore duration and always consider active. + // Set Disabled (-1) to disable/ignore duration and always consider active. RetainSinceCreateTime time.Duration // Duration from last active when a Revision should be considered active // and exempt from GC.Note that GCMaxStaleRevision may override this if set. - // Set Forever (-1) to disable/ignore duration and always consider active. + // Set Disabled (-1) to disable/ignore duration and always consider active. RetainSinceLastActiveTime time.Duration // Minimum number of stale revisions to keep before considering for GC. MinStaleRevisions int64 @@ -152,7 +152,7 @@ func parseDisabledOrDuration(val string, toSet *time.Duration) error { if val == "" { // keep default value } else if strings.EqualFold(val, disabled) { - *toSet = Forever + *toSet = Disabled } else if parsed, err := time.ParseDuration(val); err != nil { return err } else if parsed < 0 { diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index bee45eb53e0c..f1f8d3df92d3 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -72,7 +72,7 @@ func Collect( numStale++ } - if numStale > minStale || maxStale != -1 && nonactive >= maxStale { + if numStale > minStale || maxStale != gc.Infinity && nonactive >= maxStale { err := client.ServingV1().Revisions(rev.Namespace).Delete( rev.Name, &metav1.DeleteOptions{}) if err != nil { @@ -100,19 +100,26 @@ func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool { } func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger) bool { + sinceCreate, sinceActive := cfg.RetainSinceCreateTime, cfg.RetainSinceLastActiveTime + if cfg.RetainSinceCreateTime == gc.Disabled && sinceActive == gc.Disabled { + return false + } + curTime := time.Now() createTime := rev.ObjectMeta.CreationTimestamp - if createTime.Add(cfg.RetainSinceCreateTime).After(curTime) { + if sinceCreate != gc.Disabled && createTime.Add(sinceCreate).After(curTime) { // Revision was created sooner than GCRetainSinceCreateTime. Ignore it. return false } - if active := revisionLastActiveTime(rev); active.Add(cfg.RetainSinceLastActiveTime).Before(curTime) { - logger.Infof("Detected stale revision %v with creation time %v and last active time %v.", - rev.ObjectMeta.Name, createTime, active) - return true + active := revisionLastActiveTime(rev) + if sinceActive != gc.Disabled && active.Add(sinceActive).After(curTime) { + return false } - return false + + logger.Infof("Detected stale revision %v with creation time %v and last active time %v.", + rev.ObjectMeta.Name, createTime, active) + return true } // revisionLastActiveTime returns if present: From a3da2ff5bed7eb06a488448aa429238711cee715 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Wed, 15 Jul 2020 16:33:56 -0700 Subject: [PATCH 17/37] update comments --- pkg/reconciler/gc/v2/gc.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index f1f8d3df92d3..7e0bee995167 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -102,19 +102,18 @@ func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool { func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger) bool { sinceCreate, sinceActive := cfg.RetainSinceCreateTime, cfg.RetainSinceLastActiveTime if cfg.RetainSinceCreateTime == gc.Disabled && sinceActive == gc.Disabled { - return false + return false // Time checks are both disabled. Not stale. } curTime := time.Now() createTime := rev.ObjectMeta.CreationTimestamp if sinceCreate != gc.Disabled && createTime.Add(sinceCreate).After(curTime) { - // Revision was created sooner than GCRetainSinceCreateTime. Ignore it. - return false + return false // Revision was created sooner than RetainSinceCreateTime. Not stale. } active := revisionLastActiveTime(rev) if sinceActive != gc.Disabled && active.Add(sinceActive).After(curTime) { - return false + return false // Revision was recently active. Not stale. } logger.Infof("Detected stale revision %v with creation time %v and last active time %v.", From 53302c546c3d0224225655dda469c8a9624318ad Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Wed, 15 Jul 2020 17:00:26 -0700 Subject: [PATCH 18/37] review suggestions --- config/core/configmaps/gc.yaml | 12 ++++++------ pkg/gc/config.go | 11 +++++------ pkg/gc/config_test.go | 6 +++--- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index 1589a2747e81..210602a0e468 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "31700824" + knative.dev/example-checksum: "cf278803" data: _example: | ################################ @@ -63,16 +63,16 @@ data: # These settings are enabled via the "responsive-revision-gc" feature flag. # ALPHA NOTE: This feature is still experimental and under active development. # - # Stale - # * Revisions are considered stale if they were created after more than - # "retain-since-create-time" or last referenced by a route more than - # "retain-since-last-active-time" ago. - # * Either timeout may be set to "disabled" to turn off time-based GC. # Active # * Revisions which are referenced by a Route are considered active and # are exempted from GC. # * Individual revisions may be marked with the annotation # "knative.dev/no-gc":"true" to be permanently considered active. + # Stale + # * Revisions are considered stale if they are not active and were created + # after more than "retain-since-create-time" or last referenced + # by a route more than "retain-since-last-active-time" ago. + # * Either timeout may be set to "disabled" to turn off time-based GC. # # Example config to immediately collect any inactive revision: # min-stale-revisions: "0" diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 1c0feb789a13..eef829e2b72e 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -31,8 +31,7 @@ import ( const ( ConfigName = "config-gc" - Disabled = time.Duration(-1) - Infinity = -1 + Disabled = -1 disabled = "disabled" ) @@ -59,8 +58,8 @@ type Config struct { // Minimum number of stale revisions to keep before considering for GC. MinStaleRevisions int64 // Maximum number of non-active revisions to keep before considering for GC. - // regardless of creation or staleness time-bounds - // Set Infinity (-1) to disable/ignore max. + // regardless of creation or staleness time-bounds. + // Set Disabled (-1) to disable/ignore max. MaxNonActiveRevisions int64 } @@ -137,7 +136,7 @@ func parseDisabledOrInt64(val string, toSet *int64) error { if val == "" { // keep default value } else if strings.EqualFold(val, disabled) { - *toSet = Infinity + *toSet = Disabled } else if parsed, err := strconv.ParseInt(val, 10, 64); err != nil { return err } else if parsed < 0 { @@ -152,7 +151,7 @@ func parseDisabledOrDuration(val string, toSet *time.Duration) error { if val == "" { // keep default value } else if strings.EqualFold(val, disabled) { - *toSet = Disabled + *toSet = time.Duration(Disabled) } else if parsed, err := time.ParseDuration(val); err != nil { return err } else if parsed < 0 { diff --git a/pkg/gc/config_test.go b/pkg/gc/config_test.go index a7f8b91992d5..7bd26899a21e 100644 --- a/pkg/gc/config_test.go +++ b/pkg/gc/config_test.go @@ -125,7 +125,7 @@ func TestOurConfig(t *testing.T) { name: "create delay disabled", want: func() *Config { d := defaultConfig() - d.RetainSinceCreateTime = Forever + d.RetainSinceCreateTime = time.Duration(Disabled) return d }(), data: map[string]string{ @@ -135,7 +135,7 @@ func TestOurConfig(t *testing.T) { name: "last-active disabled", want: func() *Config { d := defaultConfig() - d.RetainSinceLastActiveTime = Forever + d.RetainSinceLastActiveTime = time.Duration(Disabled) return d }(), data: map[string]string{ @@ -151,7 +151,7 @@ func TestOurConfig(t *testing.T) { name: "max-non-active disabled", want: func() *Config { d := defaultConfig() - d.MaxNonActiveRevisions = Infinity + d.MaxNonActiveRevisions = Disabled return d }(), data: map[string]string{ From 0259af4b997855325a897f2abd8178f9a2a9d71a Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Thu, 16 Jul 2020 18:29:57 -0700 Subject: [PATCH 19/37] fix const --- config/core/configmaps/gc.yaml | 31 +++++++++++++++++------------- pkg/gc/config.go | 35 +++++++++++++++++++--------------- pkg/reconciler/gc/v2/gc.go | 2 +- 3 files changed, 39 insertions(+), 29 deletions(-) diff --git a/config/core/configmaps/gc.yaml b/config/core/configmaps/gc.yaml index 210602a0e468..2b4984f3fb9b 100644 --- a/config/core/configmaps/gc.yaml +++ b/config/core/configmaps/gc.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "cf278803" + knative.dev/example-checksum: "cc98c285" data: _example: | ################################ @@ -64,22 +64,27 @@ data: # ALPHA NOTE: This feature is still experimental and under active development. # # Active - # * Revisions which are referenced by a Route are considered active and - # are exempted from GC. + # * Revisions which are referenced by a Route are considered active. # * Individual revisions may be marked with the annotation # "knative.dev/no-gc":"true" to be permanently considered active. - # Stale - # * Revisions are considered stale if they are not active and were created - # after more than "retain-since-create-time" or last referenced - # by a route more than "retain-since-last-active-time" ago. - # * Either timeout may be set to "disabled" to turn off time-based GC. + # * Active revisions are not considered for GC. + # Retention + # * Revisions are retained if they are any of the following: + # 1. Active + # 1. Were created within "retain-since-create-time" + # 3. Were last referenced by a route within + # "retain-since-last-active-time" + # 4. There are fewer than "min-non-active-revisions" + # If none of these conditions are met, or if the count of revisions exceed + # "max-non-active-revisions", they will be deleted by GC. + # The special value "disabled" may be used to turn off these limits. # # Example config to immediately collect any inactive revision: - # min-stale-revisions: "0" + # min-non-active-revisions: "0" # retain-since-create-time: "0" # retain-since-last-active-time: "0" # - # Example config to always keep around the last ten revisions: + # Example config to always keep around the last ten non-active revisions: # retain-since-create-time: "disabled" # retain-since-last-active-time: "disabled" # max-non-active-revisions: "10" @@ -94,7 +99,7 @@ data: # burst activity from exploding the count of old revisions: # retain-since-create-time: "48h" # retain-since-last-active-time: "15h" - # min-stale-revisions: "2" + # min-non-active-revisions: "2" # max-non-active-revisions: "1000" # Duration since creation before considering a revision for GC or "disabled". @@ -103,8 +108,8 @@ data: # Duration since active before considering a revision for GC or "disabled". retain-since-last-active-time: "15h" - # Minimum number of stale revisions to retain. - min-stale-revisions: "20" + # Minimum number of non-active revisions to retain. + min-non-active-revisions: "20" # Maximum number of non-active revisions to retain # or "disabled" to disable any maximum limit. diff --git a/pkg/gc/config.go b/pkg/gc/config.go index eef829e2b72e..51216a199c25 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -133,30 +133,35 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf } func parseDisabledOrInt64(val string, toSet *int64) error { - if val == "" { + switch { + case val == "": // keep default value - } else if strings.EqualFold(val, disabled) { + case strings.EqualFold(val, disabled): *toSet = Disabled - } else if parsed, err := strconv.ParseInt(val, 10, 64); err != nil { - return err - } else if parsed < 0 { - return fmt.Errorf("must non-negative or %q, was: %d", disabled, parsed) - } else { - *toSet = parsed + default: + parsed, err := strconv.ParseUint(val, 10, 64) + if err != nil { + return err + } + *toSet = int64(parsed) } return nil } func parseDisabledOrDuration(val string, toSet *time.Duration) error { - if val == "" { + switch { + case val == "": // keep default value - } else if strings.EqualFold(val, disabled) { + case strings.EqualFold(val, disabled): *toSet = time.Duration(Disabled) - } else if parsed, err := time.ParseDuration(val); err != nil { - return err - } else if parsed < 0 { - return fmt.Errorf("must be non-negative") - } else { + default: + parsed, err := time.ParseDuration(val) + if err != nil { + return err + } + if parsed < 0 { + return fmt.Errorf("must be non-negative") + } *toSet = parsed } return nil diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 7e0bee995167..eff4f4a15d99 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -72,7 +72,7 @@ func Collect( numStale++ } - if numStale > minStale || maxStale != gc.Infinity && nonactive >= maxStale { + if numStale > minStale || maxStale != gc.Disabled && nonactive >= maxStale { err := client.ServingV1().Revisions(rev.Namespace).Delete( rev.Name, &metav1.DeleteOptions{}) if err != nil { From 9f8da0f1c292922bf36eb52225faeeee7c55ca91 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Thu, 16 Jul 2020 19:07:01 -0700 Subject: [PATCH 20/37] update with logging changes --- pkg/gc/config.go | 14 +++++++------- pkg/gc/config_test.go | 4 ++-- pkg/reconciler/gc/v2/gc.go | 33 ++++++++++++++++++++------------- pkg/reconciler/gc/v2/gc_test.go | 10 +++++----- 4 files changed, 34 insertions(+), 27 deletions(-) diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 51216a199c25..72c7a299b2a6 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -56,7 +56,7 @@ type Config struct { // Set Disabled (-1) to disable/ignore duration and always consider active. RetainSinceLastActiveTime time.Duration // Minimum number of stale revisions to keep before considering for GC. - MinStaleRevisions int64 + MinNonActiveRevisions int64 // Maximum number of non-active revisions to keep before considering for GC. // regardless of creation or staleness time-bounds. // Set Disabled (-1) to disable/ignore max. @@ -74,7 +74,7 @@ func defaultConfig() *Config { // V2 GC Settings RetainSinceCreateTime: 48 * time.Hour, RetainSinceLastActiveTime: 15 * time.Hour, - MinStaleRevisions: 20, + MinNonActiveRevisions: 20, MaxNonActiveRevisions: 1000, } } @@ -95,7 +95,7 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf // v2 settings cm.AsString("retain-since-create-time", &retainCreate), cm.AsString("retain-since-last-active-time", &retainActive), - cm.AsInt64("min-stale-revisions", &c.MinStaleRevisions), + cm.AsInt64("min-stale-revisions", &c.MinNonActiveRevisions), cm.AsString("max-non-active-revisions", &max), ); err != nil { return nil, fmt.Errorf("failed to parse data: %w", err) @@ -122,11 +122,11 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf if err := parseDisabledOrInt64(max, &c.MaxNonActiveRevisions); err != nil { return nil, fmt.Errorf("failed to parse max-stale-revisions: %w", err) } - if c.MinStaleRevisions < 0 { - return nil, fmt.Errorf("min-stale-revisions must be non-negative, was: %d", c.MinStaleRevisions) + if c.MinNonActiveRevisions < 0 { + return nil, fmt.Errorf("min-stale-revisions must be non-negative, was: %d", c.MinNonActiveRevisions) } - if c.MaxNonActiveRevisions >= 0 && c.MinStaleRevisions > c.MaxNonActiveRevisions { - return nil, fmt.Errorf("min-stale-revisions(%d) must be <= max-stale-revisions(%d)", c.MinStaleRevisions, c.MaxNonActiveRevisions) + if c.MaxNonActiveRevisions >= 0 && c.MinNonActiveRevisions > c.MaxNonActiveRevisions { + return nil, fmt.Errorf("min-stale-revisions(%d) must be <= max-stale-revisions(%d)", c.MinNonActiveRevisions, c.MaxNonActiveRevisions) } return c, nil } diff --git a/pkg/gc/config_test.go b/pkg/gc/config_test.go index 7bd26899a21e..a87b579d7509 100644 --- a/pkg/gc/config_test.go +++ b/pkg/gc/config_test.go @@ -53,7 +53,7 @@ func TestOurConfig(t *testing.T) { StaleRevisionLastpinnedDebounce: 2*time.Hour + 30*time.Minute + 44*time.Second, RetainSinceCreateTime: 17 * time.Hour, RetainSinceLastActiveTime: 16 * time.Hour, - MinStaleRevisions: 5, + MinNonActiveRevisions: 5, MaxNonActiveRevisions: 500, }, data: map[string]string{ @@ -167,7 +167,7 @@ func TestOurConfig(t *testing.T) { StaleRevisionLastpinnedDebounce: 5 * time.Hour, RetainSinceCreateTime: 48 * time.Hour, RetainSinceLastActiveTime: 15 * time.Hour, - MinStaleRevisions: 20, + MinNonActiveRevisions: 20, MaxNonActiveRevisions: 1000, }, data: map[string]string{ diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index eff4f4a15d99..ecd722609f6e 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -50,8 +50,11 @@ func Collect( return err } - minStale, maxStale := int(cfg.MinStaleRevisions), int(cfg.MaxNonActiveRevisions) - if l := len(revs); l <= minStale || l <= maxStale { + min, max := int(cfg.MinNonActiveRevisions), int(cfg.MaxNonActiveRevisions) + if len(revs) <= min || + max == gc.Disabled && + cfg.RetainSinceCreateTime == gc.Disabled && + cfg.RetainSinceLastActiveTime == gc.Disabled { return nil } @@ -66,21 +69,25 @@ func Collect( if isRevisionActive(rev, config) { continue } - nonactive++ if isRevisionStale(cfg, rev, logger) { numStale++ } - if numStale > minStale || maxStale != gc.Disabled && nonactive >= maxStale { - err := client.ServingV1().Revisions(rev.Namespace).Delete( - rev.Name, &metav1.DeleteOptions{}) - if err != nil { - logger.With( - zap.Error(err)).Errorf( - "Failed to delete stale revision %q", rev.Name) - continue - } + if numStale > min { + logger.Infof("Deleting stale revision %q", rev.ObjectMeta.Name) + } else if max != gc.Disabled && nonactive >= max { + logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", + max, rev.ObjectMeta.Name) + } else { + continue + } + + err := client.ServingV1().Revisions(rev.Namespace).Delete( + rev.Name, &metav1.DeleteOptions{}) + if err != nil { + logger.With(zap.Error(err)).Errorf("Failed to delete stale revision %q", rev.Name) + continue } } return nil @@ -116,7 +123,7 @@ func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger return false // Revision was recently active. Not stale. } - logger.Infof("Detected stale revision %v with creation time %v and last active time %v.", + logger.Infof("Detected stale revision %q with creation time %v and last active time %v.", rev.ObjectMeta.Name, createTime, active) return true } diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index a8c473f9504d..bd56320ead71 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -58,7 +58,7 @@ func TestCollectMin(t *testing.T) { RevisionGC: &gcconfig.Config{ RetainSinceCreateTime: 5 * time.Minute, RetainSinceLastActiveTime: 5 * time.Minute, - MinStaleRevisions: 1, + MinNonActiveRevisions: 1, MaxNonActiveRevisions: -1, // assert no changes to min case }, } @@ -85,7 +85,7 @@ func TestCollectMin(t *testing.T) { WithRevName("5554"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(oldest)), - // Stale, but MinStaleRevisions is 1 + // Stale, but MinNonActiveRevisions is 1 rev("keep-two", "foo", 5555, MarkRevisionReady, WithRevName("5555"), WithRoutingState(v1.RoutingStateReserve), @@ -117,7 +117,7 @@ func TestCollectMin(t *testing.T) { WithRevName("5554"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(oldest)), - // Stale, but MinStaleRevisions is 1 + // Stale, but MinNonActiveRevisions is 1 rev("keep-two", "foo", 5555, MarkRevisionReady, WithRevName("5555"), WithRoutingState(v1.RoutingStateReserve), @@ -255,7 +255,7 @@ func TestCollectMax(t *testing.T) { RevisionGC: &gcconfig.Config{ RetainSinceCreateTime: 1 * time.Hour, RetainSinceLastActiveTime: 1 * time.Hour, - MinStaleRevisions: 1, + MinNonActiveRevisions: 1, MaxNonActiveRevisions: 3, }, } @@ -502,7 +502,7 @@ func TestIsRevisionStale(t *testing.T) { cfg := &gcconfig.Config{ RetainSinceCreateTime: 5 * time.Minute, RetainSinceLastActiveTime: 5 * time.Minute, - MinStaleRevisions: 2, + MinNonActiveRevisions: 2, } for _, test := range tests { From 70d3b3401efbcfcdc606780bb519d3b95d57acac Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Thu, 16 Jul 2020 19:28:50 -0700 Subject: [PATCH 21/37] fix unit test --- pkg/reconciler/gc/reconciler_test.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/gc/reconciler_test.go b/pkg/reconciler/gc/reconciler_test.go index e57bf95f62c4..4145b1b94618 100644 --- a/pkg/reconciler/gc/reconciler_test.go +++ b/pkg/reconciler/gc/reconciler_test.go @@ -35,6 +35,7 @@ import ( v1 "knative.dev/serving/pkg/apis/serving/v1" servingclient "knative.dev/serving/pkg/client/injection/client/fake" configreconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1/configuration" + "knative.dev/serving/pkg/gc" gcconfig "knative.dev/serving/pkg/gc" "knative.dev/serving/pkg/reconciler/configuration/resources" "knative.dev/serving/pkg/reconciler/gc/config" @@ -71,6 +72,10 @@ func TestGCReconcile(t *testing.T) { StaleRevisionCreateDelay: 5 * time.Minute, StaleRevisionTimeout: 5 * time.Minute, StaleRevisionMinimumGenerations: 2, + RetainSinceCreateTime: 5 * time.Minute, + RetainSinceLastActiveTime: 5 * time.Minute, + MinNonActiveRevisions: 1, + MaxNonActiveRevisions: gc.Disabled, }, }, }} @@ -85,16 +90,16 @@ func TestGCReconcile(t *testing.T) { WithConfigObservedGen), rev("keep-two", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithCreationTimestamp(oldest), - WithLastPinned(oldest)), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(oldest)), rev("keep-two", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(older)), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), rev("keep-two", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(old)), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), }, WantDeletes: []clientgotesting.DeleteActionImpl{{ ActionImpl: clientgotesting.ActionImpl{ From c838773c6e1a0c8edda39723a4055840b1e0b21f Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Thu, 16 Jul 2020 19:29:17 -0700 Subject: [PATCH 22/37] comment settings --- pkg/reconciler/gc/reconciler_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/gc/reconciler_test.go b/pkg/reconciler/gc/reconciler_test.go index 4145b1b94618..f6a48b249195 100644 --- a/pkg/reconciler/gc/reconciler_test.go +++ b/pkg/reconciler/gc/reconciler_test.go @@ -69,13 +69,16 @@ func TestGCReconcile(t *testing.T) { ConfigStore: &testConfigStore{ config: &config.Config{ RevisionGC: &gcconfig.Config{ + // v1 settings StaleRevisionCreateDelay: 5 * time.Minute, StaleRevisionTimeout: 5 * time.Minute, StaleRevisionMinimumGenerations: 2, - RetainSinceCreateTime: 5 * time.Minute, - RetainSinceLastActiveTime: 5 * time.Minute, - MinNonActiveRevisions: 1, - MaxNonActiveRevisions: gc.Disabled, + + // v2 settings + RetainSinceCreateTime: 5 * time.Minute, + RetainSinceLastActiveTime: 5 * time.Minute, + MinNonActiveRevisions: 1, + MaxNonActiveRevisions: gc.Disabled, }, }, }} From e4e0d7a999293259535f3283c1c8d51243114b1e Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Thu, 16 Jul 2020 19:55:58 -0700 Subject: [PATCH 23/37] unit test update --- pkg/reconciler/gc/v2/gc_test.go | 190 +++++++++++++++++++------------- 1 file changed, 115 insertions(+), 75 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index bd56320ead71..498fbe105468 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -213,39 +213,7 @@ func TestCollectMin(t *testing.T) { for _, test := range table { t.Run(test.name, func(t *testing.T) { - ctx, _ := SetupFakeContext(t) - ctx = config.ToContext(ctx, cfgMap) - client := fakeservingclient.Get(ctx) - - ri := fakerevisioninformer.Get(ctx) - for _, rev := range test.revs { - ri.Informer().GetIndexer().Add(rev) - } - - recorderList := ActionRecorderList{client} - - Collect(ctx, client, ri.Lister(), test.cfg) - - actions, err := recorderList.ActionsByVerb() - if err != nil { - t.Errorf("Error capturing actions by verb: %q", err) - } - - for i, want := range test.wantDeletes { - if i >= len(actions.Deletes) { - t.Errorf("Missing delete: %#v", want) - continue - } - got := actions.Deletes[i] - if got.GetName() != want.GetName() { - t.Errorf("Unexpected delete[%d]: %#v", i, got) - } - } - if got, want := len(actions.Deletes), len(test.wantDeletes); got > want { - for _, extra := range actions.Deletes[want:] { - t.Errorf("Extra delete: %s/%s", extra.GetNamespace(), extra.GetName()) - } - } + runTest(t, cfgMap, test.revs, test.cfg, test.wantDeletes) }) } } @@ -301,7 +269,7 @@ func TestCollectMax(t *testing.T) { WithConfigObservedGen), revs: []*v1.Revision{ // Stale and over the max - rev("keep-two", "foo", 5554, MarkRevisionReady, + rev("keep-two", "foo", 5553, MarkRevisionReady, WithRevName("5553"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(oldest)), @@ -340,68 +308,140 @@ func TestCollectMax(t *testing.T) { WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ - // Stale and over the max - rev("keep-two", "foo", 5554, MarkRevisionReady, + rev("keep-two", "foo", 5553, MarkRevisionReady, WithRevName("5553"), - WithRoutingState(v1.RoutingStateActive), - WithRoutingStateModified(oldest)), - // Stale but under max + WithRoutingState(v1.RoutingStateActive)), rev("keep-two", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithRoutingState(v1.RoutingStateActive), - WithRoutingStateModified(older)), - // Stale but under max + WithRoutingState(v1.RoutingStateActive)), rev("keep-two", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithRoutingState(v1.RoutingStateActive), - WithRoutingStateModified(older)), - // Actively referenced by Configuration + WithRoutingState(v1.RoutingStateActive)), rev("keep-two", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithRoutingState(v1.RoutingStateActive), - WithRoutingStateModified(old)), + WithRoutingState(v1.RoutingStateActive)), }, }} for _, test := range table { t.Run(test.name, func(t *testing.T) { - ctx, _ := SetupFakeContext(t) - ctx = config.ToContext(ctx, cfgMap) - client := fakeservingclient.Get(ctx) - - ri := fakerevisioninformer.Get(ctx) - for _, rev := range test.revs { - ri.Informer().GetIndexer().Add(rev) - } + runTest(t, cfgMap, test.revs, test.cfg, test.wantDeletes) + }) + } +} - recorderList := ActionRecorderList{client} +func TestCollectSettings(t *testing.T) { + now := time.Now() + old := now.Add(-11 * time.Minute) + older := now.Add(-12 * time.Minute) + oldest := now.Add(-13 * time.Minute) - Collect(ctx, client, ri.Lister(), test.cfg) + cfg := cfg("keep-two", "foo", 5556, + WithLatestCreated("5556"), + WithLatestReady("5556"), + WithConfigObservedGen) + + revs := []*v1.Revision{ + rev("keep-two", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(oldest)), + rev("keep-two", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + rev("keep-two", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), + } - actions, err := recorderList.ActionsByVerb() - if err != nil { - t.Errorf("Error capturing actions by verb: %q", err) - } + table := []struct { + name string + gcConfig gcconfig.Config + wantDeletes []clientgotesting.DeleteActionImpl + }{{ + name: "all disabled", + gcConfig: gcconfig.Config{ + RetainSinceCreateTime: time.Duration(gcconfig.Disabled), + RetainSinceLastActiveTime: time.Duration(gcconfig.Disabled), + MinNonActiveRevisions: 1, + MaxNonActiveRevisions: gcconfig.Disabled, + }, + }, { + name: "delete oldest, keep three max", + gcConfig: gcconfig.Config{ + RetainSinceCreateTime: time.Duration(gcconfig.Disabled), + RetainSinceLastActiveTime: 1 * time.Minute, + MinNonActiveRevisions: 1, + MaxNonActiveRevisions: gcconfig.Disabled, + }, + wantDeletes: []clientgotesting.DeleteActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: "foo", + Verb: "delete", + Resource: schema.GroupVersionResource{ + Group: "serving.knative.dev", + Version: "v1", + Resource: "revisions", + }, + }, + Name: "5554", + }}, + }} - for i, want := range test.wantDeletes { - if i >= len(actions.Deletes) { - t.Errorf("Missing delete: %#v", want) - continue - } - got := actions.Deletes[i] - if got.GetName() != want.GetName() { - t.Errorf("Unexpected delete[%d]: %#v", i, got) - } - } - if got, want := len(actions.Deletes), len(test.wantDeletes); got > want { - for _, extra := range actions.Deletes[want:] { - t.Errorf("Extra delete: %s/%s", extra.GetNamespace(), extra.GetName()) - } + for _, test := range table { + t.Run(test.name, func(t *testing.T) { + cfgMap := &config.Config{ + RevisionGC: &test.gcConfig, } + runTest(t, cfgMap, revs, cfg, test.wantDeletes) }) } } +func runTest( + t *testing.T, + cfgMap *config.Config, + revs []*v1.Revision, + cfg *v1.Configuration, + wantDeletes []clientgotesting.DeleteActionImpl) { + t.Helper() + ctx, _ := SetupFakeContext(t) + ctx = config.ToContext(ctx, cfgMap) + client := fakeservingclient.Get(ctx) + + ri := fakerevisioninformer.Get(ctx) + for _, rev := range revs { + ri.Informer().GetIndexer().Add(rev) + } + + recorderList := ActionRecorderList{client} + + Collect(ctx, client, ri.Lister(), cfg) + + actions, err := recorderList.ActionsByVerb() + if err != nil { + t.Errorf("Error capturing actions by verb: %q", err) + } + + for i, want := range wantDeletes { + if i >= len(actions.Deletes) { + t.Errorf("Missing delete: %#v", want) + continue + } + got := actions.Deletes[i] + if got.GetName() != want.GetName() { + t.Errorf("Unexpected delete[%d]: %#v", i, got) + } + } + if got, want := len(actions.Deletes), len(wantDeletes); got > want { + for _, extra := range actions.Deletes[want:] { + t.Errorf("Extra delete: %s/%s", extra.GetNamespace(), extra.GetName()) + } + } +} + func TestIsRevisionStale(t *testing.T) { curTime := time.Now() staleTime := curTime.Add(-10 * time.Minute) From 65aad633e53947c1e288b0b55866c11e49a95945 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 17 Jul 2020 10:26:03 -0700 Subject: [PATCH 24/37] nits --- pkg/reconciler/gc/v2/gc.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index ecd722609f6e..9d08af80063c 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -83,9 +83,8 @@ func Collect( continue } - err := client.ServingV1().Revisions(rev.Namespace).Delete( - rev.Name, &metav1.DeleteOptions{}) - if err != nil { + if err := client.ServingV1().Revisions(rev.Namespace).Delete( + rev.Name, &metav1.DeleteOptions{}); err != nil { logger.With(zap.Error(err)).Errorf("Failed to delete stale revision %q", rev.Name) continue } @@ -108,7 +107,7 @@ func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool { func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger) bool { sinceCreate, sinceActive := cfg.RetainSinceCreateTime, cfg.RetainSinceLastActiveTime - if cfg.RetainSinceCreateTime == gc.Disabled && sinceActive == gc.Disabled { + if sinceCreate == gc.Disabled && sinceActive == gc.Disabled { return false // Time checks are both disabled. Not stale. } From de048b5c6da04790ccb62a44332f5ac569a386f2 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 17 Jul 2020 10:38:12 -0700 Subject: [PATCH 25/37] fix unit test names --- pkg/reconciler/gc/v2/gc_test.go | 60 ++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 20 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index 498fbe105468..8a18108b439d 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -142,39 +142,39 @@ func TestCollectMin(t *testing.T) { }}, }, { name: "keep oldest when none Reserved", - cfg: cfg("keep-no-last-pinned", "foo", 5556, + cfg: cfg("none-reserved", "foo", 5556, WithLatestCreated("5556"), WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ // No lastPinned so we will keep this. - rev("keep-no-last-pinned", "foo", 5554, MarkRevisionReady, + rev("none-reserved", "foo", 5554, MarkRevisionReady, WithRevName("5554"), WithRoutingState(v1.RoutingStatePending), WithCreationTimestamp(oldest)), - rev("keep-no-last-pinned", "foo", 5555, MarkRevisionReady, + rev("none-reserved", "foo", 5555, MarkRevisionReady, WithRevName("5555"), WithRoutingState(v1.RoutingStateUnset), WithCreationTimestamp(older)), - rev("keep-no-last-pinned", "foo", 5556, MarkRevisionReady, + rev("none-reserved", "foo", 5556, MarkRevisionReady, WithRevName("5556"), WithRoutingState(v1.RoutingStateActive), WithCreationTimestamp(old)), }, }, { name: "none stale", - cfg: cfg("keep-no-last-pinned", "foo", 5556, WithConfigObservedGen), + cfg: cfg("none-stale", "foo", 5556, WithConfigObservedGen), revs: []*v1.Revision{ // No lastPinned so we will keep this. - rev("keep-no-last-pinned", "foo", 5554, MarkRevisionReady, + rev("none-stale", "foo", 5554, MarkRevisionReady, WithRevName("5554"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(now)), - rev("keep-no-last-pinned", "foo", 5555, MarkRevisionReady, + rev("none-stale", "foo", 5555, MarkRevisionReady, WithRevName("5555"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(now)), - rev("keep-no-last-pinned", "foo", 5556, MarkRevisionReady, + rev("none-stale", "foo", 5556, MarkRevisionReady, WithRevName("5556"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(now)), @@ -240,46 +240,46 @@ func TestCollectMax(t *testing.T) { wantDeletes []clientgotesting.DeleteActionImpl }{{ name: "at max", - cfg: cfg("keep-two", "foo", 5556, + cfg: cfg("at max", "foo", 5556, WithLatestCreated("5556"), WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ // Under max - rev("keep-two", "foo", 5554, MarkRevisionReady, + rev("at max", "foo", 5554, MarkRevisionReady, WithRevName("5554"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(older)), // Under max - rev("keep-two", "foo", 5555, MarkRevisionReady, + rev("at max", "foo", 5555, MarkRevisionReady, WithRevName("5555"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(older)), // Actively referenced by Configuration - rev("keep-two", "foo", 5556, MarkRevisionReady, + rev("at max", "foo", 5556, MarkRevisionReady, WithRevName("5556"), WithRoutingState(v1.RoutingStateActive), WithRoutingStateModified(old)), }, }, { name: "delete oldest, keep three max", - cfg: cfg("keep-two", "foo", 5556, + cfg: cfg("delete oldest", "foo", 5556, WithLatestCreated("5556"), WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ // Stale and over the max - rev("keep-two", "foo", 5553, MarkRevisionReady, + rev("delete oldest", "foo", 5553, MarkRevisionReady, WithRevName("5553"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(oldest)), // Stale but under max - rev("keep-two", "foo", 5554, MarkRevisionReady, + rev("delete oldest", "foo", 5554, MarkRevisionReady, WithRevName("5554"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(older)), // Stale but under max - rev("keep-two", "foo", 5555, MarkRevisionReady, + rev("delete oldest", "foo", 5555, MarkRevisionReady, WithRevName("5555"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(older)), @@ -336,21 +336,21 @@ func TestCollectSettings(t *testing.T) { older := now.Add(-12 * time.Minute) oldest := now.Add(-13 * time.Minute) - cfg := cfg("keep-two", "foo", 5556, + cfg := cfg("settings-test", "foo", 5556, WithLatestCreated("5556"), WithLatestReady("5556"), WithConfigObservedGen) revs := []*v1.Revision{ - rev("keep-two", "foo", 5554, MarkRevisionReady, + rev("settings-test", "foo", 5554, MarkRevisionReady, WithRevName("5554"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(oldest)), - rev("keep-two", "foo", 5555, MarkRevisionReady, + rev("settings-test", "foo", 5555, MarkRevisionReady, WithRevName("5555"), WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(older)), - rev("keep-two", "foo", 5556, MarkRevisionReady, + rev("settings-test", "foo", 5556, MarkRevisionReady, WithRevName("5556"), WithRoutingState(v1.RoutingStateActive), WithRoutingStateModified(old)), @@ -368,6 +368,26 @@ func TestCollectSettings(t *testing.T) { MinNonActiveRevisions: 1, MaxNonActiveRevisions: gcconfig.Disabled, }, + }, { + name: "staleness disabled", + gcConfig: gcconfig.Config{ + RetainSinceCreateTime: time.Duration(gcconfig.Disabled), + RetainSinceLastActiveTime: time.Duration(gcconfig.Disabled), + MinNonActiveRevisions: 0, + MaxNonActiveRevisions: 2, + }, + wantDeletes: []clientgotesting.DeleteActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: "foo", + Verb: "delete", + Resource: schema.GroupVersionResource{ + Group: "serving.knative.dev", + Version: "v1", + Resource: "revisions", + }, + }, + Name: "5554", + }}, }, { name: "delete oldest, keep three max", gcConfig: gcconfig.Config{ From d7297ecc66173765756443f7724c4c0e246f16f6 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 17 Jul 2020 11:14:31 -0700 Subject: [PATCH 26/37] more consistent name --- pkg/reconciler/gc/v2/gc.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 9d08af80063c..ad537413368a 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -64,17 +64,17 @@ func Collect( return a.After(b) }) - numStale, nonactive := 0, 0 + stale, nonactive := 0, 0 for _, rev := range revs { if isRevisionActive(rev, config) { continue } nonactive++ if isRevisionStale(cfg, rev, logger) { - numStale++ + stale++ } - if numStale > min { + if stale > min { logger.Infof("Deleting stale revision %q", rev.ObjectMeta.Name) } else if max != gc.Disabled && nonactive >= max { logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", From e994502bcf4d762c5e2841594d7b33c516c8c9b4 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 17 Jul 2020 11:18:18 -0700 Subject: [PATCH 27/37] fix max boundary --- pkg/reconciler/gc/v2/gc.go | 2 +- pkg/reconciler/gc/v2/gc_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index ad537413368a..c00cd2d11f4a 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -76,7 +76,7 @@ func Collect( if stale > min { logger.Infof("Deleting stale revision %q", rev.ObjectMeta.Name) - } else if max != gc.Disabled && nonactive >= max { + } else if max != gc.Disabled && nonactive > max { logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", max, rev.ObjectMeta.Name) } else { diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index 8a18108b439d..145ad06bb1ee 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -224,7 +224,7 @@ func TestCollectMax(t *testing.T) { RetainSinceCreateTime: 1 * time.Hour, RetainSinceLastActiveTime: 1 * time.Hour, MinNonActiveRevisions: 1, - MaxNonActiveRevisions: 3, + MaxNonActiveRevisions: 2, }, } @@ -374,7 +374,7 @@ func TestCollectSettings(t *testing.T) { RetainSinceCreateTime: time.Duration(gcconfig.Disabled), RetainSinceLastActiveTime: time.Duration(gcconfig.Disabled), MinNonActiveRevisions: 0, - MaxNonActiveRevisions: 2, + MaxNonActiveRevisions: 1, }, wantDeletes: []clientgotesting.DeleteActionImpl{{ ActionImpl: clientgotesting.ActionImpl{ From b1d34df223d9489347831be049bca21244df71ec Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 17 Jul 2020 17:22:17 -0700 Subject: [PATCH 28/37] nit --- pkg/reconciler/gc/v2/gc_test.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index 145ad06bb1ee..dbd52b5b935b 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -509,8 +509,7 @@ func TestIsRevisionStale(t *testing.T) { name: "stale pinned time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - // We technically don't expect create time to ever be ahead of pinned + Name: "myrev", CreationTimestamp: metav1.NewTime(staleTime), Annotations: map[string]string{ "serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()), @@ -522,8 +521,7 @@ func TestIsRevisionStale(t *testing.T) { name: "fresh pinned time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - // We technically don't expect create time to ever be ahead of pinned + Name: "myrev", CreationTimestamp: metav1.NewTime(staleTime), Annotations: map[string]string{ "serving.knative.dev/lastPinned": fmt.Sprintf("%d", curTime.Unix()), @@ -535,8 +533,7 @@ func TestIsRevisionStale(t *testing.T) { name: "stale revisionStateModified", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - // We technically don't expect create time to ever be ahead of pinned + Name: "myrev", CreationTimestamp: metav1.NewTime(staleTime), Annotations: map[string]string{ "serving.knative.dev/routingStateModified": staleTime.UTC().Format(time.RFC3339), @@ -548,8 +545,7 @@ func TestIsRevisionStale(t *testing.T) { name: "fresh revisionStateModified", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - // We technically don't expect create time to ever be ahead of pinned + Name: "myrev", CreationTimestamp: metav1.NewTime(staleTime), Annotations: map[string]string{ "serving.knative.dev/routingStateModified": curTime.UTC().Format(time.RFC3339), From 5f1fc4c8609cd9d3c741379d07741bc4e21b3cf5 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Fri, 17 Jul 2020 17:26:04 -0700 Subject: [PATCH 29/37] remove ref to lastpinned --- pkg/reconciler/gc/v2/gc_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index dbd52b5b935b..fddf9c9e7987 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -147,7 +147,6 @@ func TestCollectMin(t *testing.T) { WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ - // No lastPinned so we will keep this. rev("none-reserved", "foo", 5554, MarkRevisionReady, WithRevName("5554"), WithRoutingState(v1.RoutingStatePending), @@ -165,7 +164,6 @@ func TestCollectMin(t *testing.T) { name: "none stale", cfg: cfg("none-stale", "foo", 5556, WithConfigObservedGen), revs: []*v1.Revision{ - // No lastPinned so we will keep this. rev("none-stale", "foo", 5554, MarkRevisionReady, WithRevName("5554"), WithRoutingState(v1.RoutingStateReserve), From 0085ef865a5ed4fc5666a21c1b587957396b452f Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Sun, 19 Jul 2020 21:54:41 -0700 Subject: [PATCH 30/37] review suggestions --- pkg/reconciler/gc/v2/gc.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index c00cd2d11f4a..89a9a711ce0d 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -75,7 +75,7 @@ func Collect( } if stale > min { - logger.Infof("Deleting stale revision %q", rev.ObjectMeta.Name) + logger.Info("Deleting stale revision: ", rev.ObjectMeta.Name) } else if max != gc.Disabled && nonactive > max { logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", max, rev.ObjectMeta.Name) @@ -85,7 +85,7 @@ func Collect( if err := client.ServingV1().Revisions(rev.Namespace).Delete( rev.Name, &metav1.DeleteOptions{}); err != nil { - logger.With(zap.Error(err)).Errorf("Failed to delete stale revision %q", rev.Name) + logger.With(zap.Error(err)).Error("Failed to GC revision: ", rev.Name) continue } } @@ -111,14 +111,13 @@ func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger return false // Time checks are both disabled. Not stale. } - curTime := time.Now() - createTime := rev.ObjectMeta.CreationTimestamp - if sinceCreate != gc.Disabled && createTime.Add(sinceCreate).After(curTime) { + createTime := rev.ObjectMeta.CreationTimestamp.Time + if sinceCreate != gc.Disabled && time.Since(createTime) > sinceCreate { return false // Revision was created sooner than RetainSinceCreateTime. Not stale. } active := revisionLastActiveTime(rev) - if sinceActive != gc.Disabled && active.Add(sinceActive).After(curTime) { + if sinceActive != gc.Disabled && time.Since(active) > sinceActive { return false // Revision was recently active. Not stale. } From ec96b4bff4300d6fcd4b77ed4975961be9e83c3d Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Sun, 19 Jul 2020 22:09:13 -0700 Subject: [PATCH 31/37] fix sign --- pkg/reconciler/gc/v2/gc.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 89a9a711ce0d..708522be4c84 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -112,12 +112,12 @@ func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger } createTime := rev.ObjectMeta.CreationTimestamp.Time - if sinceCreate != gc.Disabled && time.Since(createTime) > sinceCreate { + if sinceCreate != gc.Disabled && time.Since(createTime) < sinceCreate { return false // Revision was created sooner than RetainSinceCreateTime. Not stale. } active := revisionLastActiveTime(rev) - if sinceActive != gc.Disabled && time.Since(active) > sinceActive { + if sinceActive != gc.Disabled && time.Since(active) < sinceActive { return false // Revision was recently active. Not stale. } From 43316693a7e43e381b6cb3245307ad219bcbf75c Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 20 Jul 2020 19:35:00 -0700 Subject: [PATCH 32/37] use nonactive as min --- pkg/reconciler/gc/v2/gc.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 708522be4c84..5e7b36825cb2 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -64,17 +64,14 @@ func Collect( return a.After(b) }) - stale, nonactive := 0, 0 + nonactive := 0 for _, rev := range revs { if isRevisionActive(rev, config) { continue } nonactive++ - if isRevisionStale(cfg, rev, logger) { - stale++ - } - if stale > min { + if isRevisionStale(cfg, rev, logger) && nonactive > min { logger.Info("Deleting stale revision: ", rev.ObjectMeta.Name) } else if max != gc.Disabled && nonactive > max { logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", From f29b4221ce1bef984a087446db7f315db07b0f6c Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 20 Jul 2020 20:03:42 -0700 Subject: [PATCH 33/37] first filter out active --- pkg/reconciler/gc/v2/gc.go | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 5e7b36825cb2..94cb1c27914d 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -58,22 +58,19 @@ func Collect( return nil } + // filter out active revs + revs = nonactiveRevisions(revs, config) + // Sort by last active descending sort.Slice(revs, func(i, j int) bool { a, b := revisionLastActiveTime(revs[i]), revisionLastActiveTime(revs[j]) return a.After(b) }) - nonactive := 0 - for _, rev := range revs { - if isRevisionActive(rev, config) { - continue - } - nonactive++ - - if isRevisionStale(cfg, rev, logger) && nonactive > min { + for i, rev := range revs { + if isRevisionStale(cfg, rev, logger) && i >= min { logger.Info("Deleting stale revision: ", rev.ObjectMeta.Name) - } else if max != gc.Disabled && nonactive > max { + } else if max != gc.Disabled && i >= max { logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", max, rev.ObjectMeta.Name) } else { @@ -89,6 +86,16 @@ func Collect( return nil } +func nonactiveRevisions(revs []*v1.Revision, config *v1.Configuration) []*v1.Revision { + nonactiverevs := make([]*v1.Revision, 0, len(revs)) + for _, rev := range revs { + if !isRevisionActive(rev, config) { + nonactiverevs = append(nonactiverevs, rev) + } + } + return nonactiverevs +} + func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool { if config.Status.LatestReadyRevisionName == rev.Name { return true // never delete latest ready, even if config is not active. From f0264d7e9664da83548ee231e1654df302083b40 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 20 Jul 2020 20:08:45 -0700 Subject: [PATCH 34/37] comment nit on config --- pkg/gc/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 72c7a299b2a6..e71326e9fe48 100644 --- a/pkg/gc/config.go +++ b/pkg/gc/config.go @@ -55,7 +55,7 @@ type Config struct { // and exempt from GC.Note that GCMaxStaleRevision may override this if set. // Set Disabled (-1) to disable/ignore duration and always consider active. RetainSinceLastActiveTime time.Duration - // Minimum number of stale revisions to keep before considering for GC. + // Minimum number of non-active revisions to keep before considering for GC. MinNonActiveRevisions int64 // Maximum number of non-active revisions to keep before considering for GC. // regardless of creation or staleness time-bounds. From 5a7c70b5a2e9a950a78e29599d159b69ca8cf575 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 20 Jul 2020 20:26:40 -0700 Subject: [PATCH 35/37] swap if/else ordering --- pkg/reconciler/gc/v2/gc.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 94cb1c27914d..d77430655908 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -68,11 +68,11 @@ func Collect( }) for i, rev := range revs { - if isRevisionStale(cfg, rev, logger) && i >= min { - logger.Info("Deleting stale revision: ", rev.ObjectMeta.Name) - } else if max != gc.Disabled && i >= max { + if max != gc.Disabled && i >= max { logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", max, rev.ObjectMeta.Name) + } else if isRevisionStale(cfg, rev, logger) && i >= min { + logger.Info("Deleting stale revision: ", rev.ObjectMeta.Name) } else { continue } From 345e7ec9e3ccf854b8f1c6a62d5342ba753aab24 Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 20 Jul 2020 23:45:00 -0700 Subject: [PATCH 36/37] simplify --- pkg/reconciler/gc/v2/gc.go | 48 ++++++++++++++++++++++----------- pkg/reconciler/gc/v2/gc_test.go | 2 +- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index d77430655908..dc99e28b054a 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -58,42 +58,58 @@ func Collect( return nil } - // filter out active revs + // Filter out active revs revs = nonactiveRevisions(revs, config) - // Sort by last active descending + // Sort by last active ascending (oldest first) sort.Slice(revs, func(i, j int) bool { a, b := revisionLastActiveTime(revs[i]), revisionLastActiveTime(revs[j]) - return a.After(b) + return a.Before(b) }) - for i, rev := range revs { - if max != gc.Disabled && i >= max { - logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", - max, rev.ObjectMeta.Name) - } else if isRevisionStale(cfg, rev, logger) && i >= min { - logger.Info("Deleting stale revision: ", rev.ObjectMeta.Name) - } else { + // Delete stale revisions while more than min remain + remaining := len(revs) + nonstale := make([]*v1.Revision, 0, remaining) + for _, rev := range revs { + switch { + case remaining <= min: + return nil + + case !isRevisionStale(cfg, rev, logger): + nonstale = append(nonstale, rev) continue + + default: + remaining-- + logger.Info("Deleting stale revision: ", rev.ObjectMeta.Name) + if err := client.ServingV1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}); err != nil { + logger.With(zap.Error(err)).Error("Failed to GC revision: ", rev.Name) + } } + } - if err := client.ServingV1().Revisions(rev.Namespace).Delete( - rev.Name, &metav1.DeleteOptions{}); err != nil { + if max == gc.Disabled || len(nonstale) <= max { + return nil + } + + // Delete extra revisions past max + for _, rev := range nonstale[:len(nonstale)-max] { + logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", max, rev.ObjectMeta.Name) + if err := client.ServingV1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}); err != nil { logger.With(zap.Error(err)).Error("Failed to GC revision: ", rev.Name) - continue } } return nil } func nonactiveRevisions(revs []*v1.Revision, config *v1.Configuration) []*v1.Revision { - nonactiverevs := make([]*v1.Revision, 0, len(revs)) + nonactive := make([]*v1.Revision, 0, len(revs)) for _, rev := range revs { if !isRevisionActive(rev, config) { - nonactiverevs = append(nonactiverevs, rev) + nonactive = append(nonactive, rev) } } - return nonactiverevs + return nonactive } func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool { diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index 7a15b958eac1..37fd099a400b 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -388,7 +388,7 @@ func TestCollectSettings(t *testing.T) { Name: "5554", }}, }, { - name: "delete oldest, keep three max", + name: "max disabled", gcConfig: gcconfig.Config{ RetainSinceCreateTime: time.Duration(gcconfig.Disabled), RetainSinceLastActiveTime: 1 * time.Minute, From 0204045146be5a5c6f7ca8fd6227515aa9d8312f Mon Sep 17 00:00:00 2001 From: Weston Haught Date: Mon, 20 Jul 2020 23:46:49 -0700 Subject: [PATCH 37/37] whitespace --- pkg/reconciler/gc/v2/gc.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index dc99e28b054a..d3d389b696be 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/pkg/reconciler/gc/v2/gc.go @@ -52,9 +52,7 @@ func Collect( min, max := int(cfg.MinNonActiveRevisions), int(cfg.MaxNonActiveRevisions) if len(revs) <= min || - max == gc.Disabled && - cfg.RetainSinceCreateTime == gc.Disabled && - cfg.RetainSinceLastActiveTime == gc.Disabled { + max == gc.Disabled && cfg.RetainSinceCreateTime == gc.Disabled && cfg.RetainSinceLastActiveTime == gc.Disabled { return nil }