Skip to content

pkg: prevent oom watcher from depending on shim pkg#11433

Merged
AkihiroSuda merged 1 commit intocontainerd:mainfrom
zhaodiaoer:main
Feb 26, 2025
Merged

pkg: prevent oom watcher from depending on shim pkg#11433
AkihiroSuda merged 1 commit intocontainerd:mainfrom
zhaodiaoer:main

Conversation

@zhaodiaoer
Copy link
Copy Markdown
Contributor

Compared with package pkg/shim, package core/events is more suitable as the dependency for the oom watcher functions(pkg/oom).

@k8s-ci-robot
Copy link
Copy Markdown

Hi @zhaodiaoer. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@fuweid fuweid changed the title [improve] prevent oom watcher depend on shim pkg. pkg: prevent oom watcher depend on shim pkg. Feb 25, 2025
@fuweid fuweid changed the title pkg: prevent oom watcher depend on shim pkg. pkg: prevent oom watcher from depending on shim pkg Feb 25, 2025
Comment thread pkg/oom/v1/v1.go
// New returns an epoll implementation that listens to OOM events
// from a container's cgroups.
func New(publisher shim.Publisher) (oom.Watcher, error) {
func New(publisher events.Publisher) (oom.Watcher, error) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we know if there was a specific reason for picking the shim.Publisher interface? Because it looks like they're not identical; the shim.Publisher also implements io.Closer;

// Publisher for events
type Publisher interface {
events.Publisher
io.Closer
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

☝️ introduced in ae87730, which also updated the NewOOMEpoller (which I think is what's this New function is now)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If Close is not called, makes sense to reduce the interface. Its harder to expand it later, but calling Close on a passed in Publisher would never be the cleanest solution anyway.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, yes (sorry I had to jump out after I posted); I was indeed looking if any of the other methods were called anywhere. I didn't spot that being the case at an initial glance. The commit was as far as I got to see if there was something I missed, but that's where I left off ❤️

@AkihiroSuda AkihiroSuda added this pull request to the merge queue Feb 26, 2025
Merged via the queue into containerd:main with commit 2841fc8 Feb 26, 2025
@AkihiroSuda
Copy link
Copy Markdown
Member

/cherry-pick release/2.0

@k8s-infra-cherrypick-robot
Copy link
Copy Markdown

@AkihiroSuda: new pull request created: #11439

Details

In response to this:

/cherry-pick release/2.0

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@thaJeztah thaJeztah added the cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch label Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch kind/refactor ok-to-test size/S

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants