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.

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

implement arp responder #10

Merged
merged 3 commits into from Jun 4, 2019
Merged

Conversation

andrewsykim
Copy link
Contributor

@andrewsykim andrewsykim commented Jun 3, 2019

Signed-off-by: Andrew Sy Kim kiman@vmware.com
Co-authored-by: Zaalouk, Adel adel.zalok.89@gmail.com

andrewsykim and others added 3 commits June 3, 2019 00:19
Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
Co-authored-by: Zaalouk, Adel <adel.zalok.89@gmail.com>
@andrewsykim andrewsykim changed the title add arp responder implement arp responder Jun 3, 2019
Copy link
Contributor

@zanetworker zanetworker left a comment

Choose a reason for hiding this comment

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

As you mentioned, there are duplicate code from CNI, for now its needed, but maybe we should PR CNI with LinkAttr as a param instead of just name and MTU (but that would be a breaking change for other CNIs though)?

otherwise /lgtm

tableLocalARP = 100

// all pods have a fixed mac addr
podMacAddr = "aa:bb:cc:dd:ee:ff"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is defined twice (here and veth.go), would a common home for it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A single package/file for 1 const seemed overkill but agree we should revisit this soon

@andrewsykim
Copy link
Contributor Author

Agreed on both comments. Let's see how this pans out and revisit this code, definitely needs some clean up. Created #11 to follow-up.

@andrewsykim andrewsykim merged commit 2d1420e into k-vswitch:master Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants