Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

operator, k8s: Prevent CEC watcher goroutine leak #24316

Merged
merged 1 commit into from Mar 28, 2023
Merged

operator, k8s: Prevent CEC watcher goroutine leak #24316

merged 1 commit into from Mar 28, 2023

Conversation

yulng
Copy link
Contributor

@yulng yulng commented Mar 12, 2023

fix:prevent goroutine leakage

@yulng yulng requested review from a team as code owners March 12, 2023 13:22
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 12, 2023
@yulng yulng requested a review from christarazi March 12, 2023 13:22
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 12, 2023
@jrajahalme
Copy link
Member

/test

christarazi
christarazi requested changes Mar 13, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide justification as to why this change is needed? It's not clear how there is a goroutine leak. The K8s watchers will always be running forever until Cilium shuts down, so moving to a context here doesn't seem to have any impact.

@yulng
Copy link
Contributor Author

yulng commented Mar 14, 2023

Can you provide justification as to why this change is needed? It's not clear how there is a goroutine leak. The K8s watchers will always be running forever until Cilium shuts down, so moving to a context here doesn't seem to have any impact.

I refer to this #21913

@christarazi
Copy link
Member

Can you check it against this comment?

@yulng
Copy link
Contributor Author

yulng commented Mar 14, 2023

Can you check it against this comment?

ok,I thought it was all
i closed it later

@tommyp1ckles
Copy link
Contributor

/test-1.25-4.19

@tommyp1ckles
Copy link
Contributor

tommyp1ckles commented Mar 15, 2023 •
edited

Can you provide justification as to why this change is needed? It's not clear how there is a goroutine leak. The K8s watchers will always be running forever until Cilium shuts down, so moving to a context here doesn't seem to have any impact.

For testing, its probably good to ensure everything shuts down correctly.

Edit: just saw the comment you linked

Anyway, would be worth documenting the reasoning for the change in the commit.

@tommyp1ckles
Copy link
Contributor

Changes make sense to me.

@sayboras sayboras added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Mar 18, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 18, 2023
@sayboras
Copy link
Member

I have no other comment :)

@youngnick
Copy link
Contributor

@yulng Were you going to work on this one some more, or should it be closed?

If you are still interested in it, it needs:

  • a commit message update to make clear what's happening
  • a response to @christarazi's feedback above
  • a rebase from master.

@yulng
Copy link
Contributor Author

yulng commented Mar 22, 2023

@yulng Were you going to work on this one some more, or should it be closed?

If you are still interested in it, it needs:

  • a commit message update to make clear what's happening
  • a response to @christarazi's feedback above
  • a rebase from master.

comment

Christarazi's comment only talks about test/controlplane ,but i searched all and modified it
if i rebase and push again ,is there any hope to merge ?

@youngnick
Copy link
Contributor

@yulng I think if you change your commit message to include some information about how this will affect mainly the tests, as in the comment linked above, then rebase, we should be good to go.

@yulng
Copy link
Contributor Author

yulng commented Mar 23, 2023

@yulng I think if you change your commit message to include some information about how this will affect mainly the tests, as in the comment linked above, then rebase, we should be good to go.

ok,done

christarazi
christarazi requested changes Mar 23, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the commit msg title to:

operator, k8s: Prevent CEC watcher goroutine leak

@christarazi christarazi added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. and removed release-note/bug This PR fixes an issue in a previous release of Cilium. labels Mar 23, 2023
@christarazi christarazi added area/operator Impacts the cilium-operator component area/servicemesh GH issues or PRs regarding servicemesh labels Mar 23, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 23, 2023
@christarazi christarazi added kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact. and removed area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels Mar 23, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 23, 2023
@yulng
Copy link
Contributor Author

yulng commented Mar 24, 2023

Can you update the commit msg title to:

operator, k8s: Prevent CEC watcher goroutine leak

It's ok
Thanks

@christarazi christarazi changed the title fix:prevent goroutine leakage operator, k8s: Prevent CEC watcher goroutine leak Mar 24, 2023
@yulng @christarazi
operator, k8s: Prevent CEC watcher goroutine leak
edc8af1 
Use the ctx passed to instead of wait.NeverStop for prevent goroutine leakage.

Signed-off-by: yulng <wei.yang@daocloud.io>
christarazi
christarazi approved these changes Mar 24, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix and the PR!

For future submissions, we ask for justification upfront so that we can understand why you are submitting a change. This way, we can review and validate the fix with ease. For bugfixes, this is especially important, along with how you were able to find the issue.

@christarazi
Copy link
Member

/test

@julianwiedmann
Copy link
Member

net-next failed to start, let's give it another try ...

@julianwiedmann
Copy link
Member

/test-1.26-net-next

@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 28, 2023
@julianwiedmann julianwiedmann merged commit f77041d into cilium:master Mar 28, 2023
42 checks passed
@julianwiedmann
Copy link
Member

happy CI, happy me

@yulng
Copy link
Contributor Author

yulng commented Mar 29, 2023

Thanks everyone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

@sayboras sayboras sayboras approved these changes

@youngnick youngnick Awaiting requested review from youngnick youngnick was automatically assigned from cilium/sig-servicemesh

Assignees
No one assigned
Labels
area/operator Impacts the cilium-operator component area/servicemesh GH issues or PRs regarding servicemesh kind/bug This is a bug in the Cilium logic. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@yulng @jrajahalme @christarazi @tommyp1ckles @sayboras @youngnick @julianwiedmann