-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Bug 1481147 - Fix default pod image for network diagnostics #16439
Bug 1481147 - Fix default pod image for network diagnostics #16439
Conversation
@openshift/networking @knobunc PTAL |
[test] |
- This also ensures network diagnostics pod and test pod images uses deployed openshift version/tag (not the latest) so that it doesn't need to download another same image with latest tag.
Currently when network diags fails, it only informs how many test pods failed but doesn't provide why those pods failed. This change will fetch logs for the pods in case of setup failure.
fa00ddd
to
74a6699
Compare
@sosiouxme Should this be assigned to you? |
looks like a flake As an assignee, what am I supposed to do? Review only? Give a /lgtm? Doesn't look like I need to /approve. |
@sosiouxme merge-bot says this pr needs an approver from pkg/diagnostics/OWNERS and pkg/oc/OWNERS |
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.
LGTM, small nit but otherwise good stuff. Thanks!
@@ -131,6 +123,14 @@ func GetTestPod(testPodImage, testPodProtocol, podName, nodeName string, testPod | |||
return pod | |||
} | |||
|
|||
func getTrimmedImage(image string) string { | |||
// Image format could be: [<dns-name>/]openshift/origin-deployer[:<tag>] | |||
tokens := strings.Split(image, "/") |
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.
Can't you use https://golang.org/pkg/strings/#SplitN
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.
We don't know how many /
s will be present in the given image (dns path may have many) and we only care about the last token.
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.
Oh, the last. Gotcha!
/approve |
/lgtm |
@pravisankar you're also an approver for that path so my approval is superfluous :) |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dcbw, knobunc, pravisankar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue |
This also ensures network diagnostics pod and test pod images uses
deployed openshift version/tag (not the latest) so that it doesn't
need to download another same image with latest tag.
Print more details when network diagnostics test setup fails.
Currently when network diags fails, it only informs how many test pods failed but doesn't provide why those pods failed. This change will fetch logs for the pods in case of setup failure.