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

WINC-806: Add support for Windows Server 2022 in Azure #1093

Merged
merged 5 commits into from Jun 29, 2022

Conversation

mansikulkarni96
Copy link
Member

This PR adds support for WS 2022 for Azure platform by updating the dev hack script, e2e test and the documentation.

@openshift-ci 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 Jun 6, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 6, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

@@ -21,7 +21,7 @@ these errors, only use the appropriate version according to the cloud provider i
| Cloud Provider | Supported Windows Server version |
|----------------|--------------------------------------------------------------------------------------|
| AWS | Windows Server 2019, version 1809 Long-Term Servicing Channel (LTSC) |
| Azure | Windows Server 2019, version 1809 Long-Term Servicing Channel (LTSC) |
| Azure | - Windows Server 2019, version 1809 Long-Term Servicing Channel (LTSC)<br>- Windows Server 2022 Long-Term Servicing Channel (LTSC) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Format the MD table. See this change.

@@ -18,7 +18,7 @@ const (
defaultCredentialsSecretName = "azure-cloud-credentials"
defaultImageOffer = "WindowsServer"
defaultImagePublisher = "MicrosoftWindowsServer"
defaultImageSKU = "datacenter-core-20h2-with-containers-smalldisk"
defaultImageSKU = "2022-datacenter-smalldisk"
Copy link
Contributor

Choose a reason for hiding this comment

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

See the container image for Azure:

windowsServerImage = "mcr.microsoft.com/powershell:lts-nanoserver-20h2"

@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

4 similar comments
@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator
cluster bring up issues

@mansikulkarni96 mansikulkarni96 force-pushed the WINC-806 branch 3 times, most recently from 9c1c111 to 6d305f5 Compare June 22, 2022 17:58
@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

@mansikulkarni96 mansikulkarni96 marked this pull request as ready for review June 22, 2022 20:29
@openshift-ci 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 Jun 22, 2022
@mansikulkarni96 mansikulkarni96 marked this pull request as draft June 24, 2022 15:58
@openshift-ci 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 Jun 24, 2022
@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

1 similar comment
@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

@@ -452,7 +452,7 @@ func (tc *testContext) getWindowsServerContainerImage() string {
windowsServerImage = "mcr.microsoft.com/powershell:lts-nanoserver-2004"
} else if tc.CloudProvider.GetType() == config.AzurePlatformType {
// On Azure we are testing 20H2
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment needs updating away from 20H2

if tc.hasCustomVXLAN {
powershellDefaultCommand = strings.ReplaceAll(command, "\\\"", "\"")
}
powershellDefaultCommand = strings.ReplaceAll(command, "\\\"", "\"")
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this cause the AWS tests fails on this PR since they still use 2019? I think if tc.hasCustomVXLAN || tc.CloudProvider.GetType() == config.AzurePlatformType is the needed clause for this to merge

Copy link
Contributor

Choose a reason for hiding this comment

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

@saifshaikh48 is right, until #1091 merges AWS still needs this. I proposed an alternative platform-specific approach.

Copy link
Contributor

@jrvaldes jrvaldes left a comment

Choose a reason for hiding this comment

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

Thanks @mansikulkarni96 for working on this, comments added. PTAL

```shell script
az vm image list --all --location <location> \
--publisher MicrosoftWindowsServer \
--offer WindowsServer \
--sku 2019-Datacenter-with-Containers \
--query "[?contains(version, '17763.1457.2009030514')]"
--sku 2022-Datacenter
Copy link
Contributor

Choose a reason for hiding this comment

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

s/2022-Datacenter/2022-datacenter

@@ -9,17 +9,14 @@ oc get -o jsonpath='{.status.infrastructureName}{"\n"}' infrastructure cluster

`<zone>` should be replaced with a valid Azure availability zone like `us-east-1a`.

`<image>` should be a WindowsServer image offering that defines the 2019-Datacenter-with-Containers SKU with version
17763.1457.2009030514 or earlier. Run the following command to list Azure image info:
`<image>` should be a WindowsServer image offering that defines the 2022-Datacenter SKU.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we recommend 2022-datacenter-smalldisk as we are using it the machineSet.sh script and e2e test suite.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that is required, I preferred to use smalldisk due to it's size but it can be changed to 2022-datacenter. We were using smalldisk for 2019 testing as well.

@@ -452,7 +452,7 @@ func (tc *testContext) getWindowsServerContainerImage() string {
windowsServerImage = "mcr.microsoft.com/powershell:lts-nanoserver-2004"
} else if tc.CloudProvider.GetType() == config.AzurePlatformType {
// On Azure we are testing 20H2
windowsServerImage = "mcr.microsoft.com/powershell:lts-nanoserver-20h2"
windowsServerImage = "mcr.microsoft.com/powershell:lts-nanoserver-ltsc2022"
} else {
// For other providers we use 1809
windowsServerImage = "mcr.microsoft.com/powershell:lts-nanoserver-1809"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel we will be using WS2019 as default any longer. PTAL at this commit, where I'm proposing WS2022 as the new normal.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes one of our PR's would have to be rebased depending on what goes first, having that logic present in one should be sufficient right?

Copy link
Contributor

Choose a reason for hiding this comment

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

i dont see anything wrong with this change
rebase conflicts can be handled when they arise.

@@ -56,7 +56,7 @@ func GenerateUserData(publicKey ssh.PublicKey) (*core.Secret, error) {
$firewallRuleName = "ContainerLogsPort"
$containerLogsPort = "10250"
New-NetFirewallRule -DisplayName $firewallRuleName -Direction Inbound -Action Allow -Protocol TCP -LocalPort $containerLogsPort -EdgeTraversalPolicy Allow
Set-Service -Name sshd -StartupType Automatic
Set-Service -Name sshd -StartupType 'Automatic'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work without any quote? See this example.

Copy link
Member Author

Choose a reason for hiding this comment

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

It didn't work for me when I tried it locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is required for 2022 to work, it ideally should be in the same commit as the e2e tests introducing our use of 2022. that commit can then be reframed as azure 2022 support.

Alternatively this needs to be before the e2e commit, if someone checked out the repo w/ the e2e commit as HEAD the tests would fail.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2022
This commit extends the use of PS commands adjusted
for WS2022 to include vSphere and Azure platforms
instead of the vxlan port.
This commit updates the azure provider file to
bring up machineSets with Windows Server 2022 image
in CI e2e tests. It also updates the image used
for deployment in the network tests.
This commit also increases the node creation time
as the nodes are taking longer to be ready with the
requirement of VM restart after installing the
Containers feature on each VM.
This commit updates the docs to declare WMCO
support for Azure on Windows Server 2022.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2022
@jrvaldes
Copy link
Contributor

/test aws-e2e-ccm-install

@jrvaldes
Copy link
Contributor

/test aws-e2e-upgrade

@jrvaldes
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2022
@sebsoto
Copy link
Contributor

sebsoto commented Jun 29, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jrvaldes, mansikulkarni96, sebsoto

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2022
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 2 against base HEAD f3bc9da and 8 for PR HEAD dff38de in total

@selansen
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 1 against base HEAD f3bc9da and 7 for PR HEAD dff38de in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 29, 2022

@mansikulkarni96: all tests passed!

Full PR test history. Your PR dashboard.

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 openshift-ci bot merged commit cfc5bb9 into openshift:master Jun 29, 2022
@mansikulkarni96
Copy link
Member Author

/cherry-pick release-4.11

@mansikulkarni96
Copy link
Member Author

/cherry-pick community-4.10

@openshift-cherrypick-robot

@mansikulkarni96: new pull request created: #1109

In response to this:

/cherry-pick release-4.11

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.

@openshift-cherrypick-robot

@mansikulkarni96: new pull request created: #1110

In response to this:

/cherry-pick community-4.10

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.

@sebsoto
Copy link
Contributor

sebsoto commented Aug 23, 2022

/cherry-pick release-4.10

@openshift-cherrypick-robot

@sebsoto: #1093 failed to apply on top of branch "release-4.10":

Applying: Fix quotes for PS command in userDataSecret
Applying: Set Datacenter-2022 SKU in machineset.sh
Applying: Update condition for using WS2022 PS commands
Applying: Set datacenter-2022 SKU in azure tests
Applying: Declare support for Azure 2022
Using index info to reconstruct a base tree...
M	docs/wmco-prerequisites.md
Falling back to patching base and 3-way merge...
Auto-merging docs/wmco-prerequisites.md
CONFLICT (content): Merge conflict in docs/wmco-prerequisites.md
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Declare support for Azure 2022
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:

/cherry-pick release-4.10

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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants