Stabilize e2e dual-stack test

[unsortedhashsets]

What type of PR is this?

• Enhancement / New Feature
• Bug Fix
• Refactor
• Optimization
• Test
• Documentation Update

What this PR does / why we need it:

Allows wait inside CheckPodsReady func till pods will be ready (avoid errors during dual-stack tests like

[38;5;9m[FAILED] No pods found in dual-stack namespace
  Expected
      <[]v1.Pod | len:0, cap:0>: nil
  not to be empty�[0m

Same principle as used here: https://github.com/istio-ecosystem/sail-operator/blob/main/tests/e2e/controlplane/control_plane_test.go#L221

Which issue(s) this PR fixes:

Fixes #

Related Issue/PR #

Additional information:

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: unsortedhashsets / name: Mikhail Abramov (26f572e)

[istio-testing]

Hi @unsortedhashsets. Thanks for your PR.

I'm waiting for a istio-ecosystem or istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[sridhargaddam]

/ok-to-test

[sridhargaddam]

@unsortedhashsets did you see the error with Kind setup or OCP deployments?

[unsortedhashsets]

@unsortedhashsets did you see the error with Kind setup or OCP deployments?

With OCP, in general problem was in too fast check of pods existing

[fjglira]

What if we remove all the eventually inside the function and we call the eventually from outside, I mean. Something like this:

• Currently, we are using the function:

_, err = common.CheckPodsReady(ctx, cl, DualStackNamespace)
						Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("Error checking status of dual-stack pods: %v", err))

• Change the use for this:

Eventually(common.CheckPodsReady).WithArguments(ctx, cl, DualStackNamespace).Should(Succeed(), "Error checking status of dual-stack pods")

With this, we will use only one eventually, and we will simplify the code in the test. The only downside of this is that the function CheckPodsReady currently returns *corev1.PodList and error, and my suggestion will not work with those returns. But in my opinion, the function should only check that the pods in the namespace are ready, and after we check that the pods are ready and not return a list of pods, we can get the pods of the namespace in a following step to be used in later validations. Wdyt @sridhargaddam ?

[unsortedhashsets]

It has also use as:

sleepPod, err = common.CheckPodsReady(ctx, cl, SleepNamespace)

here:

• https://github.com/openshift-service-mesh/sail-operator/blob/main/tests/e2e/ambient/ambient_test.go#L267
• https://github.com/openshift-service-mesh/sail-operator/blob/main/tests/e2e/dualstack/dualstack_test.go#L219

[fjglira]

Yeah, in that case, I suggest avoiding using a function with the name CheckPodsReady to return a list of pods. So, we can check the pods are ready in the ns with a function that only checks the state of the pods, and then we can get the list of the pods from the ns to use it later in the next steps. We already get only the pods like that using the client:
For example:

					samplePodsCluster2 := &corev1.PodList{}
					It("updates the pods status to Ready", func(ctx SpecContext) {
						Expect(clRemote.List(ctx, samplePodsCluster2, client.InNamespace(sampleNamespace))).To(Succeed())
						Expect(samplePodsCluster2.Items).ToNot(BeEmpty(), "No pods found in sample namespace")

[fjglira]

@unsortedhashsets I mean this is not something strictly related to your PR, I'm just seeing that this can be improved also at the same time with your PR. Using two eventually inside the same function seems a little redundant for me, we can simplified easily moving our the eventually

[unsortedhashsets]

Thanks for clarification, will try that approach

[unsortedhashsets]

I have just replaced function call with two internal Eventually with more go one and external Eventually.
On dualstack works fine, also found few places to use in controle plane tests, lets see the test run.

[fjglira]

lgtm, only a smal small comment

[fjglira]

I suggested that you change the error message from Error deploying sample pod to Error getting the pods in sample namespace. Note: in all the places that you are using the same message :)

[unsortedhashsets]

Thanks, agree, it's describing operation better

[fjglira]

lgtm

[mkralik3]

@unsortedhashsets please, if possible, backport to release-1.26 as well as release-1.0