Skip to content

CRI: Add host networking helper#7814

Merged
dmcgowan merged 1 commit intocontainerd:mainfrom
dcantah:hostnet-helper
Dec 15, 2022
Merged

CRI: Add host networking helper#7814
dmcgowan merged 1 commit intocontainerd:mainfrom
dcantah:hostnet-helper

Conversation

@dcantah
Copy link
Copy Markdown
Member

@dcantah dcantah commented Dec 14, 2022

We do a ton of host networking checks around the CRI plugin, all mainly doing the same thing of checking the different quirks on various platforms (for windows are we a HostProcess pod, for linux is namespace mode the right thing, darwin doesn't have CNI support etc.) which could all be bundled up into a small helper that can be re-used.

@dcantah
Copy link
Copy Markdown
Member Author

dcantah commented Dec 14, 2022

There's a comment about having shared helpers between sbserver and the standard CRI server somewhere in the codebase after the main functionality is all there, that'd be lovely for this 😆

@dcantah dcantah marked this pull request as draft December 14, 2022 09:44
We do a ton of host networking checks around the CRI plugin, all mainly
doing the same thing of checking the different quirks on various platforms
(for windows are we a HostProcess pod, for linux is namespace mode the
right thing, darwin doesn't have CNI support etc.) which could all be
bundled up into a small helper that can be re-used.

Signed-off-by: Danny Canter <[email protected]>
@dcantah dcantah marked this pull request as ready for review December 14, 2022 09:47
@dcantah dcantah added the area/cri Container Runtime Interface (CRI) label Dec 14, 2022
@mxpv
Copy link
Copy Markdown
Member

mxpv commented Dec 14, 2022

/test pull-containerd-sandboxed-node-e2e

@kzys
Copy link
Copy Markdown
Member

kzys commented Dec 15, 2022

Looks good to me. Will wait the folks @mikebrow has assigned.

Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM just a nit below

Let's open an issue for someone to write a unit test for hostNetwork() could tag the issue with help wanted for new people... with a comment to consider using generateRuntimeOptions() see helpers_test.go for example..

Copy link
Copy Markdown
Member

@MikeZappa87 MikeZappa87 left a comment

Choose a reason for hiding this comment

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

It would be great to hear the plans for container networking for darwin.

@dcantah
Copy link
Copy Markdown
Member Author

dcantah commented Dec 15, 2022

Let's open an issue for someone to write a unit test for hostNetwork() could tag the issue with help wanted for new people... with a comment to consider using generateRuntimeOptions() see helpers_test.go for example..

Will do

@dmcgowan dmcgowan merged commit a4bc380 into containerd:main Dec 15, 2022
@dcantah
Copy link
Copy Markdown
Member Author

dcantah commented Dec 15, 2022

@mikebrow #7826

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

Labels

area/cri Container Runtime Interface (CRI)

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants