Provide option to exclude a registered informer from starting #2976

skonto · 2024-02-28T13:23:49Z

Changes

Adds a context based predicate to filter out registered informers that should not start.
Needed by [wip] Integrate net-certmanager in Serving serving#14955 (integrating net-certmanager in Serving)
Here is how to use the filtering mechanism:

// informer to skip from starting, it implies that we have generated code for that informer as usual
"knative.dev/serving/pkg/net-certmanager/client/certmanager/injection/informers/acme/v1/challenge"

func main () {
       ....
        if shouldEnableNetCertManagerController(ctx, client) {
		ctors = append(ctors, certificate.NewController) // add the net-certmanager controller
	} else {
		// Remove all non required informers
		ctx = injection.WithInformerExcludingFilter(ctx, excludeNetCertManagerInformers)
	}
	sharedmain.MainWithConfig(ctx, "controller", cfg, ctors...)
}

func excludeNetCertManagerInformers(ctx context.Context, inf controller.Informer) (bool, string) {
	switch {
	case v1certificate.Get(ctx).Informer() == inf:
		return true, "CertificateInformer"
	case ...
	}
	return false, ""
}

This works because injection via an init func stores instances of cache.SharedIndexInformer, it calls inf.Informer(), which are unique at the k8s client go side (one per watched resource type). Thus we can always get back the same instance (inf.Informer()) and compare.

Tested in [wip] Integrate net-certmanager in Serving serving#14955

Output in controller logs without any flag set:

2024/03/08 11:42:15 Registering 6 clients
2024/03/08 11:42:15 Registering 6 informer factories
2024/03/08 11:42:15 Registering 20 informers
2024/03/08 11:42:15 Registering 9 controllers
{"level":"info","ts":1709898135.6646473,"logger":"fallback","caller":"injection/injection.go:68","msg":"Excluding informer ChallengeInformer"}
{"level":"info","ts":1709898135.6646998,"logger":"fallback","caller":"injection/injection.go:68","msg":"Excluding informer CertificateInformer"}
{"level":"info","ts":1709898135.6647108,"logger":"fallback","caller":"injection/injection.go:68","msg":"Excluding informer CertificateRequestInformer"}
{"level":"info","ts":1709898135.664719,"logger":"fallback","caller":"injection/injection.go:68","msg":"Excluding informer ClusterIssuerInformer"}
{"level":"info","ts":1709898135.6647272,"logger":"fallback","caller":"injection/injection.go:68","msg":"Excluding informer IssuerInformer"}
....
{"level":"info","ts":1709898135.7881877,"logger":"fallback","caller":"injection/injection.go:76","msg":"Starting 15 informers..."}

In config-network cm we set external-domain-tls: enabled, install cert-manager and then delete the previous controller pod:

2024/03/08 11:47:35 Registering 6 clients
2024/03/08 11:47:35 Registering 6 informer factories
2024/03/08 11:47:35 Registering 20 informers
2024/03/08 11:47:35 Registering 10 controllers
...
{"level":"info","ts":1709898455.9024231,"logger":"fallback","caller":"injection/injection.go:76","msg":"Starting 20 informers..."}

All related informers are started.

It could be an option to avoid even registering the informers but that requires further changes at the codegen part which is more intrusive imho.

/kind enhancement

Release Note

Docs

knative-prow · 2024-02-28T13:23:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skonto

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [skonto]

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

codecov · 2024-02-28T13:30:43Z

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 78.61%. Comparing base (97fb318) to head (a13500b).
Report is 5 commits behind head on main.

Files	Patch %	Lines
injection/injection.go	0.00%	20 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2976      +/-   ##
==========================================
- Coverage   78.70%   78.61%   -0.10%     
==========================================
  Files         188      188              
  Lines       11051    11069      +18     
==========================================
+ Hits         8698     8702       +4     
- Misses       2090     2104      +14     
  Partials      263      263

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

injection/injection.go

ReToCode · 2024-02-28T14:01:01Z

Nice :)

skonto · 2024-03-06T08:32:54Z

@dprotaso gentle ping, could you pls review?

skonto · 2024-03-08T11:56:03Z

@dprotaso gentle ping, could you pls review?

skonto · 2024-03-13T08:08:27Z

@dprotaso gentle ping

pierDipi

How do you see a non-started informer to be started?

ReToCode · 2024-03-13T09:01:56Z

How do you see a non-started informer to be started?

The component holding the informer needs to be restarted, as this stuff happens on init(). In our case, the net-certmanager controller should be integrated in serving, which means you have to restart Serving controller after you install cert-manager and the CRDs are there.

Not super ideal, but IMHO still better than installing/updating a separate component :)

pierDipi · 2024-03-13T10:30:39Z

I wonder if in that case using manually creating informers is easier?

infX := factory.Get(ctx).ForResource(...)
infY := factory.Get(ctx).ForResource(...)
...  := controller.StartInformers(ctx, infX, infY)

// use infX and infY

ReToCode · 2024-03-13T10:33:43Z

I wonder if in that case using manually creating informers is easier?

I think we (as in the controller in Serving) do not know the informers, as they are added dynamically based on what packages are imported (they self-register in their init() functions).

pierDipi · 2024-03-13T10:35:56Z

I think you do, you need to avoid importing the injection informers for optional resources and just import the factory package and the concrete informers (if really needed)

ReToCode · 2024-03-13T10:40:55Z

I think you do, you need to avoid importing the injection informers for optional resources and just import the factory package and the concrete informers (if really needed)

AFAIK, the Serving controller itself holds multiple "subcontrollers". The parent one starts all the informers (via sharedmain) that are on the context (so it does not know which ones upfront). The subcontrollers add them to the context indirectly based on the init functions. So IMHO we would need to change quite a lot to do that. I mean here we just want to ignore a specific one when the CRDs are not installed. Maybe @skonto has something to add?

pierDipi · 2024-03-13T10:41:39Z

Example on the serving PR:

import "knative.dev/serving/pkg/net-certmanager/client/certmanager/injection/informers/factory/factory"
// ...

func NewController( ... ) *controller.Impl {
  infCertificates := factory.Get(ctx).Certmanager().V1().Certificates().Informer()
  ...  := controller.StartInformers(ctx, infCertificates)

  // ...
}

pierDipi · 2024-03-13T10:44:19Z

You need to avoid importing packages like this to opt-out of injection for that informer:

"knative.dev/serving/pkg/net-certmanager/client/certmanager/injection/informers/certmanager/v1/certificate/certificate"

skonto · 2024-03-13T10:54:15Z

I wonder if in that case using manually creating informers is easier?

I thought about that I didn't want to re-write too much the main function for the controller we already have that for autoscaler, activator and wanted to provide a feature.

You need to avoid importing packages like to opt-out of injection for that informer:

Yes because of the init thingy though I want to be able to start something on demand so I need to have it injected if I dont create it dynamically.

skonto · 2024-03-13T10:58:55Z

How do you see a non-started informer to be started?

It is not, it is simply going to be excluded and never started.

pierDipi · 2024-03-13T11:02:23Z

I wonder if in that case using manually creating informers is easier?

I though about that I didn't want to re-write too much the main function for the controller we already have that for autoscaler, activator and wanted to provide a feature.

With the solution I'm suggesting, the main function will remain to be the one that is currently on main, so another added benefit because with the PR you will need to maintain your own flags now.

You need to avoid importing packages like to opt-out of injection for that informer:

Yes because of the init thingy though I want to be able to start something on demand so I need to have it injected if I dont create it dynamically.

isn't this possible with the alternative solution?
It's actually more dynamic and even possible to remove the requirement of "restart controller" when enabling TLS

skonto · 2024-03-13T11:21:01Z

With the solution I'm suggesting, the main function will remain to be the one that is currently on main, so another added benefit because with the PR you will need to maintain your own flags now.

What I am saying unless you expand main then you don't report the started informers correctly because you use the factory to create them on demand within the targeted controller. See https://github.com/knative/pkg/blob/main/injection/sharedmain/main.go#L230. That is what I meant create it as a feature, go through main. Btw you need to wait for the cache to be populated after startup which is a different UX.
If we don't mind about this deviation then I am fine.

It's actually more dynamic and even possible to remove the requirement of "restart controller" when enabling TLS

The idea of dynamic informers to avoid restarts has been around for sometime but it would be nice if main did support that.

I don't mind closing this PR and just creating/starting the informer in the corresponding controller but let's wait for @dprotaso and his take on this one.

dprotaso · 2024-03-13T13:25:18Z

It would be good to add flexibility to our codegen that does the injection

eg. If can add an option to make this init optional and export the WithInformer function

pkg/codegen/cmd/injection-gen/generators/informer.go

Lines 109 to 120 in 2c15a6f

    
           func init() { 
        
           	{{.injectionRegisterInformer|raw}}(withInformer) 
        
           } 
        
           // Key is used for associating the Informer inside the context.Context. 
        
           type Key struct{} 
        
           func withInformer(ctx {{.contextContext|raw}}) ({{.contextContext|raw}}, {{.controllerInformer|raw}}) { 
        
           	f := {{.factoryGet|raw}}(ctx) 
        
           	inf := f.{{.groupGoName}}().{{.versionGoName}}().{{.type|publicPlural}}() 
        
           	return {{ .contextWithValue|raw }}(ctx, Key{}, inf), inf.Informer() 
        
           }

Then in our mains we could decide to register the informer like so

injection.Default.RegisterInformer(WithInformer)

skonto · 2024-03-13T13:32:56Z

I wanted some quick fix to unblock stuff, but maybe doing a more long term solution wrt codegen is more beneficial. I will take a look on the codegen approach.

skonto · 2024-03-19T15:25:28Z

closing in favor of #2989

provide option to exclude a registered informer from starting

9ce2cee

knative-prow bot added the kind/enhancement label Feb 28, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2024

knative-prow bot requested review from creydr and izabelacg February 28, 2024 13:23

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 28, 2024

skonto requested review from dprotaso and ReToCode and removed request for creydr and izabelacg February 28, 2024 13:24

skonto changed the title ~~Provide option to exclude a registered informer from starting~~ [wip] Provide option to exclude a registered informer from starting Feb 28, 2024

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2024

skonto added 2 commits February 28, 2024 15:38

fix lint

1487331

add a comment

536fea2

ReToCode reviewed Feb 28, 2024

View reviewed changes

injection/injection.go Outdated Show resolved Hide resolved

ReToCode reviewed Feb 28, 2024

View reviewed changes

injection/injection.go Outdated Show resolved Hide resolved

skonto changed the title ~~[wip] Provide option to exclude a registered informer from starting~~ Provide option to exclude a registered informer from starting Feb 28, 2024

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2024

comments

a13500b

pierDipi reviewed Mar 13, 2024

View reviewed changes

skonto assigned dprotaso Mar 13, 2024

skonto added this to the v1.14.0 milestone Mar 13, 2024

skonto mentioned this pull request Mar 14, 2024

Optionally generate an init func for an informer #2989

Merged

skonto closed this Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide option to exclude a registered informer from starting #2976

Provide option to exclude a registered informer from starting #2976

skonto commented Feb 28, 2024 •

edited

Loading

knative-prow bot commented Feb 28, 2024

codecov bot commented Feb 28, 2024 •

edited

Loading

ReToCode commented Feb 28, 2024

skonto commented Mar 6, 2024

skonto commented Mar 8, 2024

skonto commented Mar 13, 2024

pierDipi left a comment

ReToCode commented Mar 13, 2024 •

edited

Loading

pierDipi commented Mar 13, 2024 •

edited

Loading

ReToCode commented Mar 13, 2024 •

edited

Loading

pierDipi commented Mar 13, 2024 •

edited

Loading

ReToCode commented Mar 13, 2024

pierDipi commented Mar 13, 2024 •

edited

Loading

pierDipi commented Mar 13, 2024 •

edited

Loading

skonto commented Mar 13, 2024 •

edited

Loading

skonto commented Mar 13, 2024

pierDipi commented Mar 13, 2024

skonto commented Mar 13, 2024 •

edited

Loading

dprotaso commented Mar 13, 2024

skonto commented Mar 13, 2024 •

edited

Loading

skonto commented Mar 19, 2024

Provide option to exclude a registered informer from starting #2976

Provide option to exclude a registered informer from starting #2976

Conversation

skonto commented Feb 28, 2024 • edited Loading

Changes

knative-prow bot commented Feb 28, 2024

codecov bot commented Feb 28, 2024 • edited Loading

Codecov Report

ReToCode commented Feb 28, 2024

skonto commented Mar 6, 2024

skonto commented Mar 8, 2024

skonto commented Mar 13, 2024

pierDipi left a comment

Choose a reason for hiding this comment

ReToCode commented Mar 13, 2024 • edited Loading

pierDipi commented Mar 13, 2024 • edited Loading

ReToCode commented Mar 13, 2024 • edited Loading

pierDipi commented Mar 13, 2024 • edited Loading

ReToCode commented Mar 13, 2024

pierDipi commented Mar 13, 2024 • edited Loading

pierDipi commented Mar 13, 2024 • edited Loading

skonto commented Mar 13, 2024 • edited Loading

skonto commented Mar 13, 2024

pierDipi commented Mar 13, 2024

skonto commented Mar 13, 2024 • edited Loading

dprotaso commented Mar 13, 2024

skonto commented Mar 13, 2024 • edited Loading

skonto commented Mar 19, 2024

skonto commented Feb 28, 2024 •

edited

Loading

codecov bot commented Feb 28, 2024 •

edited

Loading

ReToCode commented Mar 13, 2024 •

edited

Loading

pierDipi commented Mar 13, 2024 •

edited

Loading

ReToCode commented Mar 13, 2024 •

edited

Loading

pierDipi commented Mar 13, 2024 •

edited

Loading

pierDipi commented Mar 13, 2024 •

edited

Loading

pierDipi commented Mar 13, 2024 •

edited

Loading

skonto commented Mar 13, 2024 •

edited

Loading

skonto commented Mar 13, 2024 •

edited

Loading

skonto commented Mar 13, 2024 •

edited

Loading