Skip to content
This repository was archived by the owner on Apr 3, 2018. It is now read-only.

cni: Fix bug while updating endpoints after scanning net namespace - #488

Merged
sboeuf merged 2 commits into
containers:masterfrom
amshinde:fix-cni-bug
Nov 20, 2017
Merged

cni: Fix bug while updating endpoints after scanning net namespace#488
sboeuf merged 2 commits into
containers:masterfrom
amshinde:fix-cni-bug

Conversation

@amshinde

Copy link
Copy Markdown
Collaborator

After scanning the network namespace, we need to update the scanned
endpoints with the DNS information, and use the scanned endpoints
thereafter discarding the endpoints structures we had created prior
to calling into the CNI plugin.

There was a bug in the update, which was causing the entire endpoint
structure being assigned back to the scanned endpoints causing us to
lose information from the scan including if a virtual or physical
endpoint was found in the scan.

Signed-off-by: Archana Shinde archana.m.shinde@intel.com

After scanning the network namespace, we need to update the scanned
endpoints with the DNS information, and use the scanned endpoints
thereafter discarding the endpoints structures we had created prior
to calling into the CNI plugin.
There was a bug in the update, which was causing the entire endpoint
structure being assigned back to the scanned endpoints causing us to
lose information from the scan including if a virtual or physical
endpoint was found in the scan.
Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@sameo

sameo commented Nov 19, 2017

Copy link
Copy Markdown
Collaborator

LGTM
But CNI unit tests are failing.

Approved with PullApproveApproved with PullApprove

In addition to the DNS, we need to copy the netpair struct as well
from the endpoint structures we created prior to the network creation
to the endpoints created after scanning the network.
This is needed for creating the tap and bridge interfaces for virtual
endpoints.
Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde

Copy link
Copy Markdown
CollaboratorAuthor

@sameo Units tests are passing now.
@sboeuf Does this look good to you?

@mcastelinomcastelino left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@sboeuf

Copy link
Copy Markdown
Collaborator

LGTM
Waiting for @amshinde to run a local test. I will merge right after that

@amshinde

Copy link
Copy Markdown
CollaboratorAuthor

@sboeuf I have run a manual test with cni-bridge plugin as well,able to see the network setup correctly and the pod is deleted successfully.

@sboeuf

Copy link
Copy Markdown
Collaborator

@amshinde thanks for that !

@sboeuf
sboeuf merged commit 4589f5b into containers:masterNov 20, 2017
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@amshinde@sameo@sboeuf@mcastelino