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.

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

memory.events handling rework #145

Merged
merged 2 commits into from Feb 17, 2020

Conversation

Zyqsempai
Copy link
Member

@Zyqsempai Zyqsempai commented Feb 5, 2020

Small rework of the memory.events handler in order to implement OOM handler in main containerD repo.

During the memory.events handler implementations, were made few mistakes, this PR fixes them.

Signed-off-by: Boris Popovschi zyqsempai@mail.ru

Signed-off-by: Boris Popovschi <zyqsempai@mail.ru>
@Zyqsempai
Copy link
Member Author

@AkihiroSuda @crosbymichael PTAL

@@ -526,20 +526,19 @@ func (c *Manager) freeze(path string, state State) error {
}
}

func (c *Manager) MemoryEventFD() (uintptr, error) {
func (c *Manager) MemoryEventFD() (int, uint32, error) {
Copy link
Member

Choose a reason for hiding this comment

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

needs godoc

Copy link
Member

Choose a reason for hiding this comment

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

(int, uint32

why different types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Coz this is the base types for file descriptor and watch descriptor, at least syscall.InotifyRmWatch() expects it in that way.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

In the commit message or the PR description text, could you document:

  • purpose of this PR
  • how to test

Signed-off-by: Boris Popovschi <zyqsempai@mail.ru>
@Zyqsempai
Copy link
Member Author

@AkihiroSuda Added goDoc and updated PR description, related to "how to test it" i tried to add test but I wasn't able to properly simulate OOM behavior for process in the test.
Maybe you have some suggestions, how to test it properly, coz I am more than agree that we need few tests here.

@Zyqsempai
Copy link
Member Author

@AkihiroSuda Can we move forward this one, coz implementation of the OOM handler in containerD main repo depends on this one.

@AkihiroSuda
Copy link
Member

@fuweid PTAL?

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit d732e37 into containerd:master Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants