Modifications to Run integration Tests With Sail Operator #300

ctartici · 2025-02-24T12:05:59Z

Please provide a description of this PR:

To help us figure out who should review this PR, please put an X in all the areas that this PR affects.

Please check any characteristics that apply to this pull request.

Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

fjglira · 2025-02-24T12:14:58Z

prow/integ-suite-ocp.sh

@@ -135,6 +135,13 @@ base_cmd=("go" "test" "-p" "1" "-v" "-count=1" "-tags=integ" "-vet=off" "-timeou
          "--istio.test.tag=${TAG}"
          "--istio.test.openshift")

+# Append sail operator setup script to base command
+if [ "${OPERATOR_TYPE:-}" == "sail" ]; then


You need to initialize at the beginning of the file OPERATOR_TYPE with some default value, but I don't like the name of the var. We are going to run the sail-operator-setup bash script to avoid doing some hacks or workaround when we want to run this integration test on OCP and against sail operator-based installed control plane, I think we can name this var something like CONTROL_PLANE_INSTALLER (The same as the istio flag) and the default value should be istio. And you can add here also some notes to explain the flags

Also, Check the script we will need later to delete SKIP_TEST_RUN because it is not going to be used anymore, but this can be done after the merge of the PR.

You need to initialize at the beginning of the file OPERATOR_TYPE with some default value, but I don't like the name of the var. We are going to run the sail-operator-setup bash script to avoid doing some hacks or workaround when we want to run this integration test on OCP and against sail operator-based installed control plane, I think we can name this var something like CONTROL_PLANE_INSTALLER (The same as the istio flag) and the default value should be istio. And you can add here also some notes to explain the flags

CONTROL_PLANE_INSTALLER is referring to this file in istio flag. So it might be confusing to use it for something else. I can change it to CONTROL_PLANE_TYPE and default it to istio on top of the script. WDYT?

Maybe it can be CONTROL_PLANE_SOURCE? And the values can be sail or istio

fjglira · 2025-02-24T12:19:06Z

prow/config/sail-operator/istioCNI-cr.yaml

@@ -0,0 +1,8 @@
+apiVersion: sailoperator.io/v1alpha1


Maybe rename this file to istio-cni.yaml only

fjglira

I like this :), I leave some comments mostly for clarity. Please tell me and I can add a new job to start running test over this new way to execute the test

prow/setup/sail-operator-setup.sh

fjglira · 2025-02-24T12:22:27Z

prow/setup/sail-operator-setup.sh

+WORKDIR="$2"
+IOP_FILE="$2"/iop.yaml
+SAIL_IOP_FILE="$(basename "${IOP_FILE%.yaml}")-sail.yaml"


Add note to explain what is the iop file

fjglira · 2025-02-24T12:23:22Z

prow/setup/sail-operator-setup.sh

+IOP_FILE="$2"/iop.yaml
+SAIL_IOP_FILE="$(basename "${IOP_FILE%.yaml}")-sail.yaml"
+
+ISTIO_VERSION="${ISTIO_VERSION:-v1.24.1}"


The current Istio version is 1.24.2; is there any chance to fill this by default? I don't know if it is possible to be honest

One option is to use "latest" as istio version, I am not sure if it works for istio-cni but works for istiod. If you think its better idea I can modify default to latest and test it

OK, try this

fjglira · 2025-02-24T12:23:50Z

prow/setup/sail-operator-setup.sh

+INGRESS_GATEWAY_SVC_NAMESPACE="${INGRESS_GATEWAY_SVC_NAMESPACE:-istio-system}"
+ISTIOCNI_NAMESPACE="${ISTIOCNI_NAMESPACE:-istio-cni}"
+
+ISTIOCNI="${PROW}/config/sail-operator/istioCNI-cr.yaml"


If you change the name of the file, this will need to change also

prow/setup/sail-operator-setup.sh

fjglira · 2025-02-24T13:17:00Z

/test istio-integration-telemetry

fjglira · 2025-02-25T10:10:28Z

/retest

ctartici · 2025-02-25T13:05:27Z

/test istio-telemetry-sail-controlpane

openshift-ci · 2025-02-25T13:05:31Z

@ctartici: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test istio-integration-helm

/test istio-integration-pilot

/test istio-integration-telemetry

The following commands are available to trigger optional jobs:

/test istio-telemetry-sail-controlplane

Use /test all to run all jobs.

In response to this:

/test istio-telemetry-sail-controlpane

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-sigs/prow repository.

ctartici · 2025-02-25T13:06:51Z

/retest

fjglira · 2025-02-25T16:14:04Z

Important: Adding do not merge label until INSTALL_SAIL_OPERATOR is set to false

openshift-ci · 2025-02-28T14:57:00Z

@ctartici: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test istio-integration-helm

/test istio-integration-pilot

/test istio-integration-telemetry

The following commands are available to trigger optional jobs:

/test istio-telemetry-sail-controlplane

Use /test all to run all jobs.

In response to this:

/test ci/prow/istio-telemetry-sail-controlplane

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-sigs/prow repository.

ctartici · 2025-02-28T14:57:14Z

/test istio-telemetry-sail-controlplane

luksa · 2025-03-03T09:07:47Z

prow/config/sail-operator/istio-cni.yaml

+metadata:
+  name: default
+spec:
+  namespace: ${ISTIOCNI_NAESPACE}


Typo:

Suggested change

namespace: ${ISTIOCNI_NAESPACE}

namespace: ${ISTIOCNI_NAMESPACE}

ctartici · 2025-03-26T14:56:53Z

/test istio-telemetry-sail-controlplane

ctartici · 2025-03-26T16:59:31Z

/test istio-telemetry-sail-controlplane

ctartici · 2025-03-27T08:29:39Z

/test istio-telemetry-sail-controlplane

ctartici · 2025-03-31T09:37:45Z

/test istio-telemetry-sail-controlplane

fjglira · 2025-04-03T12:38:55Z

prow/config/sail-operator/istio-cni.yaml

+spec:
+  namespace: ${ISTIOCNI_NAMESPACE}
+  version: ${ISTIO_VERSION}
+  profile: openshift


Question: is a profile needed?

I am not sure about this. Got the yaml from our jenkins config files. I can remove and test it.

Let's test in a consecutive PR

fjglira

lgtm in general I just leave some minor comments

prow/config/sail-operator/validatingwebhook.yaml

prow/setup/ocp_setup.sh

prow/setup/sail-operator-setup.sh

fjglira · 2025-04-03T13:01:01Z

If is ready, you can set the PR to ready to review to execute the prow jobs

fjglira · 2025-04-03T13:27:25Z

prow/setup/sail-operator-setup.sh

+IOP_FILE="$2"/iop.yaml
+SAIL_IOP_FILE="$(basename "${IOP_FILE%.yaml}")-sail.yaml"
+
+ISTIO_VERSION="${ISTIO_VERSION:-v1.24-latest}"


the current master branch of Istio is 1.25 but we do not support it officially, in our main branch we can deploy master. Maybe worth checking if we can set an alias here to deploy by default the master alias

There is this file stating version: https://github.com/ctartici/istio_ossm/blob/master/VERSION but it has 1.26 for now. Where should I read the version?

I can use this one I guess, WDYT? https://github.com/ctartici/sail-operator/blob/main/pkg/istioversion/versions.yaml

Yes, using the versions.yaml file will be the best I think

fjglira

Just one more comment related to the Istio resource version

ctartici · 2025-04-03T14:33:46Z

/test istio-integration-telemetry

openshift-ci · 2025-04-07T13:00:17Z

@ctartici: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/istio-pilot-sail-controlplane	`0e8c623`	link	false	`/test istio-pilot-sail-controlplane`
ci/prow/istio-telemetry-sail-controlplane	`6d2370e`	link	false	`/test istio-telemetry-sail-controlplane`

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

ctartici · 2025-04-07T13:08:07Z

/test istio-integration-helm

fjglira · 2025-04-07T15:48:49Z

prow/setup/sail-operator-setup.sh

+# get istio version from versions.yaml
+VERSION_FILE="https://raw.githubusercontent.com/istio-ecosystem/sail-operator/$CONVERTER_BRANCH/pkg/istioversion/versions.yaml"
+if [ -z "${ISTIO_VERSION:-}" ]; then
+  ISTIO_VERSION="$(curl -s "$VERSION_FILE" | grep -E 'name: v[0-9]+\.[0-9]+' | sed -E 's/.*(v[0-9]+\.[0-9]+).*/\1/' | sort -Vr | head -n1)-latest"
+fi


Very small change: I think it will be better to set also branch alternative to be able to run in the master branch against the master of the versions.yaml and we can set it later for another branch the same. It will be a small change

fjglira

really change

fjglira

Let's merge this and start to make changes from there if they are needed

ctartici requested a review from fjglira February 24, 2025 12:05

openshift-ci bot added the size/L label Feb 24, 2025

ctartici requested review from dgn, asmigala and MaxBab February 24, 2025 12:07

fjglira reviewed Feb 24, 2025

View reviewed changes

ctartici requested a review from FilipB February 24, 2025 12:59

ctartici force-pushed the master branch from aa3f8b5 to 9590af4 Compare February 24, 2025 15:08

ctartici force-pushed the master branch from 9590af4 to 49871ce Compare February 25, 2025 12:17

fjglira added the do-not-merge/hold label Feb 25, 2025

ctartici force-pushed the master branch 3 times, most recently from c2ab34c to 44ff87d Compare February 26, 2025 11:30

ctartici marked this pull request as draft February 26, 2025 14:16

openshift-ci bot added the do-not-merge/work-in-progress label Feb 26, 2025

ctartici force-pushed the master branch from 44ff87d to bdadf6a Compare February 28, 2025 13:07

ctartici force-pushed the master branch from bdadf6a to faa6a7d Compare March 3, 2025 09:04

luksa reviewed Mar 3, 2025

View reviewed changes

ctartici force-pushed the master branch 2 times, most recently from f9ef06b to e16e671 Compare March 4, 2025 10:29

Merge remote-tracking branch 'upstream/master'

b9f9ac3

ctartici force-pushed the master branch from 0e8c623 to b9f9ac3 Compare March 26, 2025 16:59

Skip failing tests in telemetry suite.

6d2370e

ctartici force-pushed the master branch from 3336a41 to 6d2370e Compare March 31, 2025 09:36

ctartici force-pushed the master branch from 984c822 to 70269bc Compare April 3, 2025 12:24

fjglira reviewed Apr 3, 2025

View reviewed changes

fjglira removed do-not-merge/work-in-progress do-not-merge/hold labels Apr 3, 2025

some workarounds for failing tests

e848cb7

ctartici force-pushed the master branch from 70269bc to e848cb7 Compare April 3, 2025 13:09

ctartici marked this pull request as ready for review April 3, 2025 13:10

fjglira reviewed Apr 3, 2025

View reviewed changes

ctartici force-pushed the master branch from 8959d9b to 42f9ba4 Compare April 7, 2025 12:07

fjglira reviewed Apr 7, 2025

View reviewed changes

get latest version from sail repo

85b898e

ctartici force-pushed the master branch from 42f9ba4 to 85b898e Compare April 7, 2025 15:57

fjglira approved these changes Apr 7, 2025

View reviewed changes

fjglira added the okay to merge label Apr 8, 2025

openshift-merge-bot bot merged commit 85e1ac5 into openshift-service-mesh:master Apr 8, 2025
4 checks passed

	namespace: ${ISTIOCNI_NAESPACE}
	namespace: ${ISTIOCNI_NAMESPACE}

Modifications to Run integration Tests With Sail Operator #300

Modifications to Run integration Tests With Sail Operator #300

Conversation

ctartici commented Feb 24, 2025

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

fjglira 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

fjglira commented Feb 24, 2025

fjglira commented Feb 25, 2025

ctartici commented Feb 25, 2025

openshift-ci bot commented Feb 25, 2025

ctartici commented Feb 25, 2025

fjglira commented Feb 25, 2025

openshift-ci bot commented Feb 28, 2025

ctartici commented Feb 28, 2025

Choose a reason for hiding this comment

ctartici commented Mar 26, 2025

ctartici commented Mar 26, 2025

ctartici commented Mar 27, 2025

ctartici commented Mar 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjglira left a comment

Choose a reason for hiding this comment

fjglira commented Apr 3, 2025

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

fjglira left a comment

Choose a reason for hiding this comment

ctartici commented Apr 3, 2025

openshift-ci bot commented Apr 7, 2025 • edited Loading

ctartici commented Apr 7, 2025

Choose a reason for hiding this comment

fjglira left a comment

Choose a reason for hiding this comment

fjglira left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 7, 2025 •

edited

Loading