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

5100: Adding retroactive KEP for WinDSR and WinOverlay #5112

Merged
merged 12 commits into from
Feb 12, 2025
Merged

5100: Adding retroactive KEP for WinDSR and WinOverlay #5112

merged 12 commits into from
Feb 12, 2025
+1,063 −0

Conversation

marosset
Copy link
Contributor

  • One-line PR description: Retroactive KEP for WinDSR and WinOverlay
  • Other comments:

/milestone v1.33
/sig windows network
/wip

@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 30, 2025
@k8s-ci-robot k8s-ci-robot added this to the v1.33 milestone Jan 30, 2025
@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 30, 2025
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 30, 2025
@marosset marosset changed the title WIP: 5100: Adding retroactive KEP for WinDSR and WinOverlay 5100: Adding retroactive KEP for WinDSR and WinOverlay Feb 3, 2025
@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 Feb 3, 2025
@marosset marosset mentioned this pull request Feb 3, 2025
Open
5 tasks
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 4, 2025
soltysh
soltysh reviewed Feb 10, 2025
View reviewed changes
keps/prod-readiness/sig-windows/5100.yaml Outdated Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Outdated Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Outdated Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Outdated Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Outdated Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md
are missing a bunch of machinery and tooling and can't do that now.
-->

For DSR support yes, manual verification was done to ensure that DSR can be enabled and disabled on a node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please describe the manual testing performed, see https://github.com/kubernetes/enhancements/blob/master/keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs/README.md#were-upgrade-and-rollback-tested-was-the-upgrade-downgrade-upgrade-path-tested as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated - PTAL

keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md
- when the KEP was retired or superseded
-->

## Drawbacks
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

aravindhp
aravindhp reviewed Feb 11, 2025
View reviewed changes
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Outdated Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md

#### GA

- 2 or more CNI solutions support overlay networking mode for Windows nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know OpenShift uses https://github.com/containernetworking/plugins that supports Windows overlay networking. Not sure if that counts here.

keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Show resolved Hide resolved
soltysh
soltysh reviewed Feb 11, 2025
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

Tests and deps are the remaining bits.

keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Outdated Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Outdated

For overlay, no, because the feature requires the cluster to be configured for overlay networking mode and cannot be enabled on a per-node basis.

For DSR, no, but they can be added.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please. Even unit or integration tests are better than none.

keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Show resolved Hide resolved
marosset and others added 4 commits February 11, 2025 13:53
@marosset @avin3sh @aravindhp
Apply suggestions from code review
7650020 
Co-authored-by: Avinesh Singh <[email protected]>
Co-authored-by: Aravindh Puthiyaparambil <[email protected]>
@marosset
Updates
01723ab
@marosset
spelling
5944887
@marosset
fixup
b85c91d
soltysh
soltysh approved these changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/approve
PRR

keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Show resolved Hide resolved
keps/sig-windows/5100-windows-dsr-and-overlay-support/README.md Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Feb 12, 2025
@MikeZappa87
Copy link

MikeZappa87 commented Feb 12, 2025 •
edited
Loading

/lgtm
/hold

Waiting for Windows team

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2025
@k8s-ci-robot k8s-ci-robot merged commit d96f94f into kubernetes:master Feb 12, 2025
4 checks passed
@marosset
Copy link
Contributor Author

Sigh, not sure why this merged... the /hold should have kep this open?

@marosset
Copy link
Contributor Author

I'll open another PR soon to fix up the check boxes in the KEP and also to address some minor wording updates

@marosset marosset mentioned this pull request Feb 12, 2025
Merged
@marosset marosset deleted the 5100-windsr-winoverlay branch February 13, 2025 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsturtevant jsturtevant jsturtevant left review comments

@avin3sh avin3sh avin3sh left review comments

@zylxjtu zylxjtu zylxjtu left review comments

@aravindhp aravindhp aravindhp left review comments

@soltysh soltysh soltysh approved these changes

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

Assignees

@jsturtevant jsturtevant

@MikeZappa87 MikeZappa87

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.33
Development

Successfully merging this pull request may close these issues.
8 participants
@marosset @k8s-ci-robot @MikeZappa87 @soltysh @jsturtevant @avin3sh @zylxjtu @aravindhp