WINC-815: Add support for Windows Server 2022 on GCP clusters by sebsoto · Pull Request #1197 · openshift/windows-machine-config-operator

sebsoto · 2022-08-20T14:29:50Z

This PR adds multiple commits around providing WMCO support for GCP clusters

Commits to have GCP to work with our test suite:
- [controllers] Add ignore label to filter Machines
- [e2e] Add GCP provider
Commits to add GCP functionality and docs to WMCO:
- [secrets] Enable Administrator account if needed
- [docs] Document GCP support

sebsoto · 2022-08-20T14:30:38Z

/test gcp-e2e-operator

sebsoto · 2022-08-20T14:30:43Z

/approve cancel

sebsoto · 2022-08-20T17:19:55Z

/test gcp-e2e-operator

sebsoto · 2022-08-21T00:12:52Z

/test gcp-e2e-operator

sebsoto · 2022-08-23T11:52:19Z

/test gcp-e2e-operator

aravindhp

Thanks for working on this, @sebsoto. Looks good. Just some minor comments and splitting up the controller using DNS to another PR.

aravindhp · 2022-08-23T19:23:27Z


 // GetAddress returns a non-ipv6 address that can be used to reach a Windows node. This can be either an ipv4
-// or dns address.
+// or dns address. DNS will be preferred, if available.


This change needs to be a separate PR to be on the safe side given it changes behavior across all clouds.

aravindhp · 2022-08-23T19:25:28Z

 	}

-	_, err := getInternalIPAddress(machine.Status.Addresses)
+	_, err := GetAddress(machine.Status.Addresses)


Where is GetAddress defined?

aravindhp · 2022-08-23T19:40:36Z

+				return [System.Web.Security.Membership]::GeneratePassword(16, 2)
+			}
+
+			# Check if the capi user exists, this will be the case on Azure, and will be used instead of Administrator


As an aside I wonder if we can do this on Azure also? It is also making me wonder if we should just create a new core user instead like we wanted to in the past.

We can re-open https://issues.redhat.com/browse/WINC-430
This could cause issues with people using custom images

This could cause issues with people using custom images

Why would creating a core user cause issues?

I'm thinking of a case where the Administrator user has been set up with certain permissions in the image, or theres security software installed special casing the Administrator user. Its nothing we cant doc around.

sebsoto · 2022-08-23T20:07:12Z

/test gcp-e2e-operator

aravindhp · 2022-08-23T20:14:21Z

+				return [System.Web.Security.Membership]::GeneratePassword(16, 2)
+			}
+
+			# Check if the capi user exists, this will be the case on Azure, and will be used instead of Administrator


This could cause issues with people using custom images

Why would creating a core user cause issues?

sebsoto · 2022-08-23T20:32:35Z

/test gcp-e2e-operator

aravindhp · 2022-08-23T22:13:54Z

+				# The capi user doesn't exist, ensure the Administrator account is enabled if it exists
+				# If neither users exist, an error will be written to the console, but the script will still continue
+				$UserAccount = Get-LocalUser -Name "Administrator"
+				if( ($UserAccount -ne $null) -and (!$UserAccount.Enabled) ) {


Nit extra spaces after if( and ).

mansikulkarni96 · 2022-08-24T15:13:52Z


 Not having these labels will result in the Windows node not being marked as a worker.

+If the Machine spec has the label `windowsmachineconfig.openshift.io/ignore=true`, the Machine will be ignored by WMCO,


should this information be moved to the hacking.md file? Wondering if this would be useful in the README if we don't expect users to use this other than for development and testing.

That's true, I was back and forth about this. I can move it there

We have been using the presence of the Windows label on Machines as the differentiator between Machines used to test the Windows Machine controller and those used to test the ConfigMap controller. This worked because the Windows Machine controller would filter out any Machines that did not have that label. This is now an issue as this label is now being used by the MAPI Machine controllers, indicating whether or not Windows specific steps should occur when creating the backing VM. Without this label on the Machines we are spinning up for BYOH, the VMs will not be usable. This is true now for GCP, but it may also become true for other providers. This commit introduces a new `ignore` label, this label will be used by the WindowsMachine controller to filter out Machines for reconciliation, allowing us to continue using the Machine API to create Windows Machines for BYOH testing purposes.

Enables creating MachineSets on the GCP platform Completes https://issues.redhat.com/browse/WINC-851

sebsoto · 2022-08-24T16:19:00Z

/test gcp-e2e-operator

jrvaldes · 2022-08-24T19:21:31Z

+	if tc.CloudProvider.GetType() == config.VSpherePlatformType ||
+		tc.CloudProvider.GetType() == config.GCPPlatformType || tc.CloudProvider.GetType() == config.AzurePlatformType {
 		powershellDefaultCommand = strings.ReplaceAll(command, "\\\"", "\"")


@sebsoto the cloud provider is not the driving factor here. This modification is required due to Windows Server 2022.

Consider using the existing tc.windowsServerVersion flag in the condition statement for a more accurate decision. See this commit.

For example:

if tc.windowsServerVersion != "2019" { powershellDefaultCommand = strings.ReplaceAll(command, "\\\"", "\"") }

@jrvaldes thats a good point, but I don't think that method is usable until the AWS 2022 PR is merged. If I try and do that now, the AWS 2022 jobs will fail

What I mean is the jobs that are set for use with 2022 will fail due to the wrong powershell parsing being used for them

The environment variable was introduced in openshift/release#29791, and is ready to use.

Yes but aws-e2e-ccm-install and the aws upgrade job have that env var set to ""
this will cause the test suite to detect them as running server 2022 and use the wrong powershell syntax because of that

I don't think so. the Windows Server 2019 is hard-coded.

My point is that we should stop using tc.CloudProvider.GetType() as criteria to tweak the PS commands. It was valid with AWS+WS2022 and is now a reality with GCP+WS2022.

we cannot do powershellDefaultCommand = strings.ReplaceAll(command, "\\\"", "\"") on VMs running Windows server 2019. The AWS upgrade and ccm jobs are currently running 2019, but checking that environment variable will tell us that they are running 2022.

saifshaikh48

Good stuff, just a few minor comments

saifshaikh48 · 2022-08-24T19:29:58Z

+## Preventing Windows Machines from being configured by WMCO
+
+If the Machine spec has the label `windowsmachineconfig.openshift.io/ignore=true`, the Machine will be ignored by WMCO,
+and will not be configured into a Windows node. This can be helpful when debugging userdata changes.


the Machine will be ignored by WMCO and will not be configured into a Windows node

unless it is added as an entry to the windows-instances ConfigMap, right? Since this is hack readme, might be good to call this out.

I'm avoiding calling that out specifically because I dont want anyone to assume thats okay to do.
I'll change the language to specifically call out the Machine controller will ignore the Machine though.

saifshaikh48 · 2022-08-24T19:33:00Z

+			}
+
+			# Check if the capi user exists, this will be the case on Azure, and will be used instead of Administrator
+			if((Get-LocalUser |Where-Object {$_.Name -eq "capi"})  -eq $null) {


nits: please put a space after the pipe and remove the extra space before -eq (there's 2)

jrvaldes · 2022-08-24T19:35:51Z

+
+_\<zone_suffix\>_ should be replaced with the suffix of the chosen zone, such as `a`.
+
+_\<project_id\>_ should be replaced with the GCP project this cluster was created under.


Is there a gcloud command to fetch this info? gcloud config get-value project maybe?

This could give the wrong value if the default project they have set up on their shell isn't the one that the cluster exists on

I follow, is it safe to use it in the machineset.sh script?

i think its fine to use there as that script is meant for our use, not for customers

This commit changes the Windows userdata secret to create the Administrator account if the capi user doesn't exist, and if the account is not enabled already. This is required to support GCP clusters, as GCP standard Windows images have the Administrator account disabled.

* Documents how a user can create a Windows MachineSet in a GCP cluster * Updates pre-requisites to provide information on GCP

jrvaldes · 2022-08-24T20:52:33Z

/lgtm

sebsoto · 2022-08-24T21:05:46Z

/test gcp-e2e-operator

sebsoto · 2022-08-24T23:07:51Z

/retest

openshift-ci-robot · 2022-08-25T01:15:55Z

/retest-required

Remaining retests: 2 against base HEAD cc18c77 and 8 for PR HEAD 4c2264c in total

openshift-ci-robot · 2022-08-25T03:19:58Z

/retest-required

Remaining retests: 1 against base HEAD cc18c77 and 7 for PR HEAD 4c2264c in total

openshift-ci-robot · 2022-08-25T06:52:03Z

/retest-required

Remaining retests: 0 against base HEAD cc18c77 and 6 for PR HEAD 4c2264c in total

sebsoto · 2022-08-25T11:25:50Z

/retest

sebsoto · 2022-08-25T15:19:20Z

Azure CI failing due to node logs test issues.
Opened #1214 to address this

sebsoto · 2022-08-25T20:56:29Z

/hold

sebsoto · 2022-08-26T18:03:21Z

/hold cancel

openshift-ci-robot · 2022-08-26T18:58:57Z

/retest-required

Remaining retests: 2 against base HEAD 7d47218 and 5 for PR HEAD 4c2264c in total

openshift-ci · 2022-08-27T01:48:09Z

@sebsoto: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 20, 2022

openshift-ci Bot requested review from aravindhp and mansikulkarni96 August 20, 2022 14:30

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2022

openshift-ci Bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2022

sebsoto force-pushed the gcpCIReal branch from bf85a86 to 8ede7eb Compare August 20, 2022 16:41

sebsoto force-pushed the gcpCIReal branch from 8ede7eb to 0cb5085 Compare August 20, 2022 21:53

sebsoto force-pushed the gcpCIReal branch 3 times, most recently from 38178c2 to f1b7ac4 Compare August 23, 2022 11:45

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2022

sebsoto changed the title ~~[WIP] GCP Windows Server 2022~~ WINC-815: Add support for GCP Windows Server 2022 Aug 23, 2022

sebsoto changed the title ~~WINC-815: Add support for GCP Windows Server 2022~~ WINC-815: Add support for Windows Server 2022 on GCP clusters Aug 23, 2022

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2022

sebsoto force-pushed the gcpCIReal branch from f1b7ac4 to 0b74854 Compare August 23, 2022 11:47

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2022

aravindhp suggested changes Aug 23, 2022

View reviewed changes

sebsoto force-pushed the gcpCIReal branch 2 times, most recently from 04b60a1 to d8f1ad4 Compare August 23, 2022 20:04

aravindhp suggested changes Aug 23, 2022

View reviewed changes

sebsoto force-pushed the gcpCIReal branch from d8f1ad4 to 5d16c41 Compare August 23, 2022 20:30

aravindhp suggested changes Aug 23, 2022

View reviewed changes

sebsoto force-pushed the gcpCIReal branch from 5d16c41 to e18a593 Compare August 24, 2022 11:52

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2022

mansikulkarni96 reviewed Aug 24, 2022

View reviewed changes

sebsoto added 2 commits August 24, 2022 11:34

[e2e] Add GCP provider

d155e48

Enables creating MachineSets on the GCP platform Completes https://issues.redhat.com/browse/WINC-851

sebsoto force-pushed the gcpCIReal branch from e18a593 to be64e47 Compare August 24, 2022 15:34

jrvaldes reviewed Aug 24, 2022

View reviewed changes

saifshaikh48 reviewed Aug 24, 2022

View reviewed changes

jrvaldes reviewed Aug 24, 2022

View reviewed changes

sebsoto added 2 commits August 24, 2022 15:48

[docs] Document GCP support

4c2264c

* Documents how a user can create a Windows MachineSet in a GCP cluster * Updates pre-requisites to provide information on GCP

sebsoto force-pushed the gcpCIReal branch from be64e47 to 4c2264c Compare August 24, 2022 19:54

openshift-ci Bot assigned jrvaldes Aug 24, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2022

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2022

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 26, 2022

openshift-merge-robot merged commit 44fc3ac into openshift:master Aug 27, 2022

jrvaldes mentioned this pull request Sep 4, 2022

[bundle] Add support for GCP in CSV #1236

Merged


		Not having these labels will result in the Windows node not being marked as a worker.

		If the Machine spec has the label `windowsmachineconfig.openshift.io/ignore=true`, the Machine will be ignored by WMCO,


		_\<zone_suffix\>_ should be replaced with the suffix of the chosen zone, such as `a`.

		_\<project_id\>_ should be replaced with the GCP project this cluster was created under.

Conversation

sebsoto commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebsoto commented Aug 20, 2022

Uh oh!

sebsoto commented Aug 20, 2022

Uh oh!

sebsoto commented Aug 20, 2022

Uh oh!

sebsoto commented Aug 21, 2022

Uh oh!

sebsoto commented Aug 23, 2022

Uh oh!

aravindhp 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!

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!

Uh oh!

sebsoto commented Aug 23, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sebsoto commented Aug 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebsoto commented Aug 24, 2022

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!

jrvaldes Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebsoto Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

saifshaikh48 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!

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

sebsoto commented Aug 20, 2022 •

edited

Loading

jrvaldes Aug 24, 2022 •

edited

Loading

sebsoto Aug 24, 2022 •

edited

Loading