WIP windows support for kured by marosset · Pull Request #460 · kubereboot/kured

marosset · 2021-11-16T23:06:09Z

Signed-off-by: Mark Rossetti marosset@microsoft.com

Here is a prototype that I have working for Windows support.

A few things to note include:

This requires WindowsHostProcessContainers feature (introduced in v1.22) to be enabled for kube-apiserver and kubelet
- This also means that kubeclient/API refs must be v1.22 or greater
- WindowsHostProcessContainers is on-track for beta in v1.23
InClusterConfig does not work with HostProcessContainers today (see not and linked issue in code)
An init container is used to register a scheduled task that runs on startup to delete c:\var\run\reboot-required if it exists. This way things outside of the Windows servicing stack can write the sentinel file and expect a reboot to be triggered once.
The script to check for pending reboots will write c:\var\run\reboot-required if one of several registry keys controlled by the servicing stack is set. These keys are cleared as servicing operations are processed.
Windows escapes "'s in command line args (these interfere with command construction if not trimmed)
This PR does not address how the Windows container image should be built. I left some open questions / comments in the dockerfile / makefile for discussion tho

jackfrancis · 2021-11-16T23:59:08Z

+$cbsKey = Get-ItemProperty "HKLM:\Software\Microsoft\Windows\CurrentVersion\Component Based Servicing" -ErrorAction Ignore
+if ($cbsKey.PSObject.Properties.name -contains 'RebootPending') {
+    Write-Output "Reboot requried - Component Based Servicing:RebootPending registry value detected"
+    New-Item -Path '/var/run' -Name 'reboot-required' -ItemType File -Force


Remind me: why do we want to emulate the sentinel filepath pattern here, why not just return 0 (reboot required) if any of the two keys here have the required reboot properties?

Sure - Both of those registry keys are only cleared on the next boot if Windows servicing operations set those keys.
If a user manually sets either of those keys and then reboots this would most likely result in the node being stuck in a reboot loop.
Adding a sentinel file here seemed like a more generic approach. I can image situations where things outside of the windows servicing stack would want to trigger a reboot once (things like node-problem-detector for example).
I also feel it is important to be able to detect when there is a pending reboot due to things like Windows Update needing to reboot the node for security fixes to be applied. Having a powershell script that can detect both situations seemed a good compromise.

That makes sense. So basically the idea here is to promote the general idea of "create the sentinel file c:\var\run\reboot-required to indicate that you want a reboot.

And the init container that we're shipping is a POC for how to implement an end-to-end solution (because you need a thing to delete that file at bootup to make this strategy work).

evrardjp

I didn't fully review, but I feel like we can improve the code readability by implementing interfaces and the right types.

evrardjp · 2021-11-24T15:30:36Z

 }

 func main() {
+	os.Stdout.WriteString(fmt.Sprintf("Command line args: %v\n", os.Args))


do we need this line? Do we need it in main?

No, sorry these were to help me figure out an issue related to microsoft/hcsshim#1207.
I'll remove all of these.

Well, you found it useful to have flags displayed, so we should probably address that in a later patch :)

evrardjp · 2021-11-24T15:33:12Z

+		// On Windows kubed daemonset needs to run in a HostProcess container which
+		// run all processes in the hosts process namespace.
+		return command
+	} else {


Shouldn't we start to build an interface instead, and make sure the linker/user input allows us to infer the type of command to run?

evrardjp · 2021-11-24T15:37:32Z

+	var config *rest.Config
+	var err error
+
+	if runtime.GOOS == windows {


If we implement interface with windows/linux k8s servers as types, then we could have a method for getting the config based on those. It would look simpler and simpler to test.

I'll look into building an interface to for windows/linux behaviors so we can remove the GOOS if/else statements in the code.
I wanted to cleanup some other parts of this PR first (like building the container images with make targets) but that is done.

evrardjp · 2021-11-24T15:37:52Z

 // buildSentinelCommand creates the shell command line which will need wrapping to escape
 // the container boundaries
-func buildSentinelCommand(rebootSentinelFile string, rebootSentinelCommand string) []string {
+func buildSentinelCommand(rebootSentinelFile string, rebootSentinelCommand string, GOOS string) []string {


see comment above.

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

…r buildx

… paths and use the correct service account Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset · 2021-12-15T22:09:40Z


 var (
 	version = "unreleased"
+	windows = "windows"


I'll remove this

marosset · 2021-12-15T22:14:31Z

+	"k8s.io/client-go/tools/clientcmd"
+)
+
+type osSpecificBehaviors interface {


I'm open to suggestions for better names for any of the objects in this file :-/

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

rchaganti

Adding a couple of comments around Test-PendingReboot.ps1 to ensure the code written follows the PowerShell best practices.

rchaganti · 2022-01-24T09:11:51Z

+
+$autoUpdateKey = Get-ItemProperty "HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\WindowsUpdate\Auto Update" -ErrorAction Ignore
+if ($autoUpdateKey.PSObject.Properties.name -contains 'RebootRequired') {
+    Write-Output "Reboot required - Auto Update:RebootRequired registry value detected"


Write-Output returns objects to the pipeline. We do not use it in a script unless that behavior is expected. Instead, you can use Write-Information.

PS C:\scripts> .\Test-PendingReboot.ps1
Reboot not required

rchaganti · 2022-01-24T09:16:43Z

+
+if (Test-Path -Path '/var/run/reboot-required') {
+    Write-Output "Reboot required"
+    exit 0


if your intention is to return 0 or 1, you must use return keyword and not exit. exit kills the PowerShell session where this function runs.

PS C:\scripts> $return = .\Test-PendingReboot.ps1 PS C:\scripts> $return Reboot not required

If you see in this, the return value from the script is not 0 or 1. Instead, it returned Reboot not required. If you replace the Write-Output with Write-Information and exit with return, here is what you will see.

PS C:\scripts> $return = .\Test-PendingReboot.ps1 PS C:\scripts> $return 1

github-actions · 2022-03-26T02:00:33Z

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

github-actions · 2022-05-26T02:13:37Z

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

github-actions · 2022-07-26T02:16:58Z

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

ckotzbauer · 2022-07-26T05:14:49Z

@marosset Are there any news on this? Is this still WIP?

github-actions · 2022-09-25T02:14:27Z

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

marosset mentioned this pull request Nov 16, 2021

Compatibility with Windows #458

Open

jackfrancis reviewed Nov 16, 2021

View reviewed changes

Comment thread cmd/kured/main.go Outdated

jackfrancis reviewed Nov 16, 2021

View reviewed changes

evrardjp requested changes Nov 24, 2021

View reviewed changes

marosset added 2 commits December 8, 2021 15:09

WIP windows support for kured

f9f33f7

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

Creating a multiarch image (and including windows images) using docke…

2d9b5c3

…r buildx

marosset force-pushed the windows-support branch from ee6eafc to 2d9b5c3 Compare December 8, 2021 23:20

ckotzbauer mentioned this pull request Dec 11, 2021

[WIP] ARM-Support #481

Closed

marosset added 3 commits December 13, 2021 21:53

Updating how cluster config is gotten for windows to avoid hard-coded…

d4193a5

… paths and use the correct service account Signed-off-by: Mark Rossetti <marosset@microsoft.com>

Misc PR feedback

d3b1762

Moving OS secific behaviors behind an interface

22e8885

marosset commented Dec 15, 2021

View reviewed changes

Comment thread cmd/kured/main.go Outdated

var (

version = "unreleased"

windows = "windows"

Copy link
Copy Markdown

Author

marosset Dec 15, 2021

Choose a reason for hiding this comment

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

I'll remove this

marosset commented Dec 15, 2021

View reviewed changes

Updating deployment files / helm chart to add windows support

7d5f68c

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

rchaganti reviewed Jan 24, 2022

View reviewed changes

github-actions Bot added the no-pr-activity label Mar 26, 2022

ckotzbauer added keep This won't be closed by the stale bot. and removed no-pr-activity labels Mar 26, 2022

github-actions Bot added the no-pr-activity label May 26, 2022

ckotzbauer removed the no-pr-activity label May 26, 2022

github-actions Bot added the no-pr-activity label Jul 26, 2022

ckotzbauer removed the no-pr-activity label Jul 26, 2022

github-actions Bot added the no-pr-activity label Sep 25, 2022

github-actions Bot closed this Oct 16, 2022

Conversation

marosset commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evrardjp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchaganti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 26, 2022

Uh oh!

github-actions Bot commented May 26, 2022

Uh oh!

github-actions Bot commented Jul 26, 2022

Uh oh!

ckotzbauer commented Jul 26, 2022

Uh oh!

github-actions Bot commented Sep 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marosset commented Nov 16, 2021 •

edited

Loading