Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: Add WinDSR support #4104

Merged
merged 3 commits into from
Dec 16, 2020
Merged

feat: Add WinDSR support #4104

merged 3 commits into from
Dec 16, 2020

Conversation

AbelHu
Copy link
Member

@AbelHu AbelHu commented Dec 9, 2020

Reason for Change:

Issue Fixed:

Fixes #4079

Credit Where Due:

Does this change contain code from or inspired by another project?

  • No
  • Yes

If "Yes," did you notify that project's maintainers and provide attribution?

  • No
  • Yes

Requirements:

@AbelHu
Copy link
Member Author

AbelHu commented Dec 9, 2020

@marosset @jsturtevant @mainred Please help to take a look at this PR. Expect your comments for the code logic. I will add unit tests and do more testing after you confirm the code logic.

Copy link
Contributor

@marosset marosset left a comment

Choose a reason for hiding this comment

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

Changes / logic look good so far.
Adding the kube-proxy flags to KubeClusterCongih is the right thing to do here.

pkg/api/types.go Outdated
featureGates += ","
}
// WinOverlay must be set to false
featureGates += "'WinDSR=true','WinOverlay=false'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is WinOverlay something users can set in the apimodel?
If so we should add a check in validation.go and warn/fail if both WinOverlay and WinDSR are set and they are mutally exclusive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like we had the ability to add features gates to the Windows kube-proxy prior to this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with @jsturtevant, there is no other way to set feature gates to the Windows kube-proxy.

@AbelHu AbelHu force-pushed the win-dsr branch 3 times, most recently from bcbc306 to aa4cb59 Compare December 11, 2020 08:00
@AbelHu AbelHu changed the title feat: Add WinDSR support [Draft] feat: Add WinDSR support Dec 11, 2020
@AbelHu AbelHu marked this pull request as ready for review December 11, 2020 08:03
@AbelHu
Copy link
Member Author

AbelHu commented Dec 11, 2020

@marosset @jsturtevant @mainred I have updated the PR and it is ready for code review. Please help to take a look. Thanks.

@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #4104 (72e57b0) into master (2a7a029) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4104      +/-   ##
==========================================
+ Coverage   73.23%   73.26%   +0.03%     
==========================================
  Files         135      135              
  Lines       20625    20652      +27     
==========================================
+ Hits        15104    15131      +27     
  Misses       4547     4547              
  Partials      974      974              
Impacted Files Coverage Δ
pkg/engine/templates_generated.go 43.70% <ø> (ø)
pkg/api/converterfromapi.go 95.67% <100.00%> (+0.01%) ⬆️
pkg/api/convertertoapi.go 94.02% <100.00%> (+0.02%) ⬆️
pkg/api/types.go 92.72% <100.00%> (+0.15%) ⬆️
pkg/api/vlabs/types.go 73.04% <100.00%> (+0.19%) ⬆️
pkg/engine/template_generator.go 68.01% <100.00%> (+0.45%) ⬆️
pkg/api/defaults.go 93.41% <0.00%> (-0.02%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a7a029...72e57b0. Read the comment docs.

Copy link
Member

@mainred mainred left a comment

Choose a reason for hiding this comment

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

Do we need a doc for this, it looks good to me, but let upstream folks decide.

@AbelHu
Copy link
Member Author

AbelHu commented Dec 14, 2020

s, it looks good to me, but let upstream folks decide.

Thanks for reminder. I added a doc for enableWinDSR

@jsturtevant
Copy link
Contributor

@AbelHu do you know if we can use DSR with azure cni or another cni other than Calico? It would be nice to run tests against this configuration while Calico support is in the works.

@AbelHu
Copy link
Member Author

AbelHu commented Dec 15, 2020

@AbelHu do you know if we can use DSR with azure cni or another cni other than Calico? It would be nice to run tests against this configuration while Calico support is in the works.

From my understanding, DSR is required by calico but DSR does not require Calico. I am asking the same question whether we can enable DSR by default in Windows nodes when k8s >= 1.20

Append one more information: We tested WinDSR with Calico when disabling Calico CNI.

# Set this to one of the following values:
# - "vxlan" for Calico VXLAN networking
# - "windows-bgp" for Calico BGP networking using the Windows BGP router.
# - "none" to disable the Calico CNI plugin (so that you can use another plugin).
$env:CALICO_NETWORKING_BACKEND="none"

@marosset
Copy link
Contributor

/lgtm
@ksubrmnn @daschott can you help answer the above questions for DSR support?

@marosset
Copy link
Contributor

Spoke with @jsturtevant and we can add testing in a later PR since we want to unblock calico support.

@acs-bot
Copy link

acs-bot commented Dec 16, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AbelHu, 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

@marosset marosset merged commit da6b07b into Azure:master Dec 16, 2020
@daschott
Copy link

I think we should have more end-user validation before making it default.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable DSR support in Windows nodes.
6 participants