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 "host-gw" & "vxlan" support #1042

Merged
merged 2 commits into from
Nov 8, 2018
Merged

Windows "host-gw" & "vxlan" support #1042

merged 2 commits into from
Nov 8, 2018

Conversation

thxCode
Copy link
Contributor

@thxCode thxCode commented Sep 24, 2018

Description

A patch PR for #921 & #922

Todos

  • Tests

@thxCode thxCode changed the title Windows support Windows "host-gw" & "vxlan" support Sep 24, 2018
@thxCode thxCode force-pushed the windows_support branch 3 times, most recently from 5fb56d3 to 2eee498 Compare September 24, 2018 06:49
@thxCode
Copy link
Contributor Author

thxCode commented Sep 24, 2018

@rakelkar

  • rebase to master
  • update deps

@rakelkar
Copy link
Contributor

@madhanrm to review

@song-jiang
Copy link

song-jiang commented Oct 2, 2018 •
edited
Loading

@thxCode @rakelkar I was testing my cluster setup ( AWS, Windows_Server-1803-English-Core-Containers, flannel host-gw) with PR #921 . I found management IP will not show up for newly created subnet unless X.X.X.2 endpoint been created and attached to host. I made some changes here song-jiang@d8c0eac with which the management ip issue has been fixed.

So my question is

  • Is my fix reasonable?
  • Does this PR address such issue?

Thanks.

This was referenced Oct 11, 2018
Closed
Closed
@fasaxc
Copy link

fasaxc commented Oct 17, 2018

@thxCode is this based on the work in #921 and/or #922? Your commit history doesn't include either of those PRs

ksubrmnn
ksubrmnn reviewed Oct 25, 2018
View reviewed changes
Copy link
Contributor

@ksubrmnn ksubrmnn left a comment

Choose a reason for hiding this comment

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

Does this have the latest version of hcsshim vendored? I don't see anything in the hcsshim directory

backend/hostgw/hostgw_windows.go Outdated Show resolved Hide resolved
backend/vxlan/device_windows.go Outdated Show resolved Hide resolved
@rajatchopra
Copy link
Contributor

@thxCode Can you address the feedback by @ksubrmnn ? The PR is ready to be merged and it will supercede #921 and #922.

@thxCode
Copy link
Contributor Author

thxCode commented Oct 31, 2018

@rajatchopra , addressed

@benmoss
Copy link
Contributor

benmoss commented Nov 1, 2018 •
edited
Loading

@thxCode is this based on the work in #921 and/or #922? Your commit history doesn't include either of those PRs

Yeah, I am fairly certain this is the same code just with some of the feedback from that and this thread addressed. If people care about keeping those original commits in the history I'd be happy to send another PR, but if it's all the same I'd just move ahead with this one.

@dineshgovindasamy
Copy link

@benmoss yes we are working with @rajatchopra @tomdee to merge this PR and there are some followup fixes we need to make to make this code work. Kalya can generate them as soon as this PR is merged.

ksubrmnn
ksubrmnn suggested changes Nov 2, 2018
View reviewed changes
backend/hostgw/hostgw_windows.go Outdated
log.Infof("Attached bridge endpoint %s to host successfully", bridgeEndpointName)

// 7. Enable forwarding on the host interface and endpoint
for _, interfaceIpAddress := range []string{expectedNetwork.ManagementIP, existingBridgeEndpoint.IPAddress.String()} {
Copy link
Contributor

Choose a reason for hiding this comment

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

existingBridgeEndpoint is now null since it is not in line 213

Copy link
Contributor

Choose a reason for hiding this comment

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

@thxCode Can you please take care of this feedback comment? This is ready to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rajatchopra , @ksubrmnn Addressed.

@madhanrm
Copy link
Contributor

madhanrm commented Nov 2, 2018

@thxCode Please include the contributors of #921 and #922 in your commit message as co-authors.

thxCode and others added 2 commits November 7, 2018 07:37
@thxCode @madhanrm
Prepare for Windows "host-gw" and "vxlan" support
6d72802 
    - update vendors
        (*) github.com/Microsoft/hcsshim
        (*) github.com/rakelkar/gonetsh
        (*) github.com/Microsoft/go-winio
        (*) github.com/sirupsen/logrus
        (*) github.com/buger/jsonparser
    - update Markfile

Co-authored-by: rakelkar <rakelkar@outlook.com>
Co-authored-by: madhanrm <madhanrajm@outlook.com>
@thxCode @madhanrm
Windows "host-gw" and "vxlan" support
e6fc2a2 
    - Add Windows host-gw
        (*) patch for flannel-io#921
    - Add windows vxlan
        (*) patch for flannel-io#922

Co-authored-by: rakelkar <rakelkar@outlook.com>
Co-authored-by: madhanrm <madhanrajm@outlook.com>
@thxCode
Copy link
Contributor Author

thxCode commented Nov 6, 2018

@madhanrm , Added.

@rajatchopra
Copy link
Contributor

@ksubrmnn We need a final lgtm from you.

@ksubrmnn
Copy link
Contributor

ksubrmnn commented Nov 7, 2018

@rajatchopra will update after validating

@ksubrmnn
Copy link
Contributor

ksubrmnn commented Nov 8, 2018

@rajatchopra LGTM

@rajatchopra
Copy link
Contributor

Drum roll... merge!!

@rajatchopra rajatchopra merged commit 47c5ade into flannel-io:master Nov 8, 2018
@daschott daschott mentioned this pull request Nov 21, 2018
Closed
@loganhz loganhz mentioned this pull request Mar 4, 2019
Closed
8 tasks
@thxCode thxCode deleted the windows_support branch March 4, 2019 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajatchopra rajatchopra rajatchopra left review comments

@ksubrmnn ksubrmnn ksubrmnn requested changes

Assignees
No one assigned
Labels
platform/windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@thxCode @rakelkar @song-jiang @fasaxc @rajatchopra @benmoss @dineshgovindasamy @madhanrm @ksubrmnn @tomdee