Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

network: Detect and add static ARP entries #2672

Merged
merged 2 commits into from
May 29, 2020

Conversation

amshinde
Copy link
Member

@amshinde amshinde commented May 8, 2020

Some network plugins add static arp entries in the network namespace.
Scan namespace for static entries and pass these on to the
agent to be added within the guest.

Fixes #2671

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

@amshinde amshinde added area/api Application Programming Interface do-not-merge PR has problems or depends on another feature New functionality labels May 8, 2020
@amshinde
Copy link
Member Author

amshinde commented May 8, 2020

This needs to be merged after the agent PR is merged and vendored here.

if grpcStatus.Convert(err).Code() == codes.Unimplemented {
k.Logger().WithFields(logrus.Fields{
"arpneighbors-requested": fmt.Sprintf("%+v", neighs),
}).Warn("add ARP neighbors request failed due to old agent, please upgrade agent")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be clearer if we change this message slightly since it assumes knowledge of Kata internals. Maybe something like:

.Warn("add ARP neighbors request failed due to old agent (please upgrade Kata Containers image version)")

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

}
k.Logger().WithFields(logrus.Fields{
"arpneighbors-requested": fmt.Sprintf("%+v", neighs),
}).WithError(err).Error("add ARP neighbors request failed:")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Trailing colon in error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -11,6 +11,7 @@
IPAddress
Interface
Route
ARPNeighbor
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing commit to update Gopkg.toml?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have vendored in the agent changes. This is fixed now.

@amshinde amshinde force-pushed the add-arp-neighbors branch from 34bff7b to 8fb7b1b Compare May 14, 2020 23:01
changes:
42438f network: Add grpc method to add static arp neighbors

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde force-pushed the add-arp-neighbors branch from 8fb7b1b to ea7c276 Compare May 14, 2020 23:11
Some network plugins add static arp entries in the network
namespace.
Scan namespace for static entries and pass these on to the
agent to be added within the guest.
If the grpc api is not implemented by the agent due to a older running
agent, check for this and do not error out to maintain
backward compatibility.

Fixes kata-containers#2671

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde force-pushed the add-arp-neighbors branch from ea7c276 to 67d3e2c Compare May 14, 2020 23:17
@amshinde
Copy link
Member Author

/test

@amshinde amshinde removed the do-not-merge PR has problems or depends on another label May 14, 2020
@amshinde
Copy link
Member Author

amshinde commented May 14, 2020

This is ready for review. I have vendored in agent changes.
@jodh-intel @devimc PTAL

@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #2672 into master will increase coverage by 4.58%.
The diff coverage is 32.35%.

@@            Coverage Diff             @@
##           master    #2672      +/-   ##
==========================================
+ Coverage   45.83%   50.41%   +4.58%     
==========================================
  Files         118      118              
  Lines       17526    17205     -321     
==========================================
+ Hits         8033     8674     +641     
+ Misses       8605     7467    -1138     
- Partials      888     1064     +176     

@grahamwhaley
Copy link
Contributor

ARM CI is failing - I suspect something on the build node rather than this PR. /cc @Pennyzct

01:41:04 The reset process does not clean your kubeconfig files and you must remove them manually.
01:41:04 Please, check the contents of the $HOME/.kube/config file.
01:41:05 Bad argument `[unsupported'
01:41:05 Error occurred at line: 52
01:41:05 Try `iptables-restore -h' or 'iptables-restore --help' for more information.
01:41:05 Failed at 19: sudo iptables-restore < "$iptables_cache"
01:41:05 Failed at 39: bash -f "${SCRIPT_PATH}/cleanup_bare_metal_env.sh"
01:41:05 Failed at 1: popd
01:41:05 Makefile:230: recipe for target 'kubernetes' failed
01:41:05 make: *** [kubernetes] Error 2
01:41:05 Failed at 86: sudo -E PATH="$PATH" bash -c "make test"
01:41:05 Build step 'Execute shell' marked build as failure

@Pennyzct
Copy link
Contributor

Hi @grahamwhaley
I've encountered this failure a few times, looks to me that it may come from stale saved iptables?? I'll log in to find out. ;)
BTW, it should be irrelevant with this issue.

@amshinde
Copy link
Member Author

cc @mcastelino

@amshinde
Copy link
Member Author

@devimc @egernst PTAL

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

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

thanks @amshinde

@amshinde amshinde merged commit 011ade5 into kata-containers:master May 29, 2020
@amshinde amshinde deleted the add-arp-neighbors branch December 8, 2020 23:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/api Application Programming Interface feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect any ARP entries and send them to the agent to be mirrored
5 participants