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

Add Windows Service initialisation support for Cloud Node Manager #823

Merged
merged 4 commits into from Sep 29, 2021

Conversation

JoelSpeed
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds support for running the Cloud Node Manager as a Windows Service. This is useful in scenarios where you cannot run the node manager via a daemonset, eg for security or licensing reasons.

In windows builds only, a new flag, defaulted to false, determines whether or not the windows binary should run as a windows service initialiser rather than running as a normal windows exe.

Which issue(s) this PR fixes:

Fixes #800

Special notes for your reviewer:

This is mainly copied from the kube-proxy implementation linked in the original issue. I have yet to get this tested as I don't have access to windows right now. Will make sure to get this appropriately tested before we merge.

Does this PR introduce a user-facing change?

Adds support for the Cloud Node Manager to run as a Windows Service. This can be enabled using the --windows-service flag.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 28, 2021
@coveralls
Copy link

coveralls commented Sep 28, 2021

Coverage Status

Coverage remained the same at 80.619% when pulling e09d2b4 on JoelSpeed:windows-service into 8305572 on kubernetes-sigs:master.

Comment on lines +219 to +223
# strip the following build constraints/tags:
# //go:build
# // +build \n\n
regexs["go_build_constraints"] = re.compile(
r"^(// \+build.*\n)+\n", re.MULTILINE)
r"^(//(go:build| \+build).*\n)+\n", re.MULTILINE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This update was copied from the head of K/K, with the new go:build style tags, this update is required

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@sebsoto
Copy link

sebsoto commented Sep 28, 2021

Tried this out and it seems to work properly

PS C:\Users\capi> sc.exe create "cloud-node-manager" binPath="C:\Users\capi\azure-cloud-node-manager.exe --node-name=winworker-7zql9 --wait-routes=true --kubeconfig=/k/kubeconfig --windows-service --log_dir=/var/log/cloud-node-manager --logtostderr=false"
[SC] CreateService SUCCESS
PS C:\Users\capi> sc.exe start cloud-node-manager

SERVICE_NAME: cloud-node-manager
        TYPE               : 10  WIN32_OWN_PROCESS
        STATE              : 2  START_PENDING
                                (NOT_STOPPABLE, NOT_PAUSABLE, IGNORES_SHUTDOWN)
        WIN32_EXIT_CODE    : 0  (0x0)
        SERVICE_EXIT_CODE  : 0  (0x0)
        CHECKPOINT         : 0x0
        WAIT_HINT          : 0x7d0
        PID                : 6864
        FLAGS              :
PS C:\Users\capi> cat C:\var\log\cloud-node-manager\azure-cloud-node-manager.exe.INFO
Log file created at: 2021/09/28 20:26:31 
Running on machine: winworker-7zql9
Binary: Built with gc go1.16.3 for windows/amd64
Log line format: [IWEF]mmdd hh:mm:ss.uuuuuu threadid file:line] msg
I0928 20:26:31.520719    6864 service.go:60] Running cloud-node-manager as a Windows service!
I0928 20:26:31.555859    6864 nodemanager.go:102] Version: v1.1.0-85-g83055726a
I0928 20:26:31.555859    6864 nodemanager.go:103] NodeName: winworker-7zql9
I0928 20:26:31.521255    6864 service.go:70] Service running
I0928 20:26:31.566593    6864 nodemanager.go:136] Sending events to api server.
I0928 20:26:31.566593    6864 nodemanager.go:150] Started cloud-node-manager

@feiskyer
Copy link
Member

@JoelSpeed thanks for the quick work.

@feiskyer
Copy link
Member

@JoelSpeed as we only release container images today, would you expect the windows binary be also released in the future?

@feiskyer
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, JoelSpeed

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2021
@feiskyer feiskyer added this to the v1.2 milestone Sep 29, 2021
@k8s-ci-robot k8s-ci-robot merged commit 0e8ef48 into kubernetes-sigs:master Sep 29, 2021
@JoelSpeed
Copy link
Contributor Author

@JoelSpeed as we only release container images today, would you expect the windows binary be also released in the future?

It's not strictly required for us, we will build it ourselves, but yes, I would expect it to be an option to have an officially built binary for the node manager at least

@JoelSpeed JoelSpeed deleted the windows-service branch October 17, 2021 11:08
@feiskyer
Copy link
Member

@JoelSpeed thanks for the feedbacks. We would upload the binaries as well in the future releases.

@MartinForReal
Copy link
Contributor

/cherrypick release-1.1

@k8s-infra-cherrypick-robot

@MartinForReal: #823 failed to apply on top of branch "release-1.1":

Applying: Implement Windows Service Initialisation for Cloud Node Manager
Using index info to reconstruct a base tree...
M	cmd/cloud-node-manager/app/options/options.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/cloud-node-manager/app/options/options.go
CONFLICT (content): Merge conflict in cmd/cloud-node-manager/app/options/options.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Implement Windows Service Initialisation for Cloud Node Manager
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support running Cloud Node Manager as a Windows Service
7 participants