Fix --pause-on-fail

[joestringer]

cilium connectivity test --pause-on-fail wasn't working before because the
individual tests would fail out of the goroutine and the wrapping logic
wouldn't properly detect and enforce the pause. Add it into the fail/fatal
commands to make sure it takes effect, even if that means blocking an
individual test goroutine on user input.

Fixes: #2033

[michi-covalent]

i mildly prefer deferring the whole post-failure logic like this 8000548 so that we also get printing flows and capturing a sysdump on failure 💭

[joestringer]

👍 I'm not opinionated at all. That commit doesn't work for me, but happy to try any alternative proposal.

[michi-covalent]

alright let me try and figure out why it's not working for you 💭

[michi-covalent]

alright let me try and figure out why it's not working for you 💭

sorry i'm going back and forth on this, now i like this better than 8000548. could you update this pr:

• move https://github.com/cilium/cilium-cli/blob/main/connectivity/check/action.go#L217-L224 into Test.failCommon(), not just PauseOnFail part, and delete the block from action.go.
• delete https://github.com/cilium/cilium-cli/blob/main/connectivity/check/context.go#L379-L381

[joestringer]

Hmm, I think it's pausing twice with that proposal:

[=] Test [to-cidr-external]
  ℹ️  📜 Applying CiliumNetworkPolicy 'client-egress-to-cidr' to namespace 'cilium-test'..
  [-] Scenario [to-cidr-external/pod-to-cidr]
  [.] Action [to-cidr-external/pod-to-cidr/external-1111-0: cilium-test/client-84bfddc76b-njg5j (10.244.3.22) -> external-1111 (1.1.1.1:443)]
  Pausing after action failure, press the Enter key to continue:

  ❌ command "curl -w %{local_ip}:%{local_port} -> %{remote_ip}:%{remote_port} = %{response_code} --silent --fail --show-error --output /dev/null --connect-timeout 2 --max-time 10 --retry 3 --retry-all-errors --retry-delay 3 https://1.1.1.1:443" failed: command terminated with exit code 28
  ℹ️  curl output:


  📄 No flows recorded for peer cilium-test/client-84bfddc76b-njg5j during action external-1111-0
  📄 No flows recorded for peer external-1111 during action external-1111-0
  Pausing after action failure, press the Enter key to continue:

[joestringer]

(updated to follow your latest feedback)

This first pause is actually not great, because it's pausing before printing the debuggability steps:

  [.] Action [to-cidr-external/pod-to-cidr/external-1111-0: cilium-test/client-84bfddc76b-njg5j (10.244.3.22) -> external-1111 (1.1.1.1:443)]
  Pausing after action failure, press the Enter key to continue:

  ❌ command "curl -w %{local_ip}:%{local_port} -> %{remote_ip}:%{remote_port} = %{response_code} --silent --fail --show-error --output /dev/null --connect-timeout 2 --max-time 10 --retry 3 --retry-all-errors --retry-delay 3 https://1.1.1.1:443" failed: command terminated with exit code 28
  ℹ️  curl output:

[michi-covalent]

oh yeah i think we can just delete ace4127#diff-e3c06da3c6d8b3d471002757d5e992af824a412431f22e7f18f5f245f35552adR217. no need to call failCommon() from there 💡

[joestringer]

Hmm, unfortunately that's the one that is printing after printing the other output that's relevant to debugging the test. Deleting that one will prevent there being double pause, but it also deletes the pause at the most useful time..

[joestringer]

For what it's worth, I think that this model where individual tests know about the control flow of the overall testing and run Fatal() to try to interrupt the test flow breaks the abstraction a bit.. a test should either fail or not and then leave it up to the CLI infrastructure to appropriately terminate and implement all the other bells and whistles (like the pause on failure, sysdump, etc.)

[michi-covalent]

would it help if we swap these 2 lines => https://github.com/cilium/cilium-cli/pull/2036/files#diff-66a6f82a060f753d91cfb453548d1f41fd695abde4debdb8cc5b982b6b1260beR314-R315

[joestringer]

I'm not sure about the significance of moving the flush until after the log, but swapping the order has the desired effect of seeing the failure first then pausing.

[michi-covalent]

✨