fix:prevent goroutine leakage for pkg/k8s/watchers

[yulng]

Use the ctx passed to ciliumClusterwideEnvoyConfigInit instead of wait.NeverStop.
reference #21913

@tommyp1ckles @aanm @joamaki

[maintainer-s-little-helper]

Commit 22a0cd3000edea030948482287392cab63031eed does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[maintainer-s-little-helper]

Commit d52e29f35457c27e90be0197d16d89a8b3abd964 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[joestringer]

Thanks for the PR. The code doesn't compile right now, so I would suggest that we keep it in draft until you are able to address the CI failures and push a fresh version that passes the initial smoke tests. Then, please mark the PR "Ready for review" and we can take a fresh look.

[maintainer-s-little-helper]

Commit ae19cbd14a1f493b62557bac6482f7d93e4b8bcb does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[yulng]

Use the ctx passed to ciliumClusterwideEnvoyConfigInit instead of wait.NeverStop. reference #21913

Thanks for the PR. The code doesn't compile right now, so I would suggest that we keep it in draft until you are able to address the CI failures and push a fresh version that passes the initial smoke tests. Then, please mark the PR "Ready for review" and we can take a fresh look.

I pushed again, but CI still failed, how to pass?
thanks

[joestringer]

Most jobs failed during image pull. The image pull details link shows that the build failed and has the details of the make target failure. It should be reproducible with make locally.

[maintainer-s-little-helper]

Commit 7c944970b3f8b39f2c9baa7d45045076ed772721 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[yulng]

Thanks for the PR. The code doesn't compile right now, so I would suggest that we keep it in draft until you are able to address the CI failures and push a fresh version that passes the initial smoke tests. Then, please mark the PR "Ready for review" and we can take a fresh look.

Ready for review
Thanks
@joestringer

[joestringer]

I won't look just yet, but just to let you know: To mark a PR ready for review, click this button at the bottom of the page:

You can remove the mention in the title, since that title will eventually become a release note.

[joestringer]

I marked this as release-note/misc since it looks like this will only ensure that certain goroutines are cleaned up during shutdown, and when that occurs the agent will exit anyway. If you think that there could be any user-facing impact from this PR, please describe how that impact could occur (ideally with some example observations from an environment where you observed this).

[yulng]

I marked this as release-note/misc since it looks like this will only ensure that certain goroutines are cleaned up during shutdown, and when that occurs the agent will exit anyway. If you think that there could be any user-facing impact from this PR, please describe how that impact could occur (ideally with some example observations from an environment where you observed this).

I refer to this #21913

[joestringer]

I clicked ready for review and renamed the PR per #22362 (comment).

[joestringer]

I will run CI, then assigned reviewers can take a look.

[joestringer]

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' hit: #22019 (94.17% similarity)

Job 'Cilium-PR-K8s-1.25-kernel-4.19' hit: #22019 (94.82% similarity)

[yulng]

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' hit: #22019 (94.17% similarity)

Job 'Cilium-PR-K8s-1.25-kernel-4.19' hit: #22019 (94.82% similarity)

thank you
now it can be merged?
@joestringer

[yulng]

I will run CI, then assigned reviewers can take a look.

@tommyp1ckles 😄😄

[tommyp1ckles]

From a function api level, I would say this behaviour might be a bit unintuitive. If I was to change the top level initK8sWatchers call to take a Context with a timeout, my expectation that that would just constrain how long it takes for the init procedure to complete, not how long the k8s controllers end up running for (i.e. if someone changed the global timeout expecting to constraint init times...)

Anyway, I see that this pattern is already being followed so otherwise it lgtm (might be worth adding a note about this behaviour)

[joestringer]

There are many failures, but the jobs have expired. I would suggest rebasing your PR against master, then we can re-run CI again and try to establish whether CI is identifying code issues with the PR or not.

[yulng]

There are many failures, but the jobs have expired. I would suggest rebasing your PR against master, then we can re-run CI again and try to establish whether CI is identifying code issues with the PR or not.

Pushed it again, all CI check pass
Thanks😄
@joestringer

[aanm]

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' hit: #21519 (90.01% similarity)

[yulng]

@joestringer
How about this PR?
Thanks

[joestringer]

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testserver-host-m4fs6

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

[joestringer]

A couple of tests were still failing, so I clicked the rebase button and re-triggered full CI. Let's see what CI says after that.

[yulng]

A couple of tests were still failing, so I clicked the rebase button and re-triggered full CI. Let's see what CI says after that.

I rebase and push again ,all CI is ok now

[joestringer]

Each time you rebase, it hides the results of the last CI run. If we cannot see the results of the CI run, then we cannot merge the PR. I will run the CI again, then we can see whether the CI passes or not. Please hold back on rebasing for now.

[joestringer]

/test