remove use of istioctl x wait

[AritraDey-Dev]

Description

fixes #16429

Reviewers

• Ambient
• Docs
• Installation
• Networking
• Performance and Scalability
• Extensions and Telemetry
• Security
• Test and Release
• User Experience
• Developer Infrastructure
• Localization/Translation

[istio-testing]

Hi @AritraDey-Dev. Thanks for your PR.

I'm waiting for a 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.

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.

[Arhell]

/ok-to-test

[AritraDey-Dev]

/retest

[AritraDey-Dev]

/test doc.test.profile-default

[craigbox]

/retest

[craigbox]

Does this PR cause a meaningful change to the time the tests take to run?

[craigbox]

Aren't you basically doing

kubectl wait --for=create -n $namespace $kind/$name --timeout=30s

with this whole function?

Perhaps that would be cleaner?

[craigbox]

That's what some of the other functions do.

The thing here is, that the goal of this wasn't so much to ensure that the configuration was created in Kubernetes, which either method checks for, but to ensure all the sidecars were programmed with it. They generally are, but in the instance that the object is created but is not translated to XDS and sent to the other proxy servers, there's no obvious way to tell.

So kubectl wait is probably necessary but not sufficient.

[craigbox]

Congratulations, your testing has found a bug!

In #13208 the doc was changed to not require samples/bookinfo/networking/virtual-service-all-v1.yaml, so checking for productpage and details will fail as those are not actually created.

I'll change the test to only check for the services that are used by this example. The cleanup will remove all of them, whether they exist or not.

[craigbox]

That one feels a little more like a flake I've seen before.

/retest

[AritraDey-Dev]

I'll change the test to only check for the services that are used by this example. The cleanup will remove all of them, whether they exist or not.

Thank you for catching that and updating the test accordingly!

[craigbox]

np. Before we merge, please change this function to use kubectl wait and maybe even rename the function to _wait_for_resource throughout all the tests?

[AritraDey-Dev]

please change this function to use kubectl wait and maybe even rename the function to _wait_for_resource throughout all the tests?

I have updated the implementation to use kubectl wait, and also renamed the function to _wait_for_resource throughout all the tests.

[craigbox]

https://prow.istio.io/view/gs/istio-prow/pr-logs/pull/istio_istio.io/16515/doc.test.profile-default_istio.io/1928015336469172224

https://storage.googleapis.com/istio-prow/pr-logs/pull/istio_istio.io/16515/doc.test.profile-default_istio.io/1928015336469172224/artifacts/tests-setup-profile-default-a89/TestDocs/tasks/traffic-management/ingress/secure-ingress/test.sh/test.sh/_test_context/test.sh_debug.txt

Interesting error

+_wait_for_istio gateway default mygateway
+_wait_for_istio(): local kind=gateway
+_wait_for_istio(): local namespace=default
+_wait_for_istio(): local name=mygateway
+_wait_for_istio(): kubectl wait --for=create -n default gateway/mygateway --timeout 30s
+_wait_for_istio virtualservice default helloworld-v1
+_wait_for_istio(): local kind=virtualservice
+_wait_for_istio(): local namespace=default
+_wait_for_istio(): local name=helloworld-v1
+_wait_for_istio(): kubectl wait --for=create -n default virtualservice/helloworld-v1 --timeout 30s
+_wait_for_istio(): echo 'Timed out waiting for virtualservice helloworld-v1 in namespace default to be created.'
+_wait_for_istio(): exit 1

Either this is a flake, or it's confused by (a) there being two different types of Gateway (b) waiting on create where a CRD should be condition=established ?

(Again, this shows why _wait_for_istio and kubectl wait are not the same)

[craigbox]

/test doc.test.profile-default

[AritraDey-Dev]

/retest

[AritraDey-Dev]

Our options here are (a) always sleep 1s, and hope that is enough (b) sleep something like 5s in every test where we install httpbin. I'd like feedback from other docs maintainers here.

As we're encountering edge cases where config propagation is still flaky, I guess we could consider adding an optional advanced wait mechanism in the future (e.g., polling Envoy config or using istioctl proxy-config).
For now, I'm adding a sleep 2s just to check the logs of this test. A simple sleep 1s might be sufficient, but this is for initial validation.

[AritraDey-Dev]

/test doc.test.profile-default

[dhawton]

/test doc.test.profile-default

[dhawton]

These aren't flakes it seems..

[AritraDey-Dev]

/retest

[AritraDey-Dev]

The real issue appears to be Istiod control plane health or authentication problems, not the _wait_for_istio helper. I believe the root cause is unrelated to this script and likely due to Istiod not being healthy or reachable during the tests.

look at the logs here...
https://storage.googleapis.com/istio-prow/pr-logs/pull/istio_istio.io/16515/doc.test.profile-default_istio.io/1929316089271947264/build-log.txt

[AritraDey-Dev]

/retest

[AritraDey-Dev]

Hi @craigbox @dhawton,
Just checking in on this PR as it has been on hold for a while.Any thoughts on this?

[craigbox]

The real issue appears to be Istiod control plane health or authentication problems, not the _wait_for_istio helper. I believe the root cause is unrelated to this script and likely due to Istiod not being healthy or reachable during the tests.

look at the logs here... https://storage.googleapis.com/istio-prow/pr-logs/pull/istio_istio.io/16515/doc.test.profile-default_istio.io/1929316089271947264/build-log.txt

I read that error as "test failed, trying to clean up, error logged while cleaning up"

I'm not 100% sure how best to progress on this. Right now it seems like we've just slowed everything down by 1s per test?
Daniel asked that we put back the time check.

[dhawton]

The real issue appears to be Istiod control plane health or authentication problems, not the _wait_for_istio helper. I believe the root cause is unrelated to this script and likely due to Istiod not being healthy or reachable during the tests.
look at the logs here... https://storage.googleapis.com/istio-prow/pr-logs/pull/istio_istio.io/16515/doc.test.profile-default_istio.io/1929316089271947264/build-log.txt

I read that error as "test failed, trying to clean up, error logged while cleaning up"

I'm not 100% sure how best to progress on this. Right now it seems like we've just slowed everything down by 1s per test? Daniel asked that we put back the time check.

I agree that we slowed everything down.. unfortunately, whichout a replacement for the old x wait command of istioctl, I don't know of a way to properly apply and wait for programming, deployment, etc. It's really unfortuante that that experimental command was dropped,.

I'm ok with implementing this for now... it makes things slower but hopefully will address some of the flakes that happen because we checked or tried to use something that hadn't yet applied.

[craigbox]

Ok, approving, and we'll see what the impact is to other PRs.