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

[#6475] improvement: Disable the ryuk #6480

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jerqi
Copy link
Contributor

@jerqi jerqi commented Feb 19, 2025

What changes were proposed in this pull request?

Disable the ryuk

Why are the changes needed?

Fix: #6475

Does this PR introduce any user-facing change?

No.

How was this patch tested?

CI passed.

@jerqi jerqi requested a review from xunliu February 19, 2025 07:39
@jerqi jerqi marked this pull request as draft February 19, 2025 07:40
@jerqi jerqi marked this pull request as ready for review February 19, 2025 08:57
@yuqi1129
Copy link
Contributor

I don't think disabling ryuk globally is a good choice, when the rynk is disabled, developers need to clean up docker resources when testing locally. Could you please limit the scope and disable rynk in the module that needs to clean resource manually?

@jerqi
Copy link
Contributor Author

jerqi commented Feb 19, 2025

@youngyjd Maybe you can apply this commit. This may solve your issue.

@jerqi
Copy link
Contributor Author

jerqi commented Feb 19, 2025

I don't think disabling ryuk globally is a good choice, when the rynk is disabled, developers need to clean up docker resources when testing locally. Could you please limit the scope and disable rynk in the module that needs to clean resource manually?

It seems ok that the container can be cleaned normally although we disable ryuk. I tried locally.
image

@yuqi1129
Copy link
Contributor

I don't think disabling ryuk globally is a good choice, when the rynk is disabled, developers need to clean up docker resources when testing locally. Could you please limit the scope and disable rynk in the module that needs to clean resource manually?

It seems ok that the container can be cleaned normally although we disable ryuk. I tried locally. image

I see.

@youngyjd
Copy link

@jerqi seems working. not seeing errors related to permission anymore

thanks!

@tengqm
Copy link
Contributor

tengqm commented Feb 20, 2025

Better add some comments to the source code explaining why we are doing this.
Comments for cases like this are very useful because this change is actually an experiment.
This toggling should be treated as [EXPERIMENTAL].

@jerryshao
Copy link
Contributor

I agree with @tengqm that we should add comments to explain why do we need to add this configuration.

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.

[Improvement] Run integration test in rootless and non privileged mode
5 participants