-
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 1390173 - Test more pod to pod connectivity test combinations #11717
Conversation
@openshift/networking PTAL |
[test] |
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
@@ -74,7 +76,7 @@ func (d *NetworkDiagnostic) Check() types.DiagnosticResult { | |||
return d.res | |||
} | |||
if !ok { | |||
d.res.Warn("DNet2002", nil, fmt.Sprintf("Skipping network diagnostics check. Reason: Not using openshift network plugin.")) | |||
d.res.Warn("DNet2002", nil, "Skipping network diagnostics check. Reason: Not using openshift network plugin.") |
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.
you should split out cleanups like this into separate commits
@@ -148,6 +148,13 @@ func (d CheckPodNetwork) checkConnection(pods1, pods2 []kapi.Pod, warnMsg string | |||
} | |||
} | |||
} | |||
|
|||
if !sameNamespace && !strings.Contains(warnMsg, "global to non-global projects") { |
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.
Why don't you warn in that case? Is there some better way of testing for that than doing a substring check on the warning message?
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 want to warn in this case because same namespace can't have global and non-global pods at the same time. May be we can make the caller pass a flag to checkConnection() to check this condition or not.
Created 2 isolated and 2 global namespaces on every node, so that pod to pod connectivity tests can be performed among same/different namespaces, same/different nodes and global/non-global pods.
4bf8912
to
fd725e2
Compare
Separated bug fix and cleanup. |
LGTM |
flake is #10773 |
Evaluated for origin test up to fd725e2 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11060/) (Base Commit: 3b6c372) |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11060/) (Image: devenv-rhel7_5307) |
Evaluated for origin merge up to fd725e2 |
Created 2 isolated and 2 global namespaces on every node, so that pod to pod connectivity
tests can be performed among same/different namespaces, same/different nodes and global/non-global pods.