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

fix: don't restart HNS if the ARP regkey is not changed #3498

Merged
merged 1 commit into from
Mar 14, 2025

Conversation

rbtr
Copy link
Contributor

@rbtr rbtr commented Mar 12, 2025

don't restart HNS when setting SDNRemoteArpMacAddress (or at all, it's fragile).
reverts behavior changed in #3343 (timeout and retry if HNS is not responding) and #2993 (restart HNS after the regkey is set unconditionally)

@Copilot Copilot bot review requested due to automatic review settings March 12, 2025 15:34
@rbtr rbtr requested a review from a team as a code owner March 12, 2025 15:34
@rbtr rbtr requested a review from paulyufan2 March 12, 2025 15:34
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the HNS service restart functionality when setting SDNRemoteArpMacAddress to improve reliability by avoiding fragile service restarts.

  • Removed unused imports (svc and svc/mgr)
  • Removed the restartHNS function and its invocation in SetSdnRemoteArpMacAddress

@rbtr rbtr requested a review from chandanAggarwal March 12, 2025 15:35
@rbtr rbtr self-assigned this Mar 12, 2025
@rbtr rbtr added cns Related to CNS. fix Fixes something. windows labels Mar 12, 2025
@rbtr rbtr force-pushed the fix/softer-hns-restart branch from 64335c6 to 7cd7a04 Compare March 12, 2025 22:27
@rbtr rbtr changed the title fix: remove HNS restart fix: don't restart HNS if the ARP regkey is not changed Mar 12, 2025
@rbtr
Copy link
Contributor Author

rbtr commented Mar 12, 2025

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rbtr rbtr requested a review from Copilot March 12, 2025 22:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the behavior when setting the SDNRemoteArpMacAddress registry key so that the HNS service is not restarted if the registry value is already set.

  • The setSDNRemoteARPRegKey function now returns a boolean flag indicating if the key was changed.
  • The SetSdnRemoteArpMacAddress function has been updated to check this flag and conditionally skip restarting HNS.

@rbtr rbtr enabled auto-merge March 12, 2025 22:34

Verified

This commit was signed with the committer’s verified signature.
nakatanakatana nakatanakatana
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
@rbtr rbtr force-pushed the fix/softer-hns-restart branch from 7cd7a04 to 46a5d4a Compare March 13, 2025 00:02
@rbtr rbtr requested a review from a team as a code owner March 13, 2025 00:02
@rbtr
Copy link
Contributor Author

rbtr commented Mar 13, 2025

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rbtr rbtr added this pull request to the merge queue Mar 13, 2025
Merged via the queue into master with commit 2b6f593 Mar 14, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cns Related to CNS. fix Fixes something. windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants