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.

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

change apipa network gw address from .1 to .2 #2933

Merged
merged 13 commits into from
Oct 23, 2024
Merged

Conversation

ZetaoZhuang
Copy link
Contributor

@ZetaoZhuang ZetaoZhuang commented Aug 20, 2024

Reason for Change:

currently cns using the same ip address "169.254.128.1" for both apipa gw and loopback adapter. This cause conflict issue when hns get restarted and not able to rehydrate the apipa endpoints. our long-term goal is to make this gw address configurable in dnc. But for a short-term fix, in cns, we want to overwrite the address to 169.254.128.2, so customer can recreate apipa endpoints by restarting the cns.
Issue Fixed:

Requirements:

Notes:

@ZetaoZhuang ZetaoZhuang requested a review from a team as a code owner August 20, 2024 05:20
@ZetaoZhuang ZetaoZhuang requested a review from nddq August 20, 2024 05:20
@rbtr
Copy link
Contributor

rbtr commented Aug 20, 2024

what's the impact of this change? is it backwards compatible?

@ZetaoZhuang
Copy link
Contributor Author

ZetaoZhuang commented Aug 21, 2024

here are some screenshots from my set up to test this change
image

vm ipconfig
image

container ipconfig
image

@rbtr
Copy link
Contributor

rbtr commented Aug 21, 2024

thanks for adding this context. what happens if this change goes out to a node where containers are already running with the old .1 address?

@ZetaoZhuang
Copy link
Contributor Author

thanks for adding this context. what happens if this change goes out to a node where containers are already running with the old .1 address?

This change only applies on new created apipa network/endpoints. It does not impact existed containers unless recreate them. @ashvindeodhar

@tamilmani1989
Copy link
Member

thanks for adding this context. what happens if this change goes out to a node where containers are already running with the old .1 address?

@ZetaoZhuang did we test this with existing clusters with old cns and new cni, with all combinations?

Copy link

github-actions bot commented Sep 7, 2024

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Sep 7, 2024
Copy link

Pull request closed due to inactivity.

@github-actions github-actions bot closed this Sep 15, 2024
@github-actions github-actions bot deleted the change_gw_address branch September 15, 2024 00:01
@ZetaoZhuang ZetaoZhuang restored the change_gw_address branch September 19, 2024 05:48
@ZetaoZhuang ZetaoZhuang reopened this Sep 19, 2024
@github-actions github-actions bot removed the stale Stale due to inactivity. label Sep 20, 2024
Copy link
Contributor

@QxBytes QxBytes left a comment

Choose a reason for hiding this comment

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

A couple clarifications:

  • how do the apipa gw and loopback adapter come up the first time with no restart if they both use 169.254.128.1?
  • is the long term fix that we get the gw address from elsewhere and cns consumes it?
  • is the hns default gw address not changeable?

cns/hnsclient/hnsclient_windows.go Outdated Show resolved Hide resolved
@ZetaoZhuang
Copy link
Contributor Author

A couple clarifications:

  • how do the apipa gw and loopback adapter come up the first time with no restart if they both use 169.254.128.1?
    hns allows this the first time with no restart
  • is the long term fix that we get the gw address from elsewhere and cns consumes it?
    yep. cns get it from dnc
  • is the hns default gw address not changeable?
    atm, right

QxBytes
QxBytes previously approved these changes Oct 17, 2024
Copy link
Contributor

@behzad-mir behzad-mir left a comment

Choose a reason for hiding this comment

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

Please add a TODO to redesign the hnsclient on windows

cns/hnsclient/hnsclient_windows.go Outdated Show resolved Hide resolved
cns/hnsclient/hnsclient_windows.go Show resolved Hide resolved
cns/hnsclient/hnsclient_windows.go Outdated Show resolved Hide resolved
cns/hnsclient/hnsclient_windows.go Outdated Show resolved Hide resolved
ashvindeodhar
ashvindeodhar previously approved these changes Oct 18, 2024
@ZetaoZhuang
Copy link
Contributor Author

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ZetaoZhuang ZetaoZhuang added this pull request to the merge queue Oct 22, 2024
Merged via the queue into master with commit 7d6ce69 Oct 23, 2024
14 checks passed
@ZetaoZhuang ZetaoZhuang deleted the change_gw_address branch October 23, 2024 01:28
ZetaoZhuang added a commit that referenced this pull request Oct 23, 2024
* change gw address to .2

* refactor and add UTs

* fix lint

* fix loopback adapter default gw address

* remote address should always be .1

* address comment

* adjust comment

* ddress comment

* fix func comment

* fix loopback adapter gw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants