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.

Sign up for GitHub

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

Jump to bottom

Windows hostnetwork alpha #112961

Merged
merged 4 commits into from Nov 7, 2022
Merged

Windows hostnetwork alpha #112961

merged 4 commits into from Nov 7, 2022

Conversation

marosset
Copy link
Contributor

@marosset marosset commented Oct 10, 2022 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Allows kubelet to specific if Windows pods should be joined to the node or pod network namespace for Windows pods

Which issue(s) this PR fixes:

Part of: kubernetes/enhancements#3503

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adding alpha support for WindowsHostNetworking feature

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-windows/3503-host-network-support-for-windows-pods

/sig windows node
/area kubelet

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. sig/windows Categorizes an issue or PR as relevant to SIG Windows. sig/node Categorizes an issue or PR as relevant to SIG Node. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 10, 2022
aojea
aojea reviewed Oct 10, 2022
View reviewed changes
pkg/kubelet/kuberuntime/kuberuntime_sandbox.go
@@ -233,6 +233,15 @@ func (m *kubeGenericRuntimeManager) generatePodSandboxWindowsConfig(pod *v1.Pod)
SecurityContext: &runtimeapi.WindowsSandboxSecurityContext{},
}

if utilfeature.DefaultFeatureGate.Enabled(features.WindowsHostNetwork) {
Copy link
Member

Choose a reason for hiding this comment

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

just curiosity, what is the relation with features.WindowsHostProcessContainers?

are all WindowsHostProcessContainers HostNetwork pods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, All pods that contain HostProcessContainers must set hostNetwork=true (this is enforced at API admission).
Really HostProcessContainers just start processes on the host but they are packages and deployed like contianers (hence the name).

With these changes pods that do not contain HostProcessContainers will have the option of using pod or node network namespace.

@marosset marosset mentioned this pull request Oct 11, 2022
Open
4 tasks
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2022
@marosset
Adding WindowsHostNetwork feature gate
c916f1a 
Signed-off-by: Mark Rossetti <marosset@microsoft.com>
@marosset
CRI-API updates for windows host networking
a7bafa1 
Signed-off-by: Mark Rossetti <marosset@microsoft.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2022
@marosset marosset changed the title WIP: Windows hostnetwork alpha Windows hostnetwork alpha Nov 3, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2022
@marosset
Copy link
Contributor Author

marosset commented Nov 3, 2022

/milestone v1.26
/triage accepted
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Nov 3, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 3, 2022
@k8s-ci-robot k8s-ci-robot removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 3, 2022
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 3, 2022
@marosset
Copy link
Contributor Author

marosset commented Nov 3, 2022 •
edited

I have changes mostly working end-to-end based on https://github.com/marosset/containerd/tree/windows-host-network-support so I think we can get this merged into K8s for v1.26!

/assign @jsturtevant @mrunalp @jayunit100

@marosset
Copy link
Contributor Author

marosset commented Nov 4, 2022

/assign @dchen1107

@marosset
Copy link
Contributor Author

marosset commented Nov 4, 2022

/retest

@derekwaynecarr
Copy link
Member

This PR looks fine.

Noting per the enhancement, we will need e2e tests, but you need the CRI changes merged from this PR to make that possible.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, marosset

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2022
@k8s-ci-robot k8s-ci-robot merged commit 2ef0003 into kubernetes:master Nov 7, 2022
SIG Node PR Triage automation moved this from Triage to Done Nov 7, 2022
@marosset marosset deleted the windows-hostnetwork-alpha branch November 7, 2022 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea aojea left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@feiskyer feiskyer Awaiting requested review from feiskyer

Assignees

@mrunalp mrunalp

@jsturtevant jsturtevant

@jayunit100 jayunit100

@derekwaynecarr derekwaynecarr

@dchen1107 dchen1107

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node PR Triage
Done
[sig-release] Bug Triage
Archived in project
[sig-windows] PR tracking
Status: Done
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@marosset @derekwaynecarr @k8s-ci-robot @aojea @mrunalp @jsturtevant @jayunit100 @dchen1107