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-607: Add support for platform=none in e2e test suite #858

Merged
merged 3 commits into from Jan 28, 2022

Conversation

jrvaldes
Copy link
Contributor

@jrvaldes jrvaldes commented Jan 4, 2022

This PR adds support for the e2e tests to run in a platform-agnostic infrastructure (platform=none).

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 4, 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

@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 Jan 4, 2022
@jrvaldes
Copy link
Contributor Author

jrvaldes commented Jan 4, 2022

/test unit

@jrvaldes
Copy link
Contributor Author

jrvaldes commented Jan 4, 2022

/test lint

Makefile Outdated
.PHONY: run-ci-e2e-byoh-test $(ARGS)
run-ci-e2e-byoh-test:
hack/run-ci-e2e-test.sh ${ARGS}

.PHONY: clean
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[windows-machine-config-operator]$ make run-ci-e2e-byoh-test ARGS="-t basic -m 0 -c 1"
hack/run-ci-e2e-test.sh -t basic -m 0 -c 1
ARTIFACT_DIR is not set. Artifacts will be stored in: /tmp/tmp.FojjLu5RUr
make[1]: Entering directory '/home/jvaldes/Projects/windows-machine-config-operator'
go fmt ./...
go vet ./...
build/build.sh "build/_output" 5.0.0 -mod=vendor
building windows-machine-config-operator...
make[1]: Leaving directory '/home/jvaldes/Projects/windows-machine-config-operator'
INFO[0000] Found annotations file                        bundle-dir=bundle container-tool=docker
INFO[0000] Could not find optional dependencies file     bundle-dir=bundle container-tool=docker
INFO[0000] All validation tests have completed successfully 
Error from server (NotFound): namespaces "openshift-windows-machine-config-operator" not found
namespace/openshift-windows-machine-config-operator created
namespace/openshift-windows-machine-config-operator labeled
INFO[0001] Creating windows-machine-config-operator registry 
INFO[0001]   Creating ConfigMap "openshift-windows-machine-config-operator/windows-machine-config-operator-registry-manifests-package" 
INFO[0001]   Creating ConfigMap "openshift-windows-machine-config-operator/windows-machine-config-operator-registry-manifests-5-0-0" 
INFO[0001]   Creating Deployment "openshift-windows-machine-config-operator/windows-machine-config-operator-registry-server" 
INFO[0001]   Creating Service "openshift-windows-machine-config-operator/windows-machine-config-operator-registry-server" 
INFO[0001] Waiting for Deployment "openshift-windows-machine-config-operator/windows-machine-config-operator-registry-server" rollout to complete 
INFO[0002]   Waiting for Deployment "openshift-windows-machine-config-operator/windows-machine-config-operator-registry-server" to rollout: 0 out of 1 new replicas have been updated 
INFO[0013]   Waiting for Deployment "openshift-windows-machine-config-operator/windows-machine-config-operator-registry-server" to rollout: 0 of 1 updated replicas are available 
INFO[0023]   Deployment "openshift-windows-machine-config-operator/windows-machine-config-operator-registry-server" successfully rolled out 
INFO[0023] Created CatalogSource: windows-machine-config-operator-catalog 
INFO[0023] OperatorGroup "operator-sdk-og" created      
INFO[0023] Created Subscription: windows-machine-config-operator-v5-0-0-sub 
INFO[0026] Approved InstallPlan install-w7fg8 for the Subscription: windows-machine-config-operator-v5-0-0-sub 
INFO[0026] Waiting for ClusterServiceVersion "openshift-windows-machine-config-operator/windows-machine-config-operator.v5.0.0" to reach 'Succeeded' phase 
INFO[0026]   Waiting for ClusterServiceVersion "openshift-windows-machine-config-operator/windows-machine-config-operator.v5.0.0" to appear 
INFO[0028]   Found ClusterServiceVersion "openshift-windows-machine-config-operator/windows-machine-config-operator.v5.0.0" phase: Pending 
INFO[0031]   Found ClusterServiceVersion "openshift-windows-machine-config-operator/windows-machine-config-operator.v5.0.0" phase: Installing 
INFO[0042]   Found ClusterServiceVersion "openshift-windows-machine-config-operator/windows-machine-config-operator.v5.0.0" phase: Succeeded 
INFO[0042] OLM has successfully installed "windows-machine-config-operator.v5.0.0" 
deployment "windows-machine-config-operator" successfully rolled out
configmap/windows-instances created
=== RUN   TestWMCO
=== RUN   TestWMCO/operator_deployed_without_private_key_secret
--- PASS: TestWMCO (1.32s)
    --- PASS: TestWMCO/operator_deployed_without_private_key_secret (0.42s)
PASS
ok      github.com/openshift/windows-machine-config-operator/test/e2e   1.554s
?       github.com/openshift/windows-machine-config-operator/test/e2e/clusterinfo       [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers/aws     [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers/azure   [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers/none    [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers/vsphere [no test files]
=== RUN   TestWMCO
=== RUN   TestWMCO/create
=== RUN   TestWMCO/create/Creation
=== RUN   TestWMCO/create/Creation/Windows_instance_configured_by_ConfigMap_controller
2022/01/03 20:19:43 waiting for 0/1 Windows nodes
2022/01/03 20:20:43 waiting for 0/1 Windows nodes
2022/01/03 20:21:43 waiting for 0/1 Windows nodes
2022/01/03 20:22:43 waiting for 0/1 Windows nodes
2022/01/03 20:23:43 waiting for 0/1 Windows nodes
2022/01/03 20:24:43 waiting for 0/1 Windows nodes
2022/01/03 20:25:43 waiting for 0/1 Windows nodes
2022/01/03 20:26:43 waiting for 0/1 Windows nodes
2022/01/03 20:27:43 waiting for 0/1 Windows nodes
2022/01/03 20:28:43 waiting for 0/1 Windows nodes
2022/01/03 20:29:43 waiting for 0/1 Windows nodes
2022/01/03 20:30:43 waiting for 0/1 Windows nodes
2022/01/03 20:31:43 waiting for 0/1 Windows nodes
2022/01/03 20:32:43 waiting for 0/1 Windows nodes
2022/01/03 20:33:43 waiting for 0/1 Windows nodes
2022/01/03 20:34:43 waiting for 0/1 Windows nodes
2022/01/03 20:35:43 waiting for 0/1 Windows nodes
2022/01/03 20:36:43 waiting for 0/1 Windows nodes
2022/01/03 20:37:43 waiting for 0/1 Windows nodes
2022/01/03 20:38:43 waiting for 0/1 Windows nodes
2022/01/03 20:39:43 waiting for 0/1 Windows nodes
2022/01/03 20:40:43 waiting for 0/1 Windows nodes
2022/01/03 20:41:43 waiting for 0/1 Windows nodes
2022/01/03 20:42:43 waiting for 0/1 Windows nodes
2022/01/03 20:43:43 waiting for 0/1 Windows nodes
2022/01/03 20:44:43 waiting for 0/1 Windows nodes
2022/01/03 20:45:43 waiting for 0/1 Windows nodes
2022/01/03 20:46:43 waiting for 0/1 Windows nodes
2022/01/03 20:47:43 waiting for 0/1 Windows nodes
2022/01/03 20:48:43 waiting for 0/1 Windows nodes
2022/01/03 20:48:43 waiting for 0/1 Windows nodes
2022/01/03 20:48:43 30m0.266048877s time is required to configure 1 nodes
    create_test.go:74: 
                Error Trace:    create_test.go:74
                Error:          Received unexpected error:
                                timed out waiting for the condition
                Test:           TestWMCO/create/Creation/Windows_instance_configured_by_ConfigMap_controller
                Messages:       Windows node creation failed
--- FAIL: TestWMCO (1817.31s)
    --- FAIL: TestWMCO/create (1816.45s)
        --- FAIL: TestWMCO/create/Creation (1816.45s)
            --- FAIL: TestWMCO/create/Creation/Windows_instance_configured_by_ConfigMap_controller (1800.27s)
FAIL
FAIL    github.com/openshift/windows-machine-config-operator/test/e2e   1817.534s
?       github.com/openshift/windows-machine-config-operator/test/e2e/clusterinfo       [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers/aws     [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers/azure   [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers/none    [no test files]
?       github.com/openshift/windows-machine-config-operator/test/e2e/providers/vsphere [no test files]
FAIL
make: *** [Makefile:188: run-ci-e2e-byoh-test] Error 1

While validating with a development platform=none UPI cluster, the e2e test suit creates the windows-instances configMap and supports platform=none as cloud provider. No Windows instances are getting created via machineSet generation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WMCO was not able to provision the BYOH Windows instance as a Windows node due to CSR approval issues.

ERROR	controller-runtime.manager.controller.certificatesigningrequest	Reconciler error
  {
    "reconciler group": "certificates.k8s.io",
    "reconciler kind": "CertificateSigningRequest",
    "name": "csr-47vzn",
    "namespace": "",
    "error": "WMCO CSR Approver could not approve csr-47vzn CSR:
        could not validate contents for approval of CSR: csr-47vzn:
        error validating node name winhost for CSR: csr-47vzn:
        unable to map node name to the addresses of Windows instances:
        failed to lookup   DNS for IP 192.168.20.13:
        lookup 13.20.168.192.in-addr.arpa. on 192.168.20.2:53: no such host",
        ...
}

A PTR record must exist corresponding to the instance address (13.20.168.192 in this case) which resolves to the instance hostname for successful reverse DNS lookups.

Copy link
Member

@mansikulkarni96 mansikulkarni96 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @jrvaldes
PTAL at my comments.

docs/HACKING.md Outdated
@@ -93,6 +93,23 @@ export OPENSHIFT_CI=false
# on AWS only:
export AWS_SHARED_CREDENTIALS_FILE=<path to aws credentials file>
```

In addition, just for **BYOH** use case with **platform=none** UPI cluster export the `WINDOWS_INSTANCES_DATA`
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we explain how the instances will/should be brought up when running this scenario locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will adjust.

if testCtx.CloudProvider.GetType() == config.NonePlatformType {
// TODO: setPowerShellDefaultShell for given Windows instance(s)
// TODO: validate windows-instances configMap
t.Run("Windows instance configured by ConfigMap controller", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

All BYOH nodes will be configured by configmap controller, how about a name that suggests this tests the BYOH configuration on platform none provider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will include more context around "platform none provider".

test/e2e/providers/none/none.go Show resolved Hide resolved
@@ -58,6 +58,22 @@ func testWindowsNodeCreation(t *testing.T) {
// Create a private key secret with the known private key.
require.NoError(t, testCtx.createPrivateKeySecret(true), "could not create known private key secret")

// Machine API Operator is not available in a platform=none UPI cluster. Hence, cannot use machineSets to create
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need to add this comment, the reason we cannot use machine sets is cloud provider is none in this setup.

@jrvaldes jrvaldes force-pushed the WINC-607 branch 3 times, most recently from 29a59ce to 2e46e27 Compare January 7, 2022 05:32
@jrvaldes jrvaldes changed the title WINC-607: [e2e] Add support for platform=none WINC-607: Add support for platform=none in e2e test suite Jan 7, 2022
@jrvaldes
Copy link
Contributor Author

jrvaldes commented Jan 7, 2022

/test unit

@jrvaldes
Copy link
Contributor Author

jrvaldes commented Jan 7, 2022

/test lint

Comment on lines +139 to +142
data:
10.1.42.1: |-
username=Administrator
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to have 2 instances in the example? Just so the "more complex" case is documented for users

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'd rather keep a simple example here, the "more complex" ones are listed in the above-mentioned/referenced windows-instances configMap documentation.

Comment on lines 63 to 64
// TODO: setPowerShellDefaultShell for given Windows instance(s)
// TODO: validate windows-instances configMap
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice TODOs. I suggest flipping their order when implementing them however (fail faster).

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to address the TODOs here or in a follow up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer need them, I re-worked this based on Sebastian's comment.

docs/HACKING.md Outdated
### Running e2e tests on platform-agnostic infrastructure

To run the WMCO' e2e tests on a bare metal or other platform-agnostic infrastructure (platform=none), where there
is no cloud provider specification, the desired number of Windows instances must be available beforehand, and you
Copy link
Contributor

Choose a reason for hiding this comment

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

s/available/provisioned/g

Comment on lines 62 to 71
if testCtx.CloudProvider.GetType() == config.NonePlatformType {
// TODO: setPowerShellDefaultShell for given Windows instance(s)
// TODO: validate windows-instances configMap
t.Run("[platform=none] Windows instance configured by ConfigMap controller", func(t *testing.T) {
err = testCtx.waitForWindowsNodes(gc.numberOfBYOHNodes, false, false, true)
assert.NoError(t, err, "Windows node creation failed")
})
// Expected number of Windows instances configured as worker nodes, return
return
}
Copy link
Contributor

@sebsoto sebsoto Jan 7, 2022

Choose a reason for hiding this comment

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

I'm hesitant about creating another branching path based on another if testCtx.CloudProvider.GetType() ==, this code really belongs as part of the ConfigMap controller test.

An alternative way for you to approach this, is to expand the Provider interface to include a new method such as NewBYOHConfigMap(instanceCount).

For existing providers (AWS,Azure etc.) this method could call a shared function which encapsulates the logic currently within the BYOH tests used to create the VMs and the windows-instances configmap.

func (tc *testContext) testBYOHConfiguration(t *testing.T) {
if gc.numberOfBYOHNodes == 0 {
t.Skip("BYOH testing disabled")
}
// Patch the CVO with overrides spec value for cluster-machine-approver deployment
// Doing so, stops CVO from creating/updating its deployment hereafter.
patchData := `[{"op":"add","path":"/spec/overrides","value":[{"kind":"Deployment","group":"apps","name":"machine-approver","namespace":"openshift-cluster-machine-approver","unmanaged":true}]}]`
_, err := tc.client.Config.ConfigV1().ClusterVersions().Patch(context.TODO(), "version", types.JSONPatchType, []byte(patchData), metav1.PatchOptions{})
// Scale the Cluster Machine Approver Deployment to 0
// This is required for testing BYOH CSR approval feature so that BYOH instances
// CSR's are not approved by Cluster Machine Approver
expectedPodCount := int32(0)
err = tc.scaleMachineApproverDeployment(&expectedPodCount)
require.NoError(t, err, "failed to scale Machine Approver pods")
_, err = tc.createWindowsMachineSet(gc.numberOfBYOHNodes, false)
require.NoError(t, err, "failed to create Windows MachineSet")
machines, err := tc.waitForWindowsMachines(int(gc.numberOfBYOHNodes), "Provisioned", false)
require.NoError(t, err, "Machines did not reach expected state")

Then for the new PlatformNone provider you are adding, you can either move the logic you added in the hack script into the go code, or you can assume it was created ahead of time and do a check to ensure the ConfigMap exists, with the correct amount of Windows instances listed

That way within the ConfigMap controller test, what can be done is:

require.NoError(t,testCtx.CloudProvider.NewBYOHConfigMap(gc.numberOfBYOHNodes))
err = tc.waitForWindowsNodes(gc.numberOfBYOHNodes, false, false, true)
assert.NoError(t, err, "Windows node creation failed")

Doing things this way helps smooth out the flow of our tests, and also helps us expand provider specific requirements in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebsoto Thanks for the feedback. This is good. Will adjust.

Copy link
Contributor Author

@jrvaldes jrvaldes Jan 13, 2022

Choose a reason for hiding this comment

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

Opened #865 to partially address this comment. WINC-608 is a blocker to fully decouple BYOH from MachineSets, making the creation of the BYOH instances cloud provider-specific.

Copy link
Contributor

@aravindhp aravindhp left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @jrvaldes. Please address my comments.

Makefile Outdated
Comment on lines 188 to 190
.PHONY: run-ci-e2e-byoh-test $(ARGS)
run-ci-e2e-byoh-test:
hack/run-ci-e2e-test.sh ${ARGS}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather have this explicit now instead of using $(ARGS). Once you have introduced the new single target with $(ARGS), you can collapse all the existing endpoints. The reason for this is that we will eventually get rid of run-ci-e2e-byoh-test and there is little advantage in doing this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial implementation too, the main driver for using a parameter here is:

  • the windows instance must be provisioned before invoking the test
  • the test is invoked with arguments ($ARGS) right after $WINDOWS_INSTANCES_DATA is set as env var.
  • $WINDOWS_INSTANCES_DATA and $ARGS must be aligned, given $ARGS contains the number of entries in $WINDOWS_INSTANCES_DATA
make run-ci-e2e-byoh-test ARGS="-t basic -m 0 -c 1"

So, having both definitions in the same place is safer.

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 follow your reasoning here. Why can't you use the $WINDOWS_INSTANCES_DATA in the call here then without having to use $(ARGS)?

So, having both definitions in the same place is safer.

I don't understand what you mean by this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By same place meant the commands in the release repo.

You are right, will adjust to only pass WINDOWS_INSTANCES_DATA and parse the number of instances programmatically in the e2e code.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is not what I mean. We should just have a make run-ci-e2e-byoh-test in the release repo without any args. This is what we have been doing in the past mainly because we want to reduce the interaction we have with the release repo. With the pattern you are introducing anytime we want to change the way we are calling the test we have to go to the release repo instead of the WMCO repo. IOW, don't introduce $(ARGS).

Copy link
Contributor Author

@jrvaldes jrvaldes Jan 10, 2022

Choose a reason for hiding this comment

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

I follow, by

only pass WINDOWS_INSTANCES_DATA

I meant, pass WINDOWS_INSTANCES_DATA as an environment variable.

The Makefile target has no arguments, and the test can be configured and invoked using the same pattern from a release job and/or running locally. For example:

export WINDOWS_INSTANCES_DATA

make run-ci-e2e-test

So, no need to introduce the proposed run-ci-e2e-byoh-test target at all.

hack/run-ci-e2e-test.sh will look after $WINDOWS_INSTANCES_DATA and create the ConfigMap with the provided data if any. At this point, syntax validation is deferred to oc apply API.

WMCO e2e test must be smart enough, to process an existing windows-instances ConfigMap and adjust accordingly. i.e. do not create BOYH instances on platform-agnostic infrastructure and use the provided ones, if any.

@@ -11,6 +11,9 @@ BYOH_NODE_COUNT_OPTION=""
SKIP_NODE_DELETION=""
WMCO_PATH_OPTION=""

# WINDOWS_INSTANCES_DATA holds the windows-instances configMap data section
declare -r WINDOWS_INSTANCES_DATA=${WINDOWS_INSTANCES_DATA:-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to use declare given we have not be doing this before? I under stand the weak typesetting this allows but I don't see the advantage and would like us to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using declare to ensure $WINDOWS_INSTANCES_DATA is read-only (-r) as a constant. Before the usage aligns more as variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are many other variable where this applies to. I don't see this as being reason enough to introduce this.

@@ -101,6 +104,22 @@ if ! [[ "$OPENSHIFT_CI" = "true" && "$TEST" = "upgrade" ]]; then
done
fi

# Check windows-instances data and create the windows-instances configMap, if
Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigMap. Please fix everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -58,6 +58,18 @@ func testWindowsNodeCreation(t *testing.T) {
// Create a private key secret with the known private key.
require.NoError(t, testCtx.createPrivateKeySecret(true), "could not create known private key secret")

// in case platform=none is detected just wait for Windows worker node to become available
Copy link
Contributor

Choose a reason for hiding this comment

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

For platform=none, just wait...

Comment on lines 63 to 64
// TODO: setPowerShellDefaultShell for given Windows instance(s)
// TODO: validate windows-instances configMap
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to address the TODOs here or in a follow up PR?

Comment on lines 62 to 71
if testCtx.CloudProvider.GetType() == config.NonePlatformType {
// TODO: setPowerShellDefaultShell for given Windows instance(s)
// TODO: validate windows-instances configMap
t.Run("[platform=none] Windows instance configured by ConfigMap controller", func(t *testing.T) {
err = testCtx.waitForWindowsNodes(gc.numberOfBYOHNodes, false, false, true)
assert.NoError(t, err, "Windows node creation failed")
})
// Expected number of Windows instances configured as worker nodes, return
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

docs/HACKING.md Outdated
@@ -117,6 +117,40 @@ hack/run-ci-e2e-test.sh -s -m 2 -c 1
```
Please note that you do not need to run `hack/olm.sh run` before `hack/run-ci-e2e-test.sh`.

### Running e2e tests on platform-agnostic infrastructure
Copy link
Contributor

Choose a reason for hiding this comment

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

#### as this is sub section under Running e2e tests on a cluster. Cluster includes platform=none.

docs/HACKING.md Outdated

To run the WMCO' e2e tests on a bare metal or other platform-agnostic infrastructure (platform=none), where there
is no cloud provider specification, the desired number of Windows instances must be available beforehand, and you
need provide the instance(s) information to the e2e test suite using an environment variable.
Copy link
Contributor

Choose a reason for hiding this comment

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

need to provide the instance(s) information to the e2e test suite using the WINDOWS_INSTANCES_DATA environment variable

docs/HACKING.md Outdated
- the internal IP address, and
- the Windows administrator username

Export `WINDOWS_INSTANCES_DATA` as an environment variable with the corresponding [windows-instances configMap](https://github.com/openshift/windows-machine-config-operator#adding-instances)
Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigMap. Please fix everywhere.

docs/HACKING.md Outdated
Comment on lines 151 to 152
accounts for the number of instances that will be configured by the ConfigMap controller, and must match the
number of entries in the `WINDOWS_INSTANCES_DATA` environment variable, in this example only one (1).
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be caught programmatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, plan to address this under the above comment's umbrella.

@jrvaldes
Copy link
Contributor Author

jrvaldes commented Jan 18, 2022

/hold

Depends on #865

vSphere CI (`vmc-ci.devcluster.openshift.com`) uses AWS Route53 for DNS
resolution and the following ci-segments are reserved for platform=none clusters
- ci-segment-56
- ci-segment-47
Copy link
Contributor

@rvanderp3 rvanderp3 Jan 18, 2022

Choose a reason for hiding this comment

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

I think you meant ci-segment-57

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Addressed in 79db264

@jrvaldes jrvaldes force-pushed the WINC-607 branch 3 times, most recently from 38c3c57 to 70babf6 Compare January 24, 2022 23:38
@jrvaldes
Copy link
Contributor Author

/test lint

@jrvaldes
Copy link
Contributor Author

/test unit

@jrvaldes
Copy link
Contributor Author

/test vsphere-e2e-operator

@jrvaldes
Copy link
Contributor Author

/cherry-pick release-4.10

@jrvaldes
Copy link
Contributor Author

/cherry-pick community-4.9

@jrvaldes
Copy link
Contributor Author

/cherry-pick release-4.9

@jrvaldes
Copy link
Contributor Author

/cherry-pick community-4.8

@jrvaldes
Copy link
Contributor Author

/cherry-pick release-4.8

@openshift-cherrypick-robot

@jrvaldes: new pull request created: #901

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.

@openshift-cherrypick-robot

@jrvaldes: #858 failed to apply on top of branch "community-4.9":

Applying: Add support for platform=none
Using index info to reconstruct a base tree...
M	Makefile
M	hack/run-ci-e2e-test.sh
M	test/e2e/create_test.go
M	test/e2e/providers/cloudprovider.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/providers/cloudprovider.go
Auto-merging test/e2e/create_test.go
CONFLICT (content): Merge conflict in test/e2e/create_test.go
Auto-merging hack/run-ci-e2e-test.sh
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support for platform=none
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 community-4.9

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

@jrvaldes: #858 failed to apply on top of branch "release-4.9":

Applying: Add support for platform=none
Using index info to reconstruct a base tree...
M	Makefile
M	test/e2e/create_test.go
M	test/e2e/providers/cloudprovider.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/providers/cloudprovider.go
Auto-merging test/e2e/create_test.go
CONFLICT (content): Merge conflict in test/e2e/create_test.go
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support for platform=none
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.9

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

@jrvaldes: #858 failed to apply on top of branch "community-4.8":

Applying: Add support for platform=none
Using index info to reconstruct a base tree...
M	Makefile
M	hack/run-ci-e2e-test.sh
M	test/e2e/create_test.go
M	test/e2e/providers/cloudprovider.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/providers/cloudprovider.go
Auto-merging test/e2e/create_test.go
CONFLICT (content): Merge conflict in test/e2e/create_test.go
Auto-merging hack/run-ci-e2e-test.sh
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support for platform=none
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 community-4.8

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

@jrvaldes: #858 failed to apply on top of branch "release-4.8":

Applying: Add support for platform=none
Using index info to reconstruct a base tree...
M	Makefile
M	hack/run-ci-e2e-test.sh
M	test/e2e/create_test.go
M	test/e2e/providers/cloudprovider.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/providers/cloudprovider.go
Auto-merging test/e2e/create_test.go
CONFLICT (content): Merge conflict in test/e2e/create_test.go
Auto-merging hack/run-ci-e2e-test.sh
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support for platform=none
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.8

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.

@aravindhp
Copy link
Contributor

@jrvaldes this needs to be backported only to releases where we support platform:none.

@jrvaldes
Copy link
Contributor Author

@jrvaldes this needs to be backported only to releases where we support platform:none.

From Cluster and OS Pre-Requisites, Platform none (BYOH) support starts at 4.8+.

@aravindhp
Copy link
Contributor

@jrvaldes this needs to be backported only to releases where we support platform:none.

From Cluster and OS Pre-Requisites, Platform none (BYOH) support starts at 4.8+.

My bad. I am mixing up releases. Sorry for the noise.

@jrvaldes
Copy link
Contributor Author

/cherry-pick community-4.9

@openshift-cherrypick-robot

@jrvaldes: #858 failed to apply on top of branch "community-4.9":

Applying: Add support for platform=none
Using index info to reconstruct a base tree...
M	Makefile
M	hack/run-ci-e2e-test.sh
M	test/e2e/create_test.go
M	test/e2e/providers/cloudprovider.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/providers/cloudprovider.go
Auto-merging test/e2e/create_test.go
CONFLICT (content): Merge conflict in test/e2e/create_test.go
Auto-merging hack/run-ci-e2e-test.sh
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support for platform=none
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 community-4.9

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.

@mansikulkarni96
Copy link
Member

/cherry-pick release-4.9

@openshift-cherrypick-robot

@mansikulkarni96: #858 failed to apply on top of branch "release-4.9":

Applying: Add support for platform=none
Using index info to reconstruct a base tree...
M	Makefile
M	test/e2e/create_test.go
M	test/e2e/providers/cloudprovider.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/providers/cloudprovider.go
Auto-merging test/e2e/create_test.go
CONFLICT (content): Merge conflict in test/e2e/create_test.go
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support for platform=none
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.9

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.

@mansikulkarni96
Copy link
Member

/cherry-pick release-4.9

@openshift-cherrypick-robot

@mansikulkarni96: #858 failed to apply on top of branch "release-4.9":

Applying: Add support for platform=none
Using index info to reconstruct a base tree...
M	Makefile
M	test/e2e/create_test.go
M	test/e2e/providers/cloudprovider.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/providers/cloudprovider.go
Auto-merging test/e2e/create_test.go
CONFLICT (content): Merge conflict in test/e2e/create_test.go
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support for platform=none
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.9

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.

@mansikulkarni96
Copy link
Member

/cherry-pick community-4.9

@openshift-cherrypick-robot

@mansikulkarni96: new pull request created: #955

In response to this:

/cherry-pick community-4.9

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