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

Remove WORKAROUND_ISSUE_622 #3974

Closed
wants to merge 2 commits into from
Closed

Conversation

apostasie
Copy link
Contributor

On top of #3973

We no longer test rootlesskit v1 + slirp4netns (actually, we haven't for a while), so this hack is presumably no longer necessary.

@AkihiroSuda
Copy link
Member

We no longer test rootlesskit v1 + slirp4netns

We no longer test slirp4netns port forwarder, but we still test the slirp4netns network driver, which is allegedly related to #622

@AkihiroSuda AkihiroSuda closed this Mar 4, 2025
@apostasie
Copy link
Contributor Author

apostasie commented Mar 4, 2025

#622 (comment)

Didn't you say it would only happen with rootlesskit v1?

@AkihiroSuda AkihiroSuda reopened this Mar 4, 2025
strategy:
fail-fast: false
matrix:
include:
- ubuntu: 22.04
containerd: v1.7.25
containerd: v1.6.36
rootlesskit: v1.1.1 # Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

We still have RootlessKit v1 here, so the workaround still has to be enabled for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have rootlesskit v1, but we do not test slirp with it.

The only target we are testing slirp for is https://github.com/containerd/nerdctl/pull/3974/files#diff-faff1af3d8ff408964a57b2e475f69a6b7c7b71c9978cccc8f471798caac2c88R242-R246

Copy link
Member

Choose a reason for hiding this comment

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

There are two kinds of the slirp4netns drivers:

  • network driver: outgoing connections
  • port driver: incoming connections

The code here still uses slirp4netns as the network driver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad.
Thanks for clarifying!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kind of curious to see it fail though. Can we leave this PR open long enough for the full CI run?
Then I'll close it.

@apostasie
Copy link
Contributor Author

project-checks breakage is unrelated (I guess Phil just broke it :D)

@AkihiroSuda
Copy link
Member

project-checks breakage is unrelated (I guess Phil just broke it :D)

No, I broke it 🙇

Being fixed in:

@apostasie apostasie closed this Mar 4, 2025
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.

2 participants