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: remove PowerShell from Windows registry interactions #2993

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rbtr
Copy link
Contributor

@rbtr rbtr commented Sep 5, 2024

Reason for Change:
PowerShell is heavy (+30mb of memory footprint per invocation). We don't need to use it to interact with the Windows Registry (or ServiceManager) - the Go stdlib has good native bindings that should use instead.

Notes:
This functionality was introduced in #1306. In #2315, it was changed to conditionally set the reg key value only if the HNS path exists. Why? It seems simpler and harmless to always try to set the reg key, so that's what I have done. Unclear if this is problematic - cc @ZetaoZhuang

Supersedes and closes #2974 and #2961 (and #2949)

@rbtr rbtr requested review from a team as code owners September 5, 2024 21:45
@rbtr rbtr added cns Related to CNS. fix Fixes something. release/latest Change affects latest release train needs-backport Change needs to be backported to previous release trains labels Sep 5, 2024
@rbtr rbtr self-assigned this Sep 5, 2024
@rbtr rbtr force-pushed the fix/depwsh branch 5 times, most recently from 6cf43ba to 3496998 Compare September 6, 2024 16:23
@rbtr rbtr enabled auto-merge September 6, 2024 17:03
@rbtr rbtr disabled auto-merge September 6, 2024 17:03
@ZetaoZhuang
Copy link
Contributor

In #2315, it was changed to conditionally because there was a livesite that our customer got error during the cns start up to set SdnRemoteArpMacAddress. somehow hns was not enabled on their machines so we need to skip this for them.

@rbtr
Copy link
Contributor Author

rbtr commented Sep 6, 2024

@ZetaoZhuang is it okay to try to set it? Or can we not even try to set it if HNS is not enabled?

@ZetaoZhuang
Copy link
Contributor

@ZetaoZhuang is it okay to try to set it? Or can we not even try to set it if HNS is not enabled?

from that livesite, if we set it when hns is not enabled, we will get error complaining about something like "hns state path does not exist" or so. that is the reason why we made it conditional. I believe we could set it if it does not complain anymore.

platform/os_windows.go Outdated Show resolved Hide resolved
ZetaoZhuang
ZetaoZhuang previously approved these changes Sep 12, 2024
Copy link
Contributor

@ZetaoZhuang ZetaoZhuang left a comment

Choose a reason for hiding this comment

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

lgtm

@rbtr
Copy link
Contributor Author

rbtr commented Sep 18, 2024

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rbtr
Copy link
Contributor Author

rbtr commented Sep 23, 2024

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
@rbtr
Copy link
Contributor Author

rbtr commented Sep 25, 2024

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

platform/os_windows.go Outdated Show resolved Hide resolved
platform/os_windows.go Outdated Show resolved Hide resolved
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
@rbtr
Copy link
Contributor Author

rbtr commented Sep 25, 2024

/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 Sep 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 26, 2024
@rbtr rbtr added this pull request to the merge queue Sep 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 27, 2024
@rbtr rbtr added this pull request to the merge queue Sep 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 28, 2024
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. needs-backport Change needs to be backported to previous release trains release/latest Change affects latest release train
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants