fix: many namespace lead to provider stuck #1386

shareinto · 2022-10-14T09:25:42Z

Type of change:

Bugfix
New feature provided
Improve performance
Backport patches

What this PR does / why we need it:

Resolve: #1378

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

AlinsRan · 2022-10-15T05:07:56Z

This is not correct, because informer is not started, which will not get any objects.

codecov-commenter · 2022-10-16T03:51:46Z

Codecov Report

Merging #1386 (d590755) into master (8a17eea) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1386   +/-   ##
=======================================
  Coverage   40.47%   40.47%           
=======================================
  Files          78       78           
  Lines        7076     7076           
=======================================
  Hits         2864     2864           
  Misses       3897     3897           
  Partials      315      315

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

AlinsRan

Sorry for the delay.
I thinking using Labels.Nothing() is not correct. This will not get the object.
https://pkg.go.dev/k8s.io/apimachinery/pkg/labels#Nothing

AlinsRan · 2022-12-02T07:38:09Z

pkg/providers/apisix/provider_init.go

 			switch p.common.Config.Kubernetes.APIVersion {
 			case config.ApisixV2beta3:
-				retRoutes, err := p.common.KubeClient.APISIXClient.ApisixV2beta3().ApisixRoutes(ns).List(ctx, opts)
+				retRoutes, err := p.apisixSharedInformerFactory.Apisix().V2beta3().ApisixRoutes().Lister().ApisixRoutes(ns).List(labels.Nothing())


Suggested change

retRoutes, err := p.apisixSharedInformerFactory.Apisix().V2beta3().ApisixRoutes().Lister().ApisixRoutes(ns).List(labels.Nothing())

retRoutes, err := p.apisixSharedInformerFactory.Apisix().V2beta3().ApisixRoutes().Lister().ApisixRoutes(ns).List(labels.Everything())

Thanks for reminding, I'll fix it as soon as possible

AlinsRan · 2022-12-02T07:39:05Z

pkg/providers/apisix/provider_init.go

@@ -93,13 +96,13 @@ func (p *apisixProvider) Init(ctx context.Context) error {
 					}
 				}
 			case config.ApisixV2:
-				retRoutes, err := p.common.KubeClient.APISIXClient.ApisixV2().ApisixRoutes(ns).List(ctx, opts)
+				retRoutes, err := p.apisixSharedInformerFactory.Apisix().V2().ApisixRoutes().Lister().ApisixRoutes(ns).List(labels.Nothing())


AlinsRan · 2022-12-02T07:39:17Z

pkg/providers/apisix/provider_init.go

@@ -138,13 +141,13 @@ func (p *apisixProvider) Init(ctx context.Context) error {
 			switch p.common.Config.Kubernetes.APIVersion {
 			case config.ApisixV2beta3:
 				// ApisixConsumer
-				retConsumer, err := p.common.KubeClient.APISIXClient.ApisixV2beta3().ApisixConsumers(ns).List(ctx, opts)
+				retConsumer, err := p.apisixSharedInformerFactory.Apisix().V2beta3().ApisixConsumers().Lister().ApisixConsumers(ns).List(labels.Nothing())


AlinsRan · 2022-12-02T07:39:23Z

pkg/providers/apisix/provider_init.go

@@ -154,13 +157,13 @@ func (p *apisixProvider) Init(ctx context.Context) error {
 					}
 				}
 				// ApisixTls
-				retSSL, err := p.common.KubeClient.APISIXClient.ApisixV2beta3().ApisixTlses(ns).List(ctx, opts)
+				retSSL, err := p.apisixSharedInformerFactory.Apisix().V2beta3().ApisixTlses().Lister().ApisixTlses(ns).List(labels.Nothing())


AlinsRan · 2022-12-02T07:39:29Z

pkg/providers/apisix/provider_init.go

@@ -171,13 +174,13 @@ func (p *apisixProvider) Init(ctx context.Context) error {
 				}
 			case config.ApisixV2:
 				// ApisixConsumer
-				retConsumer, err := p.common.KubeClient.APISIXClient.ApisixV2().ApisixConsumers(ns).List(ctx, opts)
+				retConsumer, err := p.apisixSharedInformerFactory.Apisix().V2().ApisixConsumers().Lister().ApisixConsumers(ns).List(labels.Nothing())


AlinsRan · 2022-12-02T07:39:44Z

pkg/providers/apisix/provider_init.go

@@ -187,13 +190,13 @@ func (p *apisixProvider) Init(ctx context.Context) error {
 					}
 				}
 				// ApisixTls
-				retSSL, err := p.common.KubeClient.APISIXClient.ApisixV2().ApisixTlses(ns).List(ctx, opts)
+				retSSL, err := p.apisixSharedInformerFactory.Apisix().V2().ApisixTlses().Lister().ApisixTlses(ns).List(labels.Nothing())


AlinsRan · 2022-12-02T08:14:08Z

We have a lot of informer.run(), cache.wait() in our code, actually it only needs to call factor.start() and factor.wait() to complete.
There is a conflict between them and they can only be started once. I suggest that the codes of informer.run and cache.wait() should be deleted.

What do you think?

cc @tao12345666333

tao12345666333 · 2022-12-08T10:06:12Z

pkg/providers/apisix/provider_init.go

@@ -48,23 +48,26 @@ func (p *apisixProvider) Init(ctx context.Context) error {
 		pluginConfigMapA6 = make(map[string]string)
 	)

+	p.apisixSharedInformerFactory.Start(ctx.Done())
+	p.apisixSharedInformerFactory.WaitForCacheSync(ctx.Done())


I think we should check its return value, and log it.

synced := p.apisixSharedInformerFactory.WaitForCacheSync(ctx.Done()) for v, ok := range synced { if !ok { // Log it and return } }

tao12345666333 · 2022-12-09T12:12:23Z

We have a lot of informer.run(), cache.wait() in our code, actually it only needs to call factor.start() and factor.wait() to complete.

There is a conflict between them and they can only be started once. I suggest that the codes of informer.run and cache.wait() should be deleted.

What do you think?

cc @tao12345666333

Sorry, I forgot to reply you. I think so. Meanwhile, I think it can be done in a follow-up PR

fix: many namespace lead to provider stuck

b9e232f

tao12345666333 requested review from lingsamuel, AlinsRan and tao12345666333 October 14, 2022 19:48

fix: add informer wait for synced

d590755

AlinsRan reviewed Dec 2, 2022

View reviewed changes

tao12345666333 added the area/controller label Dec 2, 2022

fix: Nothing labels get nothing

0bc62b5

tao12345666333 reviewed Dec 8, 2022

View reviewed changes

fix: log informer cache syned failed

e5b1a6c

lingsamuel approved these changes Dec 12, 2022

View reviewed changes

tao12345666333 approved these changes Dec 12, 2022

View reviewed changes

AlinsRan approved these changes Dec 12, 2022

View reviewed changes

tao12345666333 added this to the v1.6.0 milestone Dec 12, 2022

tao12345666333 merged commit 7e8f076 into apache:master Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: many namespace lead to provider stuck #1386

fix: many namespace lead to provider stuck #1386

shareinto commented Oct 14, 2022

AlinsRan commented Oct 15, 2022

codecov-commenter commented Oct 16, 2022

AlinsRan left a comment

AlinsRan Dec 2, 2022

shareinto Dec 2, 2022

AlinsRan Dec 2, 2022

AlinsRan Dec 2, 2022

AlinsRan Dec 2, 2022

AlinsRan Dec 2, 2022

AlinsRan Dec 2, 2022

AlinsRan commented Dec 2, 2022 •

edited

Loading

tao12345666333 Dec 8, 2022

tao12345666333 commented Dec 9, 2022

	retRoutes, err := p.apisixSharedInformerFactory.Apisix().V2beta3().ApisixRoutes().Lister().ApisixRoutes(ns).List(labels.Nothing())
	retRoutes, err := p.apisixSharedInformerFactory.Apisix().V2beta3().ApisixRoutes().Lister().ApisixRoutes(ns).List(labels.Everything())

fix: many namespace lead to provider stuck #1386

fix: many namespace lead to provider stuck #1386

Conversation

shareinto commented Oct 14, 2022

Type of change:

What this PR does / why we need it:

Pre-submission checklist:

AlinsRan commented Oct 15, 2022

codecov-commenter commented Oct 16, 2022

Codecov Report

AlinsRan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlinsRan commented Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

tao12345666333 commented Dec 9, 2022

AlinsRan commented Dec 2, 2022 •

edited

Loading