-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Ambient beta get started guide patch #14788
Ambient beta get started guide patch #14788
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
|
thanks Eric, I was using |
@ericvn what's the next step here? |
Co-authored-by: Frank Budinsky <[email protected]>
Co-authored-by: Frank Budinsky <[email protected]>
@@ -65,7 +65,7 @@ _verify_contains snip_verify_traffic_notsleep_to_productpage "$snip_verify_traff | |||
_verify_contains snip_adding_your_application_to_the_ambient_mesh_1 "$snip_adding_your_application_to_the_ambient_mesh_1_out" | |||
|
|||
# test traffic after ambient mode is enabled | |||
_verify_contains snip_adding_your_application_to_the_ambient_mesh_3 "$snip_adding_your_application_to_the_ambient_mesh_3_out" | |||
_verify_contains snip_adding_your_application_to_the_ambient_mesh_3 "received netns, starting proxy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frankbu I decided to do the validation directly on the test script as the snips keeps on failing. I see someone did the same before here on line 77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test failed again... running out of ideas here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VERIFY FAILED snip_adding_your_application_to_the_ambient_mesh_3 (timeout after 120s):
received:
""
expected:
"received netns, starting proxy"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth troubleshooting by dropping the grep in that snip and just having it dump the logs, just to see what is being logged in the ztunnel pods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @dhawton I found that one, I tried several combinations to get that snippet of the doc in but the test keeps on failing for different reasons. The idea here is to show the logs after the ns labeling, but when I use the whole output like:
kubectl logs ds/ztunnel -n istio-system | grep inpod
Found 3 pods, using pod/ztunnel-jrxln
inpod_enabled: true
inpod_uds: /var/run/ztunnel/ztunnel.sock
inpod_port_reuse: true
inpod_mark: 1337
2024-03-26T00:02:06.161802Z INFO ztunnel::inpod::workloadmanager: handling new stream
2024-03-26T00:02:06.162099Z INFO ztunnel::inpod::statemanager: pod received snapshot sent
2024-03-26T00:41:05.518194Z INFO ztunnel::inpod::statemanager: pod WorkloadUid("7ef61e18-725a-4726-84fa-05fc2a440879") received netns, starting proxy
the test _verify_like
or _verify_contains
doesn't match but I can see a similar print on the artifacts output, my last attempt just removed the snips.sh out reference and match the contains directly like:
_verify_contains snip_adding_your_application_to_the_ambient_mesh_3 "received netns, starting proxy"
but as you can see the output is empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the thing, on my previous validations I was able to see an output like described above but one pod instead of 3 and obviously different time stamps and workloadid but neither the _verify_like
or _verify_contains
matched anything...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see the output of the log really, I can only tell grep had no match... the received netns statement is ~2 months old, so it's in the commit of Istio that the docs is using atm. Wonder if it's not starting up, or hasn't finished initializing when the test is being run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/retest |
@@ -62,10 +62,9 @@ _verify_contains snip_verify_traffic_sleep_to_ingress "$snip_verify_traffic_slee | |||
_verify_contains snip_verify_traffic_sleep_to_productpage "$snip_verify_traffic_sleep_to_productpage_out" | |||
_verify_contains snip_verify_traffic_notsleep_to_productpage "$snip_verify_traffic_notsleep_to_productpage_out" | |||
|
|||
snip_adding_your_application_to_the_ambient_mesh_1 | |||
_verify_contains snip_adding_your_application_to_the_ambient_mesh_1 "$snip_adding_your_application_to_the_ambient_mesh_1_out" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not surprised it's failing now, since you're not calling snip_adding_your_application_to_the_ambient_mesh_2
Why didn't you change to the code I suggested in my previous comment? #14788 (review)
_verify_contains snip_adding_your_application_to_the_ambient_mesh_1 "$snip_adding_your_application_to_the_ambient_mesh_1_out" | |
_verify_contains snip_adding_your_application_to_the_ambient_mesh_1 "$snip_adding_your_application_to_the_ambient_mesh_1_out" | |
snip_adding_your_application_to_the_ambient_mesh_2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not calling it because when I generate the snips there is no snip_adding_your_application_to_the_ambient_mesh_2
and that is because as pointed out before there is no need to validate this section:
{{< text syntax=bash snip_id=none >}}
$ kubectl label namespace default istio.io/dataplane-mode=ambient
{{< /text >}}
as I'm not referencing any output on the document.
Are you saying that even though in this case I should remove the snip_id=none
let it generate the snip_adding_your_application_to_the_ambient_mesh_2
and just not call any validation from the test.sh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said that you don't need to validate the output of the snip, not that you don't need to call it. If you don't call the snip that adds the label, how could it possibly get labelled so that the following snips will work?
Just put it back to generate all the snips the way you had it previously and change the test to what I said yesterday.
_verify_same snip_adding_your_application_to_the_ambient_mesh_1 "$snip_adding_your_application_to_the_ambient_mesh_1_out"
# test traffic after ambient mode is enabled
snip_adding_your_application_to_the_ambient_mesh_2
_verify_like snip_adding_your_application_to_the_ambient_mesh_3 "$snip_adding_your_application_to_the_ambient_mesh_3_out"
_verify_same snip_adding_your_application_to_the_ambient_mesh_4 "$snip_adding_your_application_to_the_ambient_mesh_4_out"
_verify_same snip_adding_your_application_to_the_ambient_mesh_5 "$snip_adding_your_application_to_the_ambient_mesh_5_out"
_verify_same snip_adding_your_application_to_the_ambient_mesh_6 "$snip_adding_your_application_to_the_ambient_mesh_6_out"
Either that, or disable the test (rename it to test-disable.sh
) and I'll fix it in a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_verify_same
and _verify_contains
will both work in these cases, but _verify_same
is a better check since it really is checking that the output is "exactly the same as expected", which it is in this case. _verify_contains
checks that the expected string is part of the output, which could include more text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
understood, I think I tested with that and succeed locally but failed here. I will send one like this and we'll see, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/istio/istio.io/blob/master/tests/README.md#verify-functions
yeap, that's the one I have been using but I don't get consistent outputs when run here
/retest |
Co-authored-by: Frank Budinsky <[email protected]>
/test doc.test.profile-default |
/cherry-pick release-1.21 |
@dhawton: #14788 failed to apply on top of branch "release-1.21":
In response to this:
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. |
@dhawton: new issue created for failed cherrypick: #14801 In response to this:
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. |
* Sync #14788 into Chinese * improve * Apply suggestions from code review Co-authored-by: Michael <[email protected]> Co-authored-by: Xiaopeng Han <[email protected]> --------- Co-authored-by: Michael <[email protected]> Co-authored-by: Xiaopeng Han <[email protected]>
Description
adjusting ambient getting started guide to include in-pod ztunnel log proxy status #14355
Reviewers