🌱E2E template parametrization #218

wgslr · 2021-06-02T13:57:26Z

What this PR does / why we need it:
This PR replaces hardcoded values in e2e testing template with env variable substitution.

wgslr · 2021-06-02T14:03:31Z

/cc @furkatgofurov7

furkatgofurov7

Mostly looks good, have you tested it locally? Also, we can't for now run tests in this patch since we do not have jenkins jobs + according changes in place yet.

furkatgofurov7 · 2021-06-03T17:35:56Z

hack/e2e/environment.sh

+export KUBERNETES_VERSION=v1.21.1
+export API_ENDPOINT_HOST=192.168.111.249
+export API_ENDPOINT_PORT=6443


I think these and other vars below without quotes also should have a common format as other vars to keep consistency?

Ok, I quoted all of them.

furkatgofurov7 · 2021-06-03T22:59:04Z

hack/e2e/environment.sh

+
+# preserve the template, to be substituted based on value defined in e2e_test.go
+# shellcheck disable=SC2016
+export CONTROL_PLANE_MACHINE_COUNT='${CONTROL_PLANE_MACHINE_COUNT}'


After giving a bit more thought on this, I am wondering if this is good idea to leave this var dependent to go code value from e2e_test.go file. It seems it is possible to make it define here, have a look: here and here?

Hm, I can define the variable and have it substituted in envsubst. But I'm not sure why is decoupling from e2e_test.go better?

Because, that way it is easier IMO to set number of controlplane and worker machines for the user rather than looking for go code.

Ok, I've reverted that change. Although it means that the number given in e2e_test.go will be a bit confusing; it can be even set to a random number like 5 and the test will still pass alright.

Judging by today's call adjusting it in the template is not easier than in go. Maybe let's restore the CONTROL_PLANE_MACHINE_COUNT?

Yes, let's restore it.

wgslr · 2021-06-04T08:26:34Z

Yes, I did run the tests manually on my VM and they pass.

furkatgofurov7

There couple of more items that needs parameterization, please go through it once again. These are the ones I spotted, you can use below names borrowed from m3-dev-env if you have not defined them yet in env.sh, otherwise you can reuse already defined ones:

CLUSTER_APIENDPOINT_HOST
IRONIC_ENDPOINT_BRIDGE
REGISTRY
IMAGE_USERNAME
same IRONIC_ENDPOINT_BRIDGE and REGISTRY too

furkatgofurov7 · 2021-06-05T20:36:49Z

templates/test/cluster-template-prow-ha.yaml

-  #rolloutStrategy:
-  #  rollingUpdate:
-  #    maxSurge: 1


why are these removed?

To not keep commented out code around

macaptain · 2021-06-21T13:07:08Z

templates/test/cluster-template-prow-ha.yaml

    preKubeadmCommands:
    - netplan apply
    - systemctl enable --now crio kubelet
-    - if (curl -sk --max-time 10 https://192.168.111.249:6443/healthz); then echo "keepalived already running";else systemctl start keepalived; fi
+    - if (curl -sk --max-time 10 https://${API_ENDPOINT_HOST}:${API_ENDPOINT_PORT}/healthz); then echo "keepalived already running";else systemctl start keepalived; fi


I think this should be CLUSTER_APIENDPOINT_HOST and CLUSTER_APIENDPOINT_PORT to keep the same variable names as are used in the metal3-dev-env.

Good catch. I changed it elsewhere in the file but not here

wgslr · 2021-06-21T13:09:55Z

scripts/ci-e2e.sh

+M3_DEV_ENV_REPO="https://github.com/nordix/metal3-dev-env.git"
+M3_DEV_ENV_BRANCH="run-arbitrary-command-wgslr"


Suggested change

M3_DEV_ENV_REPO="https://github.com/nordix/metal3-dev-env.git"

M3_DEV_ENV_BRANCH="run-arbitrary-command-wgslr"

M3_DEV_ENV_REPO="https://github.com/metal3-io/metal3-dev-env.git"

M3_DEV_ENV_BRANCH=master

TODO Revert the change before merging

is this needed for testing with integration test ?

It was needed for checking that e2e tests work. The change can be removed as soon as m3-dev-env#703 gets merged

fmuyassarov

LGTM

wgslr · 2021-06-24T06:52:00Z

/test-v1a5-e2e

wgslr · 2021-06-28T07:54:04Z

/test-integration
/test-v1a5-e2e

wgslr · 2021-06-29T07:33:18Z

/test-v1a5-e2e

wgslr · 2021-06-29T08:02:57Z

/test-integration

namnx228

Overall, it looks good to me. Just leave a comment.

namnx228 · 2021-06-29T09:26:54Z

test/e2e/config/e2e_conf.yaml

-    - sourcePath: "${PWD}/templates/test/cluster-template-prow-single-master.yaml"
-      targetName: "cluster-template-single-master.yaml"
-    - sourcePath: "${PWD}/templates/test/cluster-template-prow-ha.yaml"
+    - sourcePath: "${PWD}/templates/test/cluster-template-prow-ha_envsubst.yaml"


You sure it is not cluster-template-prow-ha.yaml?

Yes. The cluster-template-prow-ha_envsubst.yaml is generated by passing cluster-template-prow-ha.yaml through envsubst, which is done in the e2e-substitutions make target.

I missed this part while reviewing, would taking e2e-substitutions into e2e-tests make target help with that, because otherwise we would have to have _envsubst added for each template yaml naming where we could have multiple of them in the future

furkatgofurov7 · 2021-06-29T09:45:25Z

hack/e2e/environment.sh

+export CAPI_VERSION=${CAPI_VERSION:-"v1alpha4"}
+export CAPM3_VERSION=${CAPM3_VERSION:-"v1alpha5"}


why v1a4/v1a5? we are not there yet right? I think we can keep both CAPI_VERSION and CAPM3_VERSION set to v1a3 && v1a4 accordingly for now, and we will update it as part of #167

When running in the CI it's overridden by env vars, which are already set to v1alpha4/alpha5. But I can stick to the old values as the defaults for manual runs.

Yes, the point is as of now we do not have a working CAPM3 v1a5 with CAPI v1a4 yet, the vars can be set to anything, it just to avoid confusion for users thinking they are running e2e with latest CAPI.

furkatgofurov7 · 2021-06-29T09:51:05Z

templates/test/cluster-template-prow-single-master.yaml

@@ -1,397 +0,0 @@
-apiVersion: cluster.x-k8s.io/v1alpha3


To clarify, we can live without this file and control number of ctpl/workers from the go code itself using the vars from environment.sh?

To clarify, we can live without this file and control number of ctpl/workers from the go code itself using the vars from environment.sh?

Have the same concern. It would be nice if we can have that

Yes. We can control the number of ctpl and workers using ControlPlaneMachineCount and WorkerMachineCount fields in ConfigClusterInput in the go code. No need to modify environment.sh either, except for setting NUM_NODES high enough to accomodate all the machines.

lentzi90

This is certainly an improvement over having everything hard-coded. But I'm wondering if it would make sense to use go templates instead of envsubst in the long run?

Environment variables are probably fine as long as we just want them set to one value for all tests. However, I guess we may want to set different values for different e2e tests and still run them all as part of one test suite. For example, we may want to create a HA cluster in one test and a single machine cluster in another. With go templates it would be very natural to define these parameters in the test itself instead of setting them through environment variables.
What do you think?

wgslr · 2021-06-29T10:16:08Z

For example, we may want to create a HA cluster in one test and a single machine cluster in another.

This particular example - differentiating node numbers - is already achievable by chaning values in the go code. But I see your point; if we reach a use case that demands more customization of the templates it may be necessary to move to a templating solution. Do you have some particular templating mechanism in mind? For reference, AWS provider uses kustomize

lentzi90 · 2021-06-29T10:27:36Z

Do you have some particular templating mechanism in mind? For reference, AWS provider uses kustomize

Interesting! Then for AWS it would still require changes outside of the go code to modify the flavors. 🤔
I was thinking about go template which is already built in to the language and should be familiar to many from Helm templates. This would allow us to specify the values we want inside the test code.

wgslr · 2021-06-29T10:27:43Z

/test-v1a5-e2e
/test-integration

jan-est · 2021-06-29T12:57:54Z

LGTM

furkatgofurov7

Last thing to clarify before we move on with this one:

furkatgofurov7 · 2021-06-29T13:48:28Z

Makefile

+.PHONY: e2e-substitutions
+e2e-substitutions: $(ENVSUBST)
+	$(ENVSUBST) < $(E2E_CONF_FILE) > $(E2E_CONF_FILE_ENVSUBST)
+	$(ENVSUBST) < $(E2E_TEMPLATE_FILE) > $(E2E_TEMPLATE_FILE_ENVSUBST)


do we really need a separate make target for env substitution? previously we had it within the e2e-tests target is not it?

furkatgofurov7 · 2021-06-29T13:52:11Z

test/e2e/config/e2e_conf.yaml

-    - sourcePath: "${PWD}/templates/test/cluster-template-prow-single-master.yaml"
-      targetName: "cluster-template-single-master.yaml"
-    - sourcePath: "${PWD}/templates/test/cluster-template-prow-ha.yaml"
+    - sourcePath: "${PWD}/templates/test/cluster-template-prow-ha_envsubst.yaml"


I missed this part while reviewing, would taking e2e-substitutions into e2e-tests make target help with that, because otherwise we would have to have _envsubst added for each template yaml naming where we could have multiple of them in the future

wgslr · 2021-06-29T14:51:12Z

/test-v1a5-e2e

wgslr · 2021-06-29T16:54:09Z

/test-v1a5-e2e
/test-integration

furkatgofurov7

/approve

metal3-io-bot · 2021-06-30T06:48:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7, lentzi90, namnx228

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:

~~OWNERS~~ [furkatgofurov7]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wgslr · 2021-06-30T07:18:15Z

/test-v1a5-e2e
/test-integration

jan-est · 2021-06-30T11:26:02Z

/lgtm

metal3-io-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 2, 2021

wgslr force-pushed the e2e-parametrization-wgslr branch from 872b3fc to de35a45 Compare June 2, 2021 14:00

metal3-io-bot requested a review from furkatgofurov7 June 2, 2021 14:03

wgslr changed the base branch from master to release-0.4 June 3, 2021 09:04

wgslr force-pushed the e2e-parametrization-wgslr branch 2 times, most recently from a81256c to 125bf96 Compare June 3, 2021 15:23

furkatgofurov7 reviewed Jun 3, 2021

View reviewed changes

furkatgofurov7 reviewed Jun 5, 2021

View reviewed changes

wgslr force-pushed the e2e-parametrization-wgslr branch 2 times, most recently from 4528697 to ae067ab Compare June 9, 2021 10:58

wgslr mentioned this pull request Jun 10, 2021

🌱 E2E tests CentOS support #225

Merged

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2021

wgslr changed the base branch from release-0.4 to master June 18, 2021 07:08

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2021

wgslr force-pushed the e2e-parametrization-wgslr branch from ae067ab to 7a0b48d Compare June 18, 2021 10:24

wgslr mentioned this pull request Jun 21, 2021

Add a script to run a command with environment variables metal3-io/metal3-dev-env#703

Merged

wgslr force-pushed the e2e-parametrization-wgslr branch 2 times, most recently from f4bd392 to 399a947 Compare June 21, 2021 12:09

macaptain reviewed Jun 21, 2021

View reviewed changes

wgslr commented Jun 21, 2021

View reviewed changes

wgslr force-pushed the e2e-parametrization-wgslr branch from 399a947 to 895f177 Compare June 21, 2021 13:28

fmuyassarov reviewed Jun 22, 2021

View reviewed changes

wgslr force-pushed the e2e-parametrization-wgslr branch from 895f177 to cf44319 Compare June 22, 2021 13:19

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2021

wgslr force-pushed the e2e-parametrization-wgslr branch from fb05f9d to 2a782ab Compare June 24, 2021 06:51

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2021

wgslr mentioned this pull request Jun 28, 2021

e2e: Adjust e2e tests to running in the CI #235

Merged

e2e: Adjust e2e tests to running in the CI

c1a1fed

wgslr force-pushed the e2e-parametrization-wgslr branch from cf61bba to 0a8c838 Compare June 29, 2021 07:32

namnx228 approved these changes Jun 29, 2021

View reviewed changes

furkatgofurov7 reviewed Jun 29, 2021

View reviewed changes

lentzi90 reviewed Jun 29, 2021

View reviewed changes

wgslr force-pushed the e2e-parametrization-wgslr branch from 0a8c838 to c7be3ca Compare June 29, 2021 10:25

furkatgofurov7 reviewed Jun 29, 2021

View reviewed changes

wgslr force-pushed the e2e-parametrization-wgslr branch from 3593b2b to 0cab4fa Compare June 29, 2021 16:53

lentzi90 approved these changes Jun 30, 2021

View reviewed changes

furkatgofurov7 approved these changes Jun 30, 2021

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 30, 2021

e2e: Use variable substitution in the test template

0e706fe

wgslr force-pushed the e2e-parametrization-wgslr branch from 0cab4fa to 0e706fe Compare June 30, 2021 07:17

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 30, 2021

metal3-io-bot merged commit dd40583 into metal3-io:master Jun 30, 2021

furkatgofurov7 mentioned this pull request Sep 20, 2021

🌱 e2e node_reuse test #267

Merged

kashifest deleted the e2e-parametrization-wgslr branch December 9, 2022 07:15

		M3_DEV_ENV_REPO="https://github.com/nordix/metal3-dev-env.git"
		M3_DEV_ENV_BRANCH="run-arbitrary-command-wgslr"

		export CAPI_VERSION=${CAPI_VERSION:-"v1alpha4"}
		export CAPM3_VERSION=${CAPM3_VERSION:-"v1alpha5"}

🌱E2E template parametrization #218

🌱E2E template parametrization #218

Conversation

wgslr commented Jun 2, 2021 • edited Loading

wgslr commented Jun 2, 2021

furkatgofurov7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgslr commented Jun 4, 2021

furkatgofurov7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmuyassarov left a comment

Choose a reason for hiding this comment

wgslr commented Jun 24, 2021

wgslr commented Jun 28, 2021

wgslr commented Jun 29, 2021

wgslr commented Jun 29, 2021

namnx228 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lentzi90 left a comment

Choose a reason for hiding this comment

wgslr commented Jun 29, 2021

lentzi90 commented Jun 29, 2021

wgslr commented Jun 29, 2021

jan-est commented Jun 29, 2021

furkatgofurov7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgslr commented Jun 29, 2021

wgslr commented Jun 29, 2021

furkatgofurov7 left a comment

Choose a reason for hiding this comment

metal3-io-bot commented Jun 30, 2021

wgslr commented Jun 30, 2021

jan-est commented Jun 30, 2021

wgslr commented Jun 2, 2021 •

edited

Loading