connectivity: Retry on inconclusive results #1543
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pchaigno
force-pushed
the
pr/pchaigno/retry-on-inconclusive-results
branch
from
e5e7733 to
ede4490
Compare
tklauser
requested changes
Apr 28, 2023
There was a problem hiding this comment.
This commit adds a check for those inconclusive results. If we see a seemingly successful command with no output, we retry it until we get something conclusive. This works because all our test commands (curl, ping, nslookup) dump something to stdout.
I'm a bit worried that at one point we start using commands that don't output to stdout and that will then fail due to this logic. But I don't know a better way to work around the issue we're seeing in cilium/cilium#22162. So let's give this a try and worry about this eventuality once we actually have such commands. Might be worth to add the above as a comment to the retry loop in (*Action).ExecInPod.
When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Unfortunately, those commands then exit without any error and are therefore considered successful. We think this is caused by connectivity blips between Kubernetes components. This commit adds a check for those inconclusive results. If we see a seemingly successful command with no output, we retry it until we get something conclusive. This works because all our test commands (curl, ping, nslookup) dump something to stdout. 1 - cilium/cilium#22162 Signed-off-by: Paul Chaignon <[email protected]>
pchaigno
force-pushed
the
pr/pchaigno/retry-on-inconclusive-results
branch
from
ede4490 to
6a4bdd4
Compare
I have the same concern. If that assumption doesn't hold anymore in the future, we should at least see a clear failure. It's also why I added a separate error message for the "inconclusive results": I'm hoping that makes the failure cause clearer if the assumption doesn't hold anymore. I've added a comment above the retry loop. |
maintainer-s-little-helper
bot
added
the
ready-to-merge
|
multicluster (helm) workflow is failing due to #1554: https://github.com/cilium/cilium-cli/actions/runs/4829721713/jobs/8605084154?pr=1543 All other workflows passed. Merging. |
mhofstetter
added a commit
to mhofstetter/cilium-cli
that referenced
this pull request
Jul 21, 2023
When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Therefore, successful command terminations with an empty output are treated as inconclusive that result in a retry [2]. Unfortunately, the ping command might get stuck by only outputting its header line to the output. `PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.` This commit adds an additional check for the ping commands. If we see an output only containing the header, we treat it as inconclusive result. 1 - cilium/cilium#22162 2 - cilium#1543 Signed-off-by: Marco Hofstetter <[email protected]>
mhofstetter
added a commit
to mhofstetter/cilium-cli
that referenced
this pull request
Jul 21, 2023
When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Therefore, successful command terminations with an empty output are treated as inconclusive that result in a retry [2]. Unfortunately, the ping command might get stuck by only outputting its header line to the output. `PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.` This commit adds an additional check for the ping commands. If we see an output only containing the header, we treat it as inconclusive result. 1 - cilium/cilium#22162 2 - cilium#1543 Signed-off-by: Marco Hofstetter <[email protected]>
mhofstetter
added a commit
to mhofstetter/cilium-cli
that referenced
this pull request
Jul 21, 2023
When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Therefore, successful command terminations with an empty output are treated as inconclusive that result in a retry [2]. Unfortunately, the ping command might get stuck by only outputting its header line to the output. `PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.` This commit adds an additional check for the ping commands. If we see an output only containing the header, we treat it as inconclusive result. 1 - cilium/cilium#22162 2 - cilium#1543 Signed-off-by: Marco Hofstetter <[email protected]>
michi-covalent
pushed a commit
that referenced
this pull request
Jul 25, 2023
When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Therefore, successful command terminations with an empty output are treated as inconclusive that result in a retry [2]. Unfortunately, the ping command might get stuck by only outputting its header line to the output. `PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.` This commit adds an additional check for the ping commands. If we see an output only containing the header, we treat it as inconclusive result. 1 - cilium/cilium#22162 2 - #1543 Signed-off-by: Marco Hofstetter <[email protected]>
liyihuang
pushed a commit
to liyihuang/cilium-cli
that referenced
this pull request
Aug 1, 2023
When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Therefore, successful command terminations with an empty output are treated as inconclusive that result in a retry [2]. Unfortunately, the ping command might get stuck by only outputting its header line to the output. `PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.` This commit adds an additional check for the ping commands. If we see an output only containing the header, we treat it as inconclusive result. 1 - cilium/cilium#22162 2 - cilium#1543 Signed-off-by: Marco Hofstetter <[email protected]>
michi-covalent
pushed a commit
to cilium/cilium
that referenced
this pull request
Jul 16, 2024
When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Therefore, successful command terminations with an empty output are treated as inconclusive that result in a retry [2]. Unfortunately, the ping command might get stuck by only outputting its header line to the output. `PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.` This commit adds an additional check for the ping commands. If we see an output only containing the header, we treat it as inconclusive result. 1 - #22162 2 - cilium/cilium-cli#1543 Signed-off-by: Marco Hofstetter <[email protected]>
michi-covalent
pushed a commit
to cilium/cilium
that referenced
this pull request
Aug 5, 2024
[ cherry-picked from cilium/cilium-cli repository ] When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Therefore, successful command terminations with an empty output are treated as inconclusive that result in a retry [2]. Unfortunately, the ping command might get stuck by only outputting its header line to the output. `PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.` This commit adds an additional check for the ping commands. If we see an output only containing the header, we treat it as inconclusive result. 1 - #22162 2 - cilium/cilium-cli#1543 Signed-off-by: Marco Hofstetter <[email protected]>
github-merge-queue bot
pushed a commit
to cilium/cilium
that referenced
this pull request
Aug 16, 2024
[ cherry-picked from cilium/cilium-cli repository ] When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Therefore, successful command terminations with an empty output are treated as inconclusive that result in a retry [2]. Unfortunately, the ping command might get stuck by only outputting its header line to the output. `PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.` This commit adds an additional check for the ping commands. If we see an output only containing the header, we treat it as inconclusive result. 1 - #22162 2 - cilium/cilium-cli#1543 Signed-off-by: Marco Hofstetter <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When running the connectivity tests in AKS, we sometimes get interrupted commands that don't have any output [1]. Unfortunately, those commands then exit without any error and are therefore considered successful. We think this is caused by connectivity blips between Kubernetes components.
This commit adds a check for those inconclusive results. If we see a seemingly successful command with no output, we retry it until we get something conclusive. This works because all our test commands (curl, ping, nslookup) dump something to stdout.
1 - cilium/cilium#22162