Refactor garbage collection from configuration reconciler by taragu · Pull Request #4876 · knative/serving

taragu · 2019-07-22T14:37:54Z

/lint

Proposed Changes

Separate GC into it's own Reconciler

Release Note

NONE

knative-prow-robot

@taragu: 7 warnings.

Details

In response to this:

/lint

Fixes #3910

Proposed Changes

Separate GC into it's own Reconciler

Release Note
NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2019-07-22T14:38:07Z

+
+type cfgKey struct{}
+
+// +k8s:deepcopy-gen=false


Golint comments: comment on exported type Config should be of the form "Config ..." (with optional leading article). More info.

knative-prow-robot · 2019-07-22T14:38:07Z

+	RevisionGC *gc.Config
+}
+
+func FromContext(ctx context.Context) *Config {


Golint comments: exported function FromContext should have comment or be unexported. More info.

knative-prow-robot · 2019-07-22T14:38:07Z

+	return ctx.Value(cfgKey{}).(*Config)
+}
+
+func ToContext(ctx context.Context, c *Config) context.Context {


Golint comments: exported function ToContext should have comment or be unexported. More info.

knative-prow-robot · 2019-07-22T14:38:07Z

+	return context.WithValue(ctx, cfgKey{}, c)
+}
+
+// +k8s:deepcopy-gen=false


Golint comments: comment on exported type Store should be of the form "Store ..." (with optional leading article). More info.

knative-prow-robot · 2019-07-22T14:38:08Z

+	*configmap.UntypedStore
+}
+
+func (s *Store) ToContext(ctx context.Context) context.Context {


Golint comments: exported method Store.ToContext should have comment or be unexported. More info.

knative-prow-robot · 2019-07-22T14:38:08Z

+	return ToContext(ctx, s.Load())
+}
+
+func (s *Store) Load() *Config {


Golint comments: exported method Store.Load should have comment or be unexported. More info.

knative-prow-robot · 2019-07-22T14:38:08Z

+	}
+}
+
+func NewStore(logger configmap.Logger, minRevisionTimeout time.Duration) *Store {


Golint comments: exported function NewStore should have comment or be unexported. More info.

knative-prow-robot · 2019-07-22T14:38:09Z

Hi @taragu. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

taragu · 2019-07-22T14:39:12Z

/assign @markusthoemmes

greghaynes · 2019-07-22T16:29:01Z

/ok-to-test

vagababov

Please fix all the linter errors as well.

vagababov · 2019-07-22T16:54:50Z

+	// Reconcile this copy of the configuration and then write back any status
+	// updates regardless of whether the reconciliation errored out.
+	reconcileErr := c.reconcile(ctx, config)
+	if equality.Semantic.DeepEqual(original.Status, config.Status) {


you are not really updating any statuses, so this whole block is redundant.

vagababov · 2019-07-22T16:58:02Z

+	c.Logger.Info("Setting up event handlers")
+	configurationInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
+
+	revisionInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{


I am not sure that this is how you want to set the handlers.
You don't really care about changes to the revisions or configs (I guess creation of a revision will trigger a change to the configuration.latestCreatedRevisionName)...
Probably just global resync every N hours would suffice.

vagababov · 2019-07-22T16:59:04Z

+	// If the spec has changed, then assume we need an upgrade and issue a patch to trigger
+	// the webhook to upgrade via defaulting.  Status updates do not trigger this due to the
+	// use of the /status resource.
+	if !equality.Semantic.DeepEqual(original.Spec, config.Spec) {


nor you're changing any specs.

taragu · 2019-07-22T19:02:59Z

/lint

knative-prow-robot

@taragu: 0 warnings.

Details

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

taragu · 2019-07-24T17:36:54Z

@vagababov I've updated the resync period to every 4 hours. @dgerd WDYT?

vagababov · 2019-07-24T18:37:01Z

+	cmw configmap.Watcher,
+) *controller.Impl {
+
+	configurationInformer := configurationinformer.Get(ctx)


You should use

serving/vendor/knative.dev/pkg/controller/controller.go

Line 438 in 4f4739b

func WithResyncPeriod(ctx context.Context, resync time.Duration) context.Context {

to get actually resync behaviour you're looking for.

vagababov · 2019-07-25T17:46:00Z

+) *controller.Impl {
+
+	// Globally resync every couple of hours
+	controller.WithResyncPeriod(ctx, resyncInterval)


The way this works is:
ctx := controller.With...
The old context remains, in theory, immutable.

vagababov · 2019-07-25T17:46:39Z

+)
+
+// Reconciler implements controller.Reconciler for Garbage Collection resources.
+type Reconciler struct {


There is no reason for this to be a public type.

vagababov · 2019-07-25T17:47:16Z

+	original, err := c.configurationLister.Configurations(namespace).Get(name)
+	if errors.IsNotFound(err) {
+		// The resource no longer exists, in which case we stop processing.
+		logger.Errorf("configuration %q in work queue no longer exists", key)


Suggested change

logger.Errorf("configuration %q in work queue no longer exists", key)

logger.Errorf("Configuration %q in work queue no longer exists", key)

vagababov · 2019-07-25T17:47:36Z

+	}
+
+	// Don't modify the informer's copy.
+	config := original.DeepCopy()


you don't really modify configs, so this is redundant.

vagababov · 2019-07-25T17:47:45Z

+	// Don't modify the informer's copy.
+	config := original.DeepCopy()
+
+	// Reconcile this copy of the configuration and then write back any status


Comment does not reflect the reality.

vagababov · 2019-07-25T17:58:46Z

+		return revs[j].CreationTimestamp.Before(&revs[i].CreationTimestamp)
+	})
+
+	for _, rev := range revs[gcSkipOffset:] {


I am not a fan of this logic, since let's presume I have
r1, r2,..., r10 and gcskipoffset is 8 and traffic uses r8 and r9 -- we'll never collect anything.
But I guess it depends on the point of view

/rant
@dgerd

I think its mainly about letting users have so many latest revisions and not worrying too much about potentially overshooting revision count (they arent that expensive, especially when scaled down).

Either way though - IMO we should try and leave behavior as is for this change and do any modifications to behavior afterward.

Well they aren't cheap. It's at least 3 K8s services/endpoints pairs and additional work in the autoscaler. It's not very expensive, but not cheap either.
It's more of a rant, than a call for action :-)

vagababov · 2019-07-25T17:59:39Z

+			err := c.ServingClientSet.ServingV1alpha1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{})
+			if err != nil {
+				logger.Errorf("Failed to delete stale revision: %v", err)
+				return err


Do we want perhaps to continue collecting the rest of them?
Also error.Wrapf() would be nice to include the actual revision name being deleted

vagababov · 2019-07-25T18:00:49Z

+	cfg := configns.FromContext(ctx).RevisionGC
+	logger := logging.FromContext(ctx)
+
+	if config.Status.LatestReadyRevisionName == rev.Name {


Well, I guess you can move this to the top, since it does not use the first two variables, so let's save a few cycles.

greghaynes

Just minor comment, overall I think this looks great!

greghaynes · 2019-07-25T19:13:13Z

 	// The set of controllers this controller process runs.
+	"knative.dev/pkg/injection/sharedmain"
 	"knative.dev/serving/pkg/reconciler/configuration"
+	"knative.dev/serving/pkg/reconciler/gc" // This defines the shared main for injected controllers.


I think the comment here can be removed (its no longer valid)

greghaynes · 2019-07-25T19:19:17Z

+
+const (
+	controllerAgentName = "gc-controller"
+	resyncInterval      = 4 * time.Hour


Nit: I think its fine for us to decrease this value but with a change like this I prefer to keep our existing behavior first and then update configuration like this in a later change - makes it easier to notice if we broke anything with this change. Currently I believe this value is 10 * time.Hour.

Actually, rather than statically set this to 10, can we keep using controller.GetResyncPeriod(ctx)?

@greghaynes I was talking to @vagababov, he mentioned he had a discussion with @dgerd and decided to resync every hour here. @vagababov what's the reasoning behind it?

Dan's point was that if users set their GC collection time to a short window, e.g. 1 hour, keeping them around for 10 hours is not very nice.

Taking a look at config-gc.yaml it looks like we prevent (or maybe just highly encourage) stale-revision-timeout from going lower than the resync period.

# Duration since a route has been pointed at a revision before it should be GC'd # This minus lastpinned-debounce be longer than the controller resync period (10 hours) stale-revision-timeout: "15h"

We likely will want to make this at least twice as fast as the stale-revision-timeout to avoid having it just miss the resync, but I am going to agree with greg on this to keep it the same for this change and follow up with lowering it.

greghaynes · 2019-07-25T19:29:08Z

+		return revs[j].CreationTimestamp.Before(&revs[i].CreationTimestamp)
+	})
+
+	for _, rev := range revs[gcSkipOffset:] {


I think its mainly about letting users have so many latest revisions and not worrying too much about potentially overshooting revision count (they arent that expensive, especially when scaled down).

Either way though - IMO we should try and leave behavior as is for this change and do any modifications to behavior afterward.

taragu · 2019-07-26T16:13:32Z

@vagababov @dgerd @greghaynes I've updated the PR to keep the existing resync period. Would you please review this PR again?

vagababov · 2019-07-26T16:19:19Z

/test pull-knative-serving-go-coverage

mattmoor

/hold

Couple of comments inline, one that's quite important...

Given that it takes bake time to uncover GC bugs (like those found here) I am inclined to hold this until after the 0.8 cut and land it right after.
-M

mattmoor · 2019-07-29T21:43:23Z

 	"knative.dev/serving/pkg/reconciler/service"
-
-	// This defines the shared main for injected controllers.
-	"knative.dev/pkg/injection/sharedmain"


Can you leave this where it is? It is now grouped under an irrelevant comment.

mattmoor · 2019-07-29T21:47:38Z

 	}

-	return c.gcRevisions(ctx, config)
+	return nil


I think there's a bunch more code you can blast from orbit because config-gc is the only configmap consumed by this controller:

serving/pkg/reconciler/configuration/config/store.go

Line 63 in 52a6e2c

gc.ConfigName: gc.NewConfigFromConfigMapFunc(logger, minRevisionTimeout),

If you do it here, then the same logic for the GC controller will show up as moves and be easier to review

mattmoor · 2019-07-29T21:48:19Z

+	"knative.dev/serving/pkg/apis/serving/v1beta1"
+	listers "knative.dev/serving/pkg/client/listers/serving/v1alpha1"
+	pkgreconciler "knative.dev/serving/pkg/reconciler"
+	configns "knative.dev/serving/pkg/reconciler/configuration/config"


As above, this should move to this controller's own directory.

mattmoor · 2019-07-29T21:50:18Z

+)
+
+const (
+	controllerAgentName = "gc-controller"


nit: revision-gc-controller

mattmoor · 2019-07-29T21:51:16Z

+		revisionLister:      revisionInformer.Lister(),
+	}
+	impl := controller.NewImpl(c, c.Logger, "Garbage Collection")
+


You aren't enqueuing any events, so this controller won't do anything :(

Something like:

serving/pkg/reconciler/configuration/controller.go

Lines 51 to 57 in 52a6e2c

c.Logger.Info("Setting up event handlers")

configurationInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))

revisionInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{

FilterFunc: controller.Filter(v1alpha1.SchemeGroupVersion.WithKind("Configuration")),

Handler: controller.HandleAll(impl.EnqueueControllerOf),

})

@mattmoor I see. In this case, I don't think this controller needs to respond to any specific configuration or revision events, because we just want to sync globally every few hours. Does the following do that?

configStore := configns.NewStore(c.Logger.Named("config-store"), controller.GetResyncPeriod(ctx)) configStore.WatchConfigs(c.ConfigMapWatcher)

No, that just registers callbacks when the configmap changes to update the store.

You need something like this:

serving/pkg/reconciler/revision/controller.go

Lines 111 to 117 in 74e366a

resync := configmap.TypeFilter(configsToResync...)(func(string, interface{}) {

// Triggers syncs on all revisions when configuration

// changes

impl.GlobalResync(revisionInformer.Informer())

})

configStore := config.NewStore(c.Logger.Named("config-store"), resync)

It looks like to add global resync will require a change to the store to expose onAfterStore like this:

serving/pkg/reconciler/revision/config/store.go

Line 57 in 74e366a

func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value interface{})) *Store {

mattmoor · 2019-07-29T21:52:13Z

+	c.Logger.Info("Setting up ConfigMap receivers")
+	configStore := configns.NewStore(c.Logger.Named("config-store"), controller.GetResyncPeriod(ctx))
+	configStore.WatchConfigs(c.ConfigMapWatcher)
+	c.configStore = configStore


We should probably have a global resync on changes to the configmap

vagababov · 2019-07-30T14:12:17Z

It seems we need to add handler, but just process everything. 🤦‍♂️

…

On Tuesday, July 30, 2019, Tara Gu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/reconciler/gc/controller.go <#4876 (comment)>: > +// NewController creates a new Garbage Collection controller +func NewController( + ctx context.Context, + cmw configmap.Watcher, +) *controller.Impl { + + configurationInformer := configurationinformer.Get(ctx) + revisionInformer := revisioninformer.Get(ctx) + + c := &reconciler{ + Base: pkgreconciler.NewBase(ctx, controllerAgentName, cmw), + configurationLister: configurationInformer.Lister(), + revisionLister: revisionInformer.Lister(), + } + impl := controller.NewImpl(c, c.Logger, "Garbage Collection") + @mattmoor <https://github.com/mattmoor> I see. In this case, I don't think this controller needs to respond to any specific configuration or revision events, because we just want to sync globally every few hours. Does the following do that? configStore := configns.NewStore(c.Logger.Named("config-store"), controller.GetResyncPeriod(ctx)) configStore.WatchConfigs(c.ConfigMapWatcher) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#4876?email_source=notifications&email_token=AAF2WX5WMUSUQPJN54EJT23QCA3YZA5CNFSM4IFZKTLKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB77Q7WY#discussion_r308707334>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF2WXYXESLOQFQA5PR4GT3QCA3YZANCNFSM4IFZKTLA> .

taragu · 2019-07-30T19:13:29Z

@vagababov I've talked to @mattmoor on slack, and agreed that we should match the event handlers of revision gc controller and the existing configuration controller to minimize risk as we head into v1.0. I've updated the gc controller accordingly.

vagababov

Looks fine by me.
I'll let @mattmoor to do the final review.

mattmoor · 2019-08-06T14:21:38Z


-	// Inject the fake informers we need.
-	_ "knative.dev/serving/pkg/client/injection/informers/serving/v1alpha1/configuration/fake"
-	_ "knative.dev/serving/pkg/client/injection/informers/serving/v1alpha1/revision/fake"


Can you leave these where they are (with comment)?

mattmoor · 2019-08-06T14:22:17Z

+	revisioninformer "knative.dev/serving/pkg/client/injection/informers/serving/v1alpha1/revision"
 	"knative.dev/serving/pkg/reconciler"
-	configns "knative.dev/serving/pkg/reconciler/configuration/config"
+	configns "knative.dev/serving/pkg/reconciler/gc/config"


This should not be needed anymore.

mattmoor

/hold cancel
/lgtm
/approve

knative-prow-robot · 2019-08-07T14:00:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, taragu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~cmd/controller/OWNERS~~ [mattmoor]
~~hack/OWNERS~~ [mattmoor]
~~pkg/reconciler/OWNERS~~ [mattmoor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

taragu · 2019-08-07T14:33:39Z

/test pull-knative-serving-integration-tests

knative-test-reporter-robot · 2019-08-07T17:31:34Z

The following tests are currently flaky. Running them again to verify...

Test name	Retries
pull-knative-serving-integration-tests	2/3

Automatically retrying...
/test pull-knative-serving-integration-tests

taragu · 2019-08-07T17:32:00Z

/test pull-knative-serving-integration-tests

taragu · 2019-08-07T20:02:29Z

/test pull-knative-serving-integration-tests

taragu · 2019-08-07T21:32:36Z

/test pull-knative-serving-build-tests

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 22, 2019

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 22, 2019

knative-prow-robot requested review from jonjohnsonjr and vagababov July 22, 2019 14:38

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 22, 2019

knative-prow-robot reviewed Jul 22, 2019

View reviewed changes

knative-prow-robot added the area/API API objects and controllers label Jul 22, 2019

knative-prow-robot assigned markusthoemmes Jul 22, 2019

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 22, 2019

vagababov reviewed Jul 22, 2019

View reviewed changes

taragu force-pushed the gc-reconciler branch from a7b0e84 to f28c925 Compare July 22, 2019 19:02

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 22, 2019

knative-prow-robot reviewed Jul 22, 2019

View reviewed changes

taragu force-pushed the gc-reconciler branch 2 times, most recently from 907fedd to 4f4739b Compare July 24, 2019 17:29

vagababov reviewed Jul 24, 2019

View reviewed changes

taragu force-pushed the gc-reconciler branch from 4f4739b to 6421b98 Compare July 24, 2019 18:47

vagababov suggested changes Jul 25, 2019

View reviewed changes

taragu force-pushed the gc-reconciler branch from 3ec40ab to b32ac1f Compare July 25, 2019 19:16

greghaynes reviewed Jul 25, 2019

View reviewed changes

taragu force-pushed the gc-reconciler branch from b32ac1f to e7ce73c Compare July 26, 2019 12:48

knative-prow-robot assigned dgerd Jul 29, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2019

mattmoor reviewed Jul 29, 2019

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2019

taragu force-pushed the gc-reconciler branch from 9d56da1 to 91984d5 Compare July 30, 2019 17:43

knative-prow-robot added area/test-and-release It flags unit/e2e/conformance/perf test issues for product features and removed lgtm Indicates that a PR is ready to be merged. labels Jul 30, 2019

taragu force-pushed the gc-reconciler branch 2 times, most recently from 2131906 to 01a059c Compare July 30, 2019 19:00

vagababov reviewed Jul 31, 2019

View reviewed changes

mattmoor reviewed Aug 6, 2019

View reviewed changes

taragu force-pushed the gc-reconciler branch 2 times, most recently from bdc9ec8 to 5c6af10 Compare August 6, 2019 17:30

mattmoor reviewed Aug 7, 2019

View reviewed changes

Comment thread pkg/reconciler/configuration/configuration_test.go Outdated

Refactor garbage collection from configuration reconciler

598c203

taragu force-pushed the gc-reconciler branch from 96e10ef to 598c203 Compare August 7, 2019 13:54

mattmoor reviewed Aug 7, 2019

View reviewed changes

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 7, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2019

knative-prow-robot merged commit a6a69f0 into knative:master Aug 7, 2019

taragu deleted the gc-reconciler branch November 12, 2019 16:03

	logger.Errorf("configuration %q in work queue no longer exists", key)
	logger.Errorf("Configuration %q in work queue no longer exists", key)

	c.Logger.Info("Setting up event handlers")
	configurationInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))

	revisionInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
	FilterFunc: controller.Filter(v1alpha1.SchemeGroupVersion.WithKind("Configuration")),
	Handler: controller.HandleAll(impl.EnqueueControllerOf),
	})

	resync := configmap.TypeFilter(configsToResync...)(func(string, interface{}) {
	// Triggers syncs on all revisions when configuration
	// changes
	impl.GlobalResync(revisionInformer.Informer())
	})

	configStore := config.NewStore(c.Logger.Named("config-store"), resync)

Conversation

taragu commented Jul 22, 2019

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jul 22, 2019

Uh oh!

taragu commented Jul 22, 2019

Uh oh!

greghaynes commented Jul 22, 2019

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taragu commented Jul 22, 2019

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

taragu commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greghaynes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment