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

Windows upstream e2e #1119

Merged

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Jan 13, 2021

What type of PR is this?
/kind other

What this PR does / why we need it:
Add testing support for upstream windows tests similar to https://testgrid.k8s.io/sig-cluster-lifecycle-cluster-api-provider-azure#periodic-capz-conformance-k8s-master.

TODO:

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 #

Special notes for your reviewer:
Some more work needs to be done to make this ready and it relies on:

Windows doesn't currently have Conformance tests but does have a set of tests that can verify Windows behavior. Defining Conformance tests for windows is a Work in progress and this set of tests can be updated later.

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Windows e2e tests from kubernetes are run against capz workload cluster

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot
Copy link
Contributor

@jsturtevant: The label(s) kind/other cannot be applied, because the repository doesn't have them

In response to this:

What type of PR is this?
/kind other

What this PR does / why we need it:
Add testing support for upstream windows tests similar to https://testgrid.k8s.io/sig-cluster-lifecycle-cluster-api-provider-azure#periodic-capz-conformance-k8s-master.

TODO: Add ability to inject kube binaries similiar to https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/a3525d2d89821192dbaeef270f46eff09e4c4587/templates/test/prow-machine-pool-ci-version/patches/machine-pool-ci-version.yaml

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 #

Special notes for your reviewer:
Some more work needs to be done to make this ready and it relies on:

Windows doesn't currently have Conformance tests but does have a set of tests that can verify Windows behavior. Defining Conformance tests for windows is a Work in progress and this set of tests can be updated later.

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:


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.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 13, 2021
@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Jan 13, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2021
@jsturtevant jsturtevant force-pushed the windows-upstream-e2e branch from 247ec83 to e95e75e Compare March 26, 2021 18:32
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2021
@jsturtevant jsturtevant marked this pull request as ready for review March 26, 2021 18:33
@jsturtevant jsturtevant changed the title [WIP] Windows upstream e2e Windows upstream e2e Mar 26, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 26, 2021
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 26, 2021
@jsturtevant
Copy link
Contributor Author

@CecileRobertMichon @nader-ziada This is ready for a review. I am running the suite now and will post results once it is complete. Sorry this took so long.

@CecileRobertMichon
Copy link
Contributor

Added a comment in https://github.com/kubernetes/test-infra/pull/21544/files#r602575057, we should add a PR test equivalent to the periodic job and use it to validate this PR

@jsturtevant
Copy link
Contributor Author

I got the test results back and had a few failures. Looking into today.

@jsturtevant
Copy link
Contributor Author

update: there are a few tests that fail due to networking configuration. I am working with the networking team to get the correct settings.

@jsturtevant
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

Superficial nits

@@ -0,0 +1,3 @@
gcAuthenticatedRegistry: e2eprivate
gcEtcdRegistry: k8sprow.azurecr.io/kubernetes-e2e-test-images
privateRegistry: e2eteam
Copy link
Contributor

Choose a reason for hiding this comment

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

new line nit

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 was directly pulled during make from https://github.com/kubernetes-sigs/windows-testing/blob/master/images/image-repo-list-master. And it looks like that file doesn't have the file endings.

I checked it in here for an example file and so it was standalone.

@jsturtevant
Copy link
Contributor Author

Will take a little longer to run but until kubernetes/test-infra#22180 merges but will be a signal nothing else changed and we can run it again for additional validation since this is a new test.

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows

@nader-ziada
Copy link
Contributor

this one is good to go as well, @devigned any final thoughts since you had requested changes before

@jsturtevant 👏

@jsturtevant
Copy link
Contributor Author

Running one last time with the updates in kubernetes/test-infra#22180

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows

@jsturtevant
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows

@jsturtevant
Copy link
Contributor Author

@devigned @nader-ziada @chewong any further thoughts on this?

@@ -298,6 +298,10 @@ spec:
New-HnsNetwork -Type Overlay -AddressPrefix "192.168.255.0/30" -Gateway "192.168.255.1" -Name "External" -AdapterName "Ethernet 2" -SubnetPolicies @(@{Type = "VSID"; VSID = 9999; })
path: C:/create-external-network.ps1
permissions: "0744"
- content: |
New-Item -ItemType Directory -Force -Path C:\tmp\
path: C:/create-temp-folder.ps1
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this temp folder useful for?

Copy link
Contributor Author

@jsturtevant jsturtevant May 20, 2021

Choose a reason for hiding this comment

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

It is required for the end to end tests. It is assume to be already created on the machines for several of the tests. I swear I left a comment here but it looks like it might have been dropped during a rebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add a comment

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

@jsturtevant jsturtevant force-pushed the windows-upstream-e2e branch 2 times, most recently from dbd6d2a to 3537f83 Compare May 20, 2021 17:33
@jsturtevant jsturtevant force-pushed the windows-upstream-e2e branch from 3537f83 to 3cc5960 Compare May 20, 2021 17:37
@jsturtevant
Copy link
Contributor Author

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

Outside of a naming question, lgtm

@@ -93,4 +94,8 @@ cleanup() {

trap cleanup EXIT

make test-conformance
if [[ "${WINDOWS}" == "true" ]]; then
make test-windows-upstream
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: what do you think about make test-conformance-windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named it this way since there is no official "Conformance" for windows. We run a good set of conformance test but not all and additional Windows tests.

kubernetes/kubernetes#96639 for more info on where we are with Conformance for Windows

@jsturtevant
Copy link
Contributor Author

We should run this again since there were changes

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows

@jsturtevant
Copy link
Contributor Author

I do not think the failure was not related to these changes:

I0520 19:56:58.101565       1 azurejson_machine_controller.go:116] controllers/AzureJSONMachine "msg"="object was not found" "azureMachine"="capz-conf-lhu7ze-control-plane-262fp" "namespace"="capz-conf-lhu7ze" 
E0520 19:56:58.103259       1 controller.go:302] controller-runtime/manager/controller/azuremachine "msg"="Reconciler error" "error"="azuremachines.infrastructure.cluster.x-k8s.io \"capz-conf-lhu7ze-control-plane-262fp\" not found" "name"="capz-conf-lhu7ze-control-plane-262fp" "namespace"="capz-conf-lhu7ze" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="AzureMachine" 

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows

@jsturtevant
Copy link
Contributor Author

The management cluster didn't come online, error was:

  �[91mTimed out after 180.003s.
  Deployment capi-system/capi-controller-manager failed to get status.Available = True condition

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

lgtm
/assign @nader-ziada

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 May 25, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8ebf049 into kubernetes-sigs:master May 25, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.5.0 milestone May 25, 2021
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. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants