fix: watch all namespaces by default #919

nayihz · 2022-03-13T03:37:03Z

Type of change:

Bugfix
New feature provided
Improve performance
Backport patches

What this PR does / why we need it:

fix #908

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

codecov-commenter · 2022-03-13T03:39:40Z

Codecov Report

Merging #919 (d570739) into master (75098d1) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

❗ Current head d570739 differs from pull request most recent head ae028aa. Consider uploading reports for the commit ae028aa to get more accurate results

@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
- Coverage   31.90%   31.86%   -0.04%     
==========================================
  Files          72       72              
  Lines        7899     7908       +9     
==========================================
  Hits         2520     2520              
- Misses       5103     5112       +9     
  Partials      276      276

Impacted Files	Coverage Δ
pkg/ingress/compare.go	`0.00% <0.00%> (ø)`
pkg/ingress/namespace.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11bd92b...ae028aa. Read the comment docs.

nayihz · 2022-03-13T03:40:04Z

I didn't find any other documentation or other files related to this issue that need to be modified.
Correct me if I'm wrong. @tao12345666333

tao12345666333 · 2022-03-13T17:12:41Z

Maybe it should be noted in the configuration file.
This is an important change.
I think multiple reminders are important。

re-run CI

lingsamuel

Can we add e2e test for this behavior?

nayihz · 2022-03-14T09:21:52Z

Can we add e2e test for this behavior?

OK，I will try. Maybe need to take a litte more time.

tao12345666333 · 2022-03-16T01:22:23Z

Thanks! I will finish the review today.

re-run CI.

nayihz · 2022-03-16T01:22:36Z

Can't reproduce the failed test cases in my environment.

tao12345666333 · 2022-03-16T04:08:26Z

let me check it.

test/e2e/ingress/namespace.go

test/e2e/scaffold/httpbin.go

test/e2e/scaffold/scaffold.go

test/e2e/ingress/namespace.go

test/e2e/ingress/sanity.go

tao12345666333

thanks

tao12345666333 · 2022-03-18T11:33:00Z

let me re-run CI

nayihz · 2022-03-21T07:54:27Z

maybe need re-run again? 😂 @tao12345666333

gxthrj · 2022-03-21T12:58:05Z

Should check the failed case.

• Failure [71.433 seconds]
namespacing filtering
/home/runner/work/apisix-ingress-controller/apisix-ingress-controller/test/e2e/ingress/namespace.go:39
  without namespace_selector
  /home/runner/work/apisix-ingress-controller/apisix-ingress-controller/test/e2e/ingress/namespace.go:104
    all resources will be watched [It]
    /home/runner/work/apisix-ingress-controller/apisix-ingress-controller/test/e2e/ingress/namespace.go:125

    
    	Error Trace:	namespace.go:177
    	            				runner.go:113
    	            				runner.go:64
    	            				it_node.go:26
    	            				spec.go:215
    	            				spec.go:138
    	            				spec_runner.go:200
    	            				spec_runner.go:170
    	            				spec_runner.go:66
    	            				suite.go:79
    	            				ginkgo_dsl.go:238
    	            				ginkgo_dsl.go:213
    	            				e2e_test.go:26
    	Error:      	Expected nil, but got: &errors.errorString{s:"timed out waiting for the condition"}
    	Test:       	namespacing filtering without namespace_selector all resources will be watched
    	Messages:   	checking number of routes
    

    /home/runner/work/apisix-ingress-controller/apisix-ingress-controller/test/e2e/ingress/namespace.go:177

    Full Stack Trace
    github.com/apache/apisix-ingress-controller/test/e2e/ingress.glob..func7.2.3()
    	/home/runner/work/apisix-ingress-controller/apisix-ingress-controller/test/e2e/ingress/namespace.go:177 +0x9cf
    github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync(0xc000091ec0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/internal/leafnodes/runner.go:113 +0xa3
    github.com/onsi/ginkgo/internal/leafnodes.(*runner).run(0xc000091ec0, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/internal/leafnodes/runner.go:64 +0x15c
    github.com/onsi/ginkgo/internal/leafnodes.(*ItNode).Run(0xc0008671e0, 0x2106040, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/internal/leafnodes/it_node.go:26 +0x87
    github.com/onsi/ginkgo/internal/spec.(*Spec).runSample(0xc000a28870, 0x0, 0x2106040, 0xc0001628c0)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/internal/spec/spec.go:215 +0x72f
    github.com/onsi/ginkgo/internal/spec.(*Spec).Run(0xc000a28870, 0x2106040, 0xc0001628c0)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/internal/spec/spec.go:138 +0xf2
    github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpec(0xc0002ce6e0, 0xc000a28870, 0x0)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/internal/specrunner/spec_runner.go:200 +0x111
    github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpecs(0xc0002ce6e0, 0x1)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/internal/specrunner/spec_runner.go:170 +0x147
    github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).Run(0xc0002ce6e0, 0xc0002ee570)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/internal/specrunner/spec_runner.go:66 +0x117
    github.com/onsi/ginkgo/internal/suite.(*Suite).Run(0xc0001829a0, 0x7f945d065e80, 0xc0004b4d80, 0x1e52816, 0x1e, 0xc000057b00, 0x1, 0x1, 0x21468f8, 0xc0001628c0, ...)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/internal/suite/suite.go:79 +0x546
    github.com/onsi/ginkgo.runSpecsWithCustomReporters(0x2107d40, 0xc0004b4d80, 0x1e52816, 0x1e, 0xc000068f28, 0x1, 0x1, 0x0)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/ginkgo_dsl.go:238 +0x218
    github.com/onsi/ginkgo.RunSpecs(0x2107d40, 0xc0004b4d80, 0x1e52816, 0x1e, 0xac1ec19958)
    	/home/runner/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/ginkgo_dsl.go:213 +0xa7
    github.com/apache/apisix-ingress-controller/test/e2e.TestRunE2E(0xc0004b4d80)
    	/home/runner/work/apisix-ingress-controller/apisix-ingress-controller/test/e2e/e2e_test.go:26 +0x5c
    testing.tRunner(0xc0004b4d80, 0x1f209e0)
    	/opt/hostedtoolcache/go/1.16.14/x64/src/testing/testing.go:1203 +0xe5
    created by testing.(*T).Run
    	/opt/hostedtoolcache/go/1.16.14/x64/src/testing/testing.go:1248 +0x2b3

gxthrj · 2022-03-21T12:59:07Z

Re-runed E2E CI.

nayihz · 2022-03-21T15:58:48Z

I have ran the e2e-test for many times and all new test cases passed. The failed test cases can not reproduced in my local environment.

tao12345666333 · 2022-03-21T16:11:27Z

It is related to the running resource of GitHub Action.

nayihz · 2022-03-22T10:06:16Z

Maybe there are other cases did not clean up the dirty data. All cases ware passed after I delete

assert.Nil(ginkgo.GinkgoT(), s.EnsureNumApisixRoutesCreated(2), "checking number of routes")

tao12345666333 · 2022-03-22T10:22:53Z

Thanks, I'll check it

tao12345666333 · 2022-03-23T03:29:00Z

Let's move forward. Thanks!

Maybe there are other cases did not clean up the dirty data. All cases ware passed after I delete
assert.Nil(ginkgo.GinkgoT(), s.EnsureNumApisixRoutesCreated(2), "checking number of routes")

We can check this condition later

tao12345666333 requested review from lingsamuel, gxthrj and tao12345666333 March 14, 2022 01:15

tao12345666333 added this to In progress in v1.5 Planning via automation Mar 14, 2022

tao12345666333 added the area/controller label Mar 14, 2022

tao12345666333 self-assigned this Mar 14, 2022

lingsamuel approved these changes Mar 14, 2022

View reviewed changes

lingsamuel requested changes Mar 14, 2022

View reviewed changes

v1.5 Planning automation moved this from In progress to Review in progress Mar 14, 2022

nayihz marked this pull request as draft March 14, 2022 10:19

nayihz force-pushed the fix-ns-selector branch 4 times, most recently from f6453e0 to 5b2b9f7 Compare March 15, 2022 10:24

nayihz marked this pull request as ready for review March 16, 2022 01:19

tao12345666333 requested changes Mar 17, 2022

View reviewed changes

fix namespaces_selector

268562d

nayihz force-pushed the fix-ns-selector branch from 5b2b9f7 to 268562d Compare March 17, 2022 01:51

tao12345666333 requested changes Mar 17, 2022

View reviewed changes

test/e2e/ingress/namespace.go Outdated Show resolved Hide resolved

nayihz commented Mar 17, 2022

View reviewed changes

test/e2e/ingress/sanity.go Show resolved Hide resolved

nayihz requested review from tao12345666333 and lingsamuel March 17, 2022 11:29

lingsamuel approved these changes Mar 18, 2022

View reviewed changes

v1.5 Planning automation moved this from Review in progress to Reviewer approved Mar 18, 2022

tao12345666333 approved these changes Mar 18, 2022

View reviewed changes

fix: fix e2e test case assertion

f414359

nayihz force-pushed the fix-ns-selector branch from 35bc291 to f414359 Compare March 22, 2022 08:40

tao12345666333 merged commit 0a66151 into apache:master Mar 23, 2022

v1.5 Planning automation moved this from Reviewer approved to Done Mar 23, 2022

tao12345666333 added this to the 1.5.0 milestone Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: watch all namespaces by default #919

fix: watch all namespaces by default #919

nayihz commented Mar 13, 2022

codecov-commenter commented Mar 13, 2022 •

edited

nayihz commented Mar 13, 2022

tao12345666333 commented Mar 13, 2022

lingsamuel left a comment

nayihz commented Mar 14, 2022

tao12345666333 commented Mar 16, 2022

nayihz commented Mar 16, 2022 •

edited

tao12345666333 commented Mar 16, 2022

tao12345666333 left a comment

tao12345666333 commented Mar 18, 2022

nayihz commented Mar 21, 2022

gxthrj commented Mar 21, 2022

gxthrj commented Mar 21, 2022

nayihz commented Mar 21, 2022

tao12345666333 commented Mar 21, 2022

nayihz commented Mar 22, 2022

tao12345666333 commented Mar 22, 2022

tao12345666333 commented Mar 23, 2022

fix: watch all namespaces by default #919

fix: watch all namespaces by default #919

Conversation

nayihz commented Mar 13, 2022

Type of change:

What this PR does / why we need it:

Pre-submission checklist:

codecov-commenter commented Mar 13, 2022 • edited

Codecov Report

nayihz commented Mar 13, 2022

tao12345666333 commented Mar 13, 2022

lingsamuel left a comment

Choose a reason for hiding this comment

nayihz commented Mar 14, 2022

tao12345666333 commented Mar 16, 2022

nayihz commented Mar 16, 2022 • edited

tao12345666333 commented Mar 16, 2022

tao12345666333 left a comment

Choose a reason for hiding this comment

tao12345666333 commented Mar 18, 2022

nayihz commented Mar 21, 2022

gxthrj commented Mar 21, 2022

gxthrj commented Mar 21, 2022

nayihz commented Mar 21, 2022

tao12345666333 commented Mar 21, 2022

nayihz commented Mar 22, 2022

tao12345666333 commented Mar 22, 2022

tao12345666333 commented Mar 23, 2022

codecov-commenter commented Mar 13, 2022 •

edited

nayihz commented Mar 16, 2022 •

edited