Add support for binaries to run as Windows services #60144

alinbalutoiu · 2018-02-21T17:29:25Z

What this PR does / why we need it:
Add support for binaries to run as Windows services

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #59562

Special notes for your reviewer:

Release note:

kubelet and kube-proxy can now be ran as Windows services

aserdean · 2018-02-21T17:33:41Z

Acked-by: Alin Gabriel Serdean aserdean@ovn.org
/assign @bsteciuk @michmike .

bsteciuk · 2018-02-21T21:32:47Z

/sig windows

PatrickLang · 2018-02-21T22:00:54Z

@jstarks or @jhowardmsft can you take a look since you implemented something similar for dockerd?

PatrickLang · 2018-02-21T22:39:56Z

pkg/util/service/service_windows.go

+	}
+	t := []scAction{
+		{Type: scActionRestart, Delay: uint32(60 * time.Second / time.Millisecond)},
+		{Type: scActionRestart, Delay: uint32(60 * time.Second / time.Millisecond)},


I think it would be better to bump this up an order of magnitude from 1 to 10 minutes for second retry. I think that would be better for dealing with the case where a dependent service like dockerd.exe isn't running yet.

For others reviewing - here's the Windows SDK doc on how restart actions work:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms685939(v=vs.85).aspx

As written, it will retry after one minute, again after 1 more minute, then wait 24 hours before trying again

Sure we can change it. I wouldn't go for a hard dependency on dockerd.exe since kubelet will be used using a different CRI in the future.

To be honest this is a default value for service creation. The hardcoded values can substitute for a default value, but I would rather add documentation on how to config the service as a sysadmin (i.e. changing start type, adding triggers, modifying restart actions as people pleases). Do you know where we can add such a documentation?

Changed the second timeout from 1 minute to 10 minutes.

PatrickLang · 2018-02-21T22:41:08Z

I added a short comment. Thanks for submitting this and getting restart actions included!

bsteciuk

These code changes look good to me. I also built the bins and verified I was able to register/unregister kube-proxy & kubelet services, as well as do a kubeadm join after setting up the kubelet service (passes kubeadm pre-flight check validating kubelet is setup as a service)

cblecker · 2018-02-23T01:33:58Z

@alinbalutoiu You've added files to vendor without going through the correct process. As such, this will fail CI.
The proper process for adding dependencies is here: https://github.com/kubernetes/community/blob/master/contributors/devel/godep.md

alinbalutoiu · 2018-02-23T15:39:12Z

@cblecker thanks for pointing that out. Done.

cblecker · 2018-02-23T16:47:35Z

/ok-to-test

michmike · 2018-02-23T17:20:44Z

/approve
/lgtm

michmike · 2018-02-23T17:21:31Z

/assign @thockin

michmike · 2018-02-23T17:23:16Z

@alinbalutoiu please also add the status/approved-for-milestone label

alinbalutoiu · 2018-02-23T18:44:04Z

Fixed the boilerplate header for service_windows.go, added missing licenses and added comments to the exported functions from pkg/util/service package. This were causing the build to fail.

PatrickLang · 2018-03-06T18:58:41Z

/priority important-soon

PatrickLang · 2018-03-06T19:01:26Z

Alright - think labels are fixed. Just need final review closed from @mtaufen I hope?

mtaufen

Updates mostly look good, I had a few more suggestions to tighten things up.

mtaufen · 2018-03-06T19:06:18Z

cmd/kube-proxy/app/init_windows.go

+)
+
+var (
+	flRunService bool


@ncdc this global state look good? Or does kube-proxy prefer something akin to KubeletFlags?

I had put everything into the Options struct, such as CleanupAndExit:

kubernetes/cmd/kube-proxy/app/server.go

Lines 93 to 100 in 86ca3af

// Options contains everything necessary to create and run a proxy server.

type Options struct {

// ConfigFile is the location of the proxy server's configuration file.

ConfigFile string

// WriteConfigTo is the path where the default configuration will be written.

WriteConfigTo string

// CleanupAndExit, when true, makes the proxy server clean up iptables rules, then exit.

CleanupAndExit bool

And then the flag registered directly into that field:

kubernetes/cmd/kube-proxy/app/server.go

Lines 120 to 124 in 86ca3af

// AddFlags adds flags to fs and binds them to options.

func (o *Options) AddFlags(fs *pflag.FlagSet) {

fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, "The path to the configuration file.")

fs.StringVar(&o.WriteConfigTo, "write-config-to", o.WriteConfigTo, "If set, write the default configuration values to this file and exit.")

fs.BoolVar(&o.CleanupAndExit, "cleanup-iptables", o.CleanupAndExit, "If true cleanup iptables and ipvs rules and exit.")

But I realize this is OS specific and it doesn't make sense to add it to kube-proxy's Options struct, so it's probably ok like this.

FWIW, I wouldn't prefix the package variable's name with fl. I'd probably call it runAsWIndowsServer or something to that effect instead.

Done. I changed the variable name into WindowsService to be consistent with Kubelet as @mtaufen requested.

mtaufen · 2018-03-06T19:10:38Z

cmd/kubelet/app/options/osflags_windows.go

+)
+
+var (
+	FlRunService bool


This should be a subfield of the KubeletFlags struct in options/options.go. Leave a comment above the field noting that its corresponding flag only gets registered in Windows builds.

Let's also rename it to WindowsService here and in kube-proxy so it matches the flag name.

mtaufen · 2018-03-06T19:17:04Z

cmd/kubelet/app/options/osflags_windows.go

+)
+
+// AddOSFlags adds specific OS flags
+func AddOSFlags(fs *pflag.FlagSet) {


Modify this as follows when you move the destination value into KubeletFlags, so we're consistent with KubeletFlags.AddFlags:

func (f *KubeletFlags) addOSFlags(fs *pflag.FlagSet) { fs.BoolVar(&f.WindowsService, "windows-service", f.WindowsService, "Enable Windows Service Control Manager API integration") }

and now call it at the beginning of KubeletFlags.AddFlags instead of in cmd/kubelet/app/server.go (I made KubeletFlags.addOSFlags private, since there shouldn't be a reason to call this separately from KubeletFlags.AddFlags).

mtaufen · 2018-03-06T19:17:53Z

cmd/kubelet/app/server.go

@@ -238,6 +238,7 @@ HTTP server: The kubelet can also listen for HTTP and respond to a simple API
 	kubeletFlags.AddFlags(cleanFlagSet)
 	options.AddKubeletConfigFlags(cleanFlagSet, kubeletConfig)
 	options.AddGlobalFlags(cleanFlagSet)
+	options.AddOSFlags(cleanFlagSet)


merge with kubeletFlags.AddFlags, per above comment

mtaufen · 2018-03-06T19:24:52Z

cmd/kubelet/app/init_windows.go

+	serviceName = "kubelet"
+)
+
+func initForOS() error {


When you remove the global FlRunService var in favor of KubeletFlags.WindowsService, you'll have to pass it in as an arg here, e.g. func initForOS(windowsService bool) error.

I know that's not as pretty a function signature, but it's better than global state floating around and keeps things consistent with the other flags.

mtaufen

One nit, and also want @ncdc to give a final thumbs-up on the kube-proxy stuff, otherwise LGTM.

mtaufen · 2018-03-06T23:33:08Z

cmd/kube-proxy/app/server.go

@@ -100,6 +100,9 @@ type Options struct {
 	CleanupAndExit bool
 	// CleanupIPVS, when true, makes the proxy server clean up ipvs rules before running.
 	CleanupIPVS bool
+	// WindowsService should be set to true if kubelet is running as a service on Windows.


s/kubelet/kube-proxy

This patch adds support for kubernetes to integrate with Windows SCM. As a first step both `kubelet` and `kube-proxy` can be registered as a service. To create the service: PS > sc.exe create <component_name> binPath= "<path_to_binary> --service <other_args>" CMD > sc create <component_name> binPath= "<path_to_binary> --service <other_args>" Please note that if the arguments contain spaces, it must be escaped. Example: PS > sc.exe create kubelet binPath= "C:\kubelet.exe --service --hostname-override 'minion' <other_args>" CMD > sc create kubelet binPath= "C:\kubelet.exe --service --hostname-override 'minion' <other_args>" Example to start the service: PS > Start-Service kubelet; Start-Service kube-proxy CMD > net start kubelet && net start kube-proxy Example to stop the service: PS > Stop-Service kubelet (-Force); Stop-Service kube-proxy (-Force) CMD > net stop kubelet && net stop kube-proxy Example to query the service: PS > Get-Service kubelet; Get-Service kube-proxy; CMD > sc.exe queryex kubelet && sc qc kubelet && sc.exe queryex kube-proxy && sc.exe qc kube-proxy Signed-off-by: Alin Balutoiu <abalutoiu@cloudbasesolutions.com> Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> Co-authored-by: Alin Gabriel Serdean <aserdean@ovn.org>

This is needed for service manager support on Windows in kubernetes binaries.

alinbalutoiu · 2018-03-07T00:55:14Z

/test pull-kubernetes-kubemark-e2e-gce

alinbalutoiu · 2018-03-07T08:11:48Z

/test pull-kubernetes-kubemark-e2e-gce

alinbalutoiu · 2018-03-07T14:40:23Z

/test pull-kubernetes-kubemark-e2e-gce

mtaufen · 2018-03-07T17:30:11Z

I pinged @ncdc last night on Slack, and he was cool with the kube-proxy side.
Kubelet side LGTM.
/lgtm

k8s-ci-robot · 2018-03-07T17:30:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alinbalutoiu, aserdean, michmike, mtaufen, thockin

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:

~~Godeps/OWNERS~~ [thockin]
~~cmd/OWNERS~~ [thockin]
~~pkg/OWNERS~~ [thockin]
~~vendor/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-03-07T17:30:38Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-03-07T17:31:07Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@alinbalutoiu @michmike @mtaufen @thockin

Pull Request Labels

sig/windows: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/feature: New functionality.

Help

k8s-github-robot · 2018-03-07T18:33:41Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from cblecker, dchen1107, pmorie and smarterclayton February 21, 2018 17:30

k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Feb 21, 2018

PatrickLang reviewed Feb 21, 2018

View reviewed changes

alinbalutoiu force-pushed the master branch from 0459b7e to 8e2c93b Compare February 22, 2018 11:29

bsteciuk approved these changes Feb 22, 2018

View reviewed changes

alinbalutoiu force-pushed the master branch from 8e2c93b to 344e5f9 Compare February 23, 2018 13:46

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 23, 2018

k8s-ci-robot assigned michmike Feb 23, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2018

k8s-ci-robot assigned thockin Feb 23, 2018

alinbalutoiu force-pushed the master branch from 344e5f9 to 7fbcf59 Compare February 23, 2018 18:37

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2018

k8s-github-robot added milestone/incomplete-labels and removed milestone/removed labels Mar 6, 2018

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 6, 2018

k8s-github-robot removed the milestone/incomplete-labels label Mar 6, 2018

mtaufen reviewed Mar 6, 2018

View reviewed changes

alinbalutoiu force-pushed the master branch from 5e2e757 to 9d9aef5 Compare March 6, 2018 20:53

mtaufen reviewed Mar 6, 2018

View reviewed changes

alinbalutoiu and others added 2 commits March 7, 2018 00:51

Add sys/windows/svc to vendor

448ed6e

This is needed for service manager support on Windows in kubernetes binaries.

alinbalutoiu force-pushed the master branch from 9d9aef5 to 448ed6e Compare March 6, 2018 23:52

mtaufen approved these changes Mar 7, 2018

View reviewed changes

k8s-ci-robot assigned mtaufen Mar 7, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2018

k8s-github-robot merged commit b30fed0 into kubernetes:master Mar 7, 2018

PatrickLang mentioned this pull request Mar 9, 2018

Remove NSSM and use kubelet.exe and kube-proxy --service flags instead Azure/acs-engine#2419

Closed

alinbalutoiu mentioned this pull request Jun 14, 2018

move cni to cnishim/cniserver architecture ovn-kubernetes/ovn-kubernetes#354

Merged

PatrickLang mentioned this pull request Sep 14, 2018

Cleanup Windows installations Azure/acs-engine#3851

Closed

8 tasks

ptylenda mentioned this pull request Sep 21, 2018

Discussion roadmap v1.0 pablodav/kubernetes-for-windows#13

Closed

PatrickLang mentioned this pull request Sep 20, 2019

Remove NSSM dependency from Windows nodes Azure/aks-engine#2003

Closed

2 tasks

danwinship mentioned this pull request Jan 21, 2024

KEP-784: Update design-details kubernetes/enhancements#4337

Merged

	// Options contains everything necessary to create and run a proxy server.
	type Options struct {
	// ConfigFile is the location of the proxy server's configuration file.
	ConfigFile string
	// WriteConfigTo is the path where the default configuration will be written.
	WriteConfigTo string
	// CleanupAndExit, when true, makes the proxy server clean up iptables rules, then exit.
	CleanupAndExit bool

	// AddFlags adds flags to fs and binds them to options.
	func (o Options) AddFlags(fs pflag.FlagSet) {
	fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, "The path to the configuration file.")
	fs.StringVar(&o.WriteConfigTo, "write-config-to", o.WriteConfigTo, "If set, write the default configuration values to this file and exit.")
	fs.BoolVar(&o.CleanupAndExit, "cleanup-iptables", o.CleanupAndExit, "If true cleanup iptables and ipvs rules and exit.")

Add support for binaries to run as Windows services #60144

Add support for binaries to run as Windows services #60144

Conversation

alinbalutoiu commented Feb 21, 2018 • edited Loading

aserdean commented Feb 21, 2018

bsteciuk commented Feb 21, 2018

PatrickLang commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatrickLang commented Feb 21, 2018

bsteciuk left a comment

Choose a reason for hiding this comment

cblecker commented Feb 23, 2018

alinbalutoiu commented Feb 23, 2018

cblecker commented Feb 23, 2018

michmike commented Feb 23, 2018

michmike commented Feb 23, 2018

michmike commented Feb 23, 2018

alinbalutoiu commented Feb 23, 2018 • edited Loading

PatrickLang commented Mar 6, 2018

PatrickLang commented Mar 6, 2018

mtaufen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtaufen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinbalutoiu commented Mar 7, 2018

alinbalutoiu commented Mar 7, 2018

alinbalutoiu commented Mar 7, 2018

mtaufen commented Mar 7, 2018

k8s-ci-robot commented Mar 7, 2018

k8s-github-robot commented Mar 7, 2018

k8s-github-robot commented Mar 7, 2018

k8s-github-robot commented Mar 7, 2018

alinbalutoiu commented Feb 21, 2018 •

edited

Loading

alinbalutoiu commented Feb 23, 2018 •

edited

Loading