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

[release-1.21] quick verbiage adjustment to be accurate #14895

Conversation

nauticalmike
Copy link
Contributor

Description

Ambient beta get started guide patch3 based on #14805 per @linsun

cc. @bleggett

Reviewers

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

@nauticalmike nauticalmike requested a review from a team as a code owner April 17, 2024 15:08
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test labels Apr 17, 2024
@istio-testing
Copy link
Contributor

Hi @nauticalmike. 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/test-infra repository.

Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

Can we enable the test? I'm not comfortable to have test disabled for this page for release 1.21. It should be working, no?

ericvn
ericvn previously requested changes Apr 17, 2024
Copy link
Contributor

@ericvn ericvn left a comment

Choose a reason for hiding this comment

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

Need to continue to test.

@ericvn
Copy link
Contributor

ericvn commented Apr 17, 2024

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Apr 17, 2024
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 17, 2024
@ericvn ericvn dismissed their stale review April 17, 2024 19:37

Change was reverted (snippets updated)

@nauticalmike
Copy link
Contributor Author

@linsun @dhawton @ericvn need advise here, this is the same that was happening to me on the other PR, pretty much simple validations fail:
_verify_contains snip_adding_your_application_to_the_ambient_mesh_3 "received netns, starting proxy"

I already tried _verify_like, leaving it on the snips etc etc and tests fail. What other options do we have here?
https://storage.googleapis.com/istio-prow/pr-logs/pull/istio_istio.io/14895/doc.test.profile-none_istio.io_release-1.21/1780955318851735552/artifacts/tests-setup-profile-none-4a7e8e/TestDocs/ops/ambient/getting-started/test.sh/test.sh/_test_context/test.sh_output.txt

@ericvn
Copy link
Contributor

ericvn commented Apr 18, 2024

Looking at the last failure:

VERIFY FAILED snip_adding_your_application_to_the_ambient_mesh_3 (timeout after 120s):
received:
"2024-04-18T14:02:41.712161Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("a992e39d-c294-4f40-985c-520739a56e86") received netns, starting proxy
2024-04-18T14:02:41.794754Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("cd515845-1c7d-4b74-a35a-c70782b9c8b8") received netns, starting proxy
2024-04-18T14:02:41.859111Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("b4e6db0c-a486-43b1-821b-acef6babec39") received netns, starting proxy
2024-04-18T14:02:41.930326Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("31a16751-73ba-4e78-9f39-4791ad4a6cd0") received netns, starting proxy
2024-04-18T14:02:42.008360Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("d1ce26c4-1636-48f2-bddb-1f782b9c8309") received netns, starting proxy
2024-04-18T14:02:42.072142Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("2f1a61e4-8e64-4c43-a0f3-073fadd3df64") received netns, starting proxy
2024-04-18T14:02:42.130898Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("08bc5ad6-78de-4d1a-acc3-0c6fc2ce74f2") received netns, starting proxy
2024-04-18T14:02:42.206381Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("86e407d3-ea5c-4508-8457-777803582b05") received netns, starting proxy"
expected:
"received netns, starting proxy"

and the test.sh for that line:

_verify_like snip_adding_your_application_to_the_ambient_mesh_3 "received netns, starting proxy"

It looks like that maybe should be a verify_contains.

Changes like this may make the test pass, but I have no idea on what the expected output should really be. Writing the test to pass based on trying to match output may or may not be the correct answer.

@nauticalmike
Copy link
Contributor Author

Looking at the last failure:

VERIFY FAILED snip_adding_your_application_to_the_ambient_mesh_3 (timeout after 120s):
received:
"2024-04-18T14:02:41.712161Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("a992e39d-c294-4f40-985c-520739a56e86") received netns, starting proxy
2024-04-18T14:02:41.794754Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("cd515845-1c7d-4b74-a35a-c70782b9c8b8") received netns, starting proxy
2024-04-18T14:02:41.859111Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("b4e6db0c-a486-43b1-821b-acef6babec39") received netns, starting proxy
2024-04-18T14:02:41.930326Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("31a16751-73ba-4e78-9f39-4791ad4a6cd0") received netns, starting proxy
2024-04-18T14:02:42.008360Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("d1ce26c4-1636-48f2-bddb-1f782b9c8309") received netns, starting proxy
2024-04-18T14:02:42.072142Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("2f1a61e4-8e64-4c43-a0f3-073fadd3df64") received netns, starting proxy
2024-04-18T14:02:42.130898Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("08bc5ad6-78de-4d1a-acc3-0c6fc2ce74f2") received netns, starting proxy
2024-04-18T14:02:42.206381Z  INFO ztunnel::inpod::statemanager: pod WorkloadUid("86e407d3-ea5c-4508-8457-777803582b05") received netns, starting proxy"
expected:
"received netns, starting proxy"

and the test.sh for that line:

_verify_like snip_adding_your_application_to_the_ambient_mesh_3 "received netns, starting proxy"

It looks like that maybe should be a verify_contains.

I already tried that too...

@ericvn
Copy link
Contributor

ericvn commented Apr 18, 2024

I will also note that this test had some issues on the main branch, and was disabled (and needs to be fixed before we release 1.22.0): istio/istio#50145

@nauticalmike
Copy link
Contributor Author

Can I just remove this validation while the deeper test problems get fixed?

@nauticalmike
Copy link
Contributor Author

@ericvn @linsun is there anything else I need to do on my end for this one?

@linsun
Copy link
Member

linsun commented Apr 22, 2024

FYI, the test has been enabled on the master branch. I also removed some in pod checking for master branch as they are not APIs.

For 1.21, i think it is okay to have the inpod checking as long as they work properly as this is the first release inpod is introduced.

Copy link
Contributor

@ericvn ericvn left a comment

Choose a reason for hiding this comment

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

Needs a make gen to update the scripts, but looks good. Thanks.

@ericvn ericvn changed the title quick verbiage adjustment to be accurate [release-1.21] quick verbiage adjustment to be accurate Apr 23, 2024
@istio-testing istio-testing merged commit f335826 into istio:release-1.21 Apr 23, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ambient kind/docs ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants