Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix network discovery issue in ovn non-ic deployments #2674

Merged
merged 3 commits into from
Sep 11, 2023
Merged

Fix network discovery issue in ovn non-ic deployments #2674

merged 3 commits into from
Sep 11, 2023

Conversation

aswinsuryan
Copy link
Contributor

Fix network discovery issue in ovn non-ic deployments

Fixes: #2673

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr2674/aswinsuryan/fix-nonic
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@aswinsuryan aswinsuryan force-pushed the fix-nonic branch 2 times, most recently from 65dba9d to cb0a47e Compare September 7, 2023 14:34
sridhargaddam
sridhargaddam previously requested changes Sep 8, 2023
View reviewed changes
@aswinsuryan aswinsuryan force-pushed the fix-nonic branch 2 times, most recently from 6e9252f to 1640291 Compare September 8, 2023 12:52
@aswinsuryan
Fix network discovery issue in ovn non-ic deployments
2ff6786 
Signed-off-by: Aswin Suryanarayanan <asuryana@redhat.com>
@aswinsuryan
Refactor code
7d24625 
Signed-off-by: Aswin Suryanarayanan <asuryana@redhat.com>
@aswinsuryan aswinsuryan added the ready-to-test When a PR is ready for full E2E testing label Sep 9, 2023
yboaron
yboaron reviewed Sep 10, 2023
View reviewed changes
pkg/routeagent_driver/handlers/ovn/connection.go

_, err = k8sClientSet.CoreV1().Services(ovnPod.Namespace).Get(ctx, ovnKubeService, metav1.GetOptions{})
if err == nil {
return defaultOpenshiftOVNNBDB, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to code it seems that there are only two options

  • non-ic ==> when both pod and service exist
  • rest of the cases
    Is it correct?

if the answer is yes , maybe it would be better to have a separate boolean function for detecting non-ic deployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-ic deployment identification is not same for kind and Openshift. So cannot have common for both.

sridhargaddam
sridhargaddam reviewed Sep 11, 2023
View reviewed changes
Copy link
Member

@sridhargaddam sridhargaddam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for refactoring the code @aswinsuryan

If you happen to re-spin, please address the review comments.

pkg/routeagent_driver/handlers/ovn/connection.go
}

ovnPod, err := FindPod(ctx, k8sClientSet, "app=ovnkube-node")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you are using ovnPod just for the namespace. Can we use the namespace of ovnDBPod? or do you think they can be deployed in different namespaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if for the case where we are not able to find a db pod.

@sridhargaddam sridhargaddam dismissed their stale review September 11, 2023 05:58

The code is refactored, but I have few comments and would like to hear from Aswin.

@sridhargaddam
Copy link
Member

Verified the PR manually in an OCP 4.13.10 cluster and confirm that CrashloopBackoff Error is no longer seen.

sridhargaddam
sridhargaddam approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@sridhargaddam sridhargaddam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discused on slack, please address the review comments in a follow up PR.

https://kubernetes.slack.com/archives/C010RJV694M/p1694419770879479?thread_ts=1694415527.515619&cid=C010RJV694M

@sridhargaddam sridhargaddam merged commit 69701d3 into submariner-io:devel Sep 11, 2023
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr2674/aswinsuryan/fix-nonic]

@aswinsuryan aswinsuryan mentioned this pull request Sep 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yboaron yboaron yboaron approved these changes

@sridhargaddam sridhargaddam sridhargaddam approved these changes

@Oats87 Oats87 Awaiting requested review from Oats87 Oats87 is a code owner

@skitt skitt Awaiting requested review from skitt skitt is a code owner

@tpantelis tpantelis Awaiting requested review from tpantelis tpantelis is a code owner

Assignees
No one assigned
Labels
ready-to-test When a PR is ready for full E2E testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fail to run submariner with OCP 4.13.9 on AWS
4 participants
@aswinsuryan @submariner-bot @sridhargaddam @yboaron