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

network: guest network interface support NOARP#493

Merged
teawater merged 1 commit into
kata-containers:masterfrom
zhabinecho:noarp-feature
Mar 25, 2019
Merged

network: guest network interface support NOARP#493
teawater merged 1 commit into
kata-containers:masterfrom
zhabinecho:noarp-feature

Conversation

@zhabinecho
Copy link
Copy Markdown

@zhabinecho zhabinecho commented Mar 20, 2019

According to the host link rawflags, setting link NOARP when needed

Fixes: #492

Signed-off-by: Zha Bin zhabin@linux.alibaba.com

jodh-intel
jodh-intel approved these changes Mar 20, 2019
View reviewed changes
Copy link
Copy Markdown

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @zhabinecho!

lgtm but let's get input from @amshinde and @mcastelino.

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Mar 20, 2019

/test

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Mar 20, 2019

@jodh-intel - like the parallel PR over in the runtime, this has failed the 'Fixes' check, even though I can see one - I wonder if maybe it could be a character encoding issue?

Found 1 commit between commit HEAD and branch master
ERROR: No "Fixes" found
ERROR: checkcommits failed. See the document below for help on formatting
commits for the project.

Any ideas?

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Mar 20, 2019

Ah - I have see it now! The "raw" commit shows:

Fixes: kata-containers#492

... but github unhelpfully in this case renders it as:

Fixes: #492

The problem is that our checkcommits tool doesn't understand that syntax.

@zhabinecho - please could you change the "Fixes" line of you commit to be exactly as follows:

Fixes: #492

@bergwolf bergwolf mentioned this pull request Mar 20, 2019
Merged
@zhabinecho
Copy link
Copy Markdown
Author

zhabinecho commented Mar 20, 2019

Ah - I have see it now! The "raw" commit shows:

Fixes: kata-containers#492

... but github unhelpfully in this case renders it as:

Fixes: #492

The problem is that our checkcommits tool doesn't understand that syntax.

@zhabinecho - please could you change the "Fixes" line of you commit to be exactly as follows:

Fixes: #492

OK, I will fixed it. Thanks

sboeuf
sboeuf suggested changes Mar 20, 2019
View reviewed changes
Copy link
Copy Markdown

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

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

The PR looks good, but please fix the protobuf first.

Comment thread pkg/types/types.proto Outdated
mcastelino
mcastelino approved these changes Mar 20, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcastelino mcastelino left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

This is a good catch. We have never really tested pure L3 interfaces properly.

sboeuf
sboeuf reviewed Mar 21, 2019
View reviewed changes
Comment thread pkg/types/types.proto Outdated
// library, regarding each type of link. Here is a non exhaustive
// list: "veth", "macvtap", "vlan", "macvlan", "tap", ...
string type = 7;
uint32 RawFlags = 8;
Copy link
Copy Markdown

@sboeuf sboeuf Mar 21, 2019

Choose a reason for hiding this comment

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

Sorry to be picky but we should follow the protobuf syntax defined here. Therefore, RawFlags should be raw_flags here.
But I noticed we forgot to enforce this for other fields of this structure, which is confusing...

Copy link
Copy Markdown
Author

@zhabinecho zhabinecho Mar 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Firstly I use raw_flags. Next, I will submit another PR to follow the protobuf for other fileds.

Copy link
Copy Markdown
Author

@zhabinecho zhabinecho Mar 22, 2019

Choose a reason for hiding this comment

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

I don't understand why the continuous-integration failed ?

Copy link
Copy Markdown
Member

@gnawux gnawux Mar 22, 2019

Choose a reason for hiding this comment

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

The travis tests on ppc64le looks not stable today

Copy link
Copy Markdown
Member

@teawater teawater Mar 22, 2019

Choose a reason for hiding this comment

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

INFO: Forcing gometalinter version 
Installing:
  deadcode
  dupl
  errcheck
  gochecknoglobals
  gochecknoinits
  goconst
  gocyclo
  goimports
  golint
  gosec
  gotype
  gotypex
  ineffassign
  interfacer
  lll
  maligned
  misspell
  nakedret
  safesql
  staticcheck
  structcheck
  unconvert
  unparam
  varcheck
INFO: gometalinter args: '--exclude=vendor/.*" --tests --exclude=".*\.pb\.go" --disable-all --enable=gofmt --concurrency=2 --enable=misspell --enable=vet --enable=ineffassign --enable=gocyclo --cyclo-over=15 --enable=golint --deadline=600s --enable=structcheck --enable=unused --enable=staticcheck --enable=maligned --enable=varcheck --enable=unconvert'

gometalinter: error: unknown linters: unused
The command ".ci/static-checks.sh" failed and exited with 1 during .

I just met the issue in my host. Maybe gometalinter removed unused. I will take a look on it.

Copy link
Copy Markdown
Member

@teawater teawater Mar 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can just wait for kata-containers/tests#1315 to fix this issue.

Copy link
Copy Markdown
Author

@zhabinecho zhabinecho Mar 25, 2019

Choose a reason for hiding this comment

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

Can we merge it ? :-)

@zhabinecho
network: guest network interface support NOARP
3aa90d2 
According to the host link rawflags, setting link NOARP when needed

Fixes: kata-containers#492

Signed-off-by: Zha Bin <zhabin@linux.alibaba.com>
This was referenced Mar 22, 2019
Closed
Closed
@teawater
Copy link
Copy Markdown
Member

teawater commented Mar 25, 2019

/test

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Mar 25, 2019

@sboeuf - please can you re-review?

@sboeuf
Copy link
Copy Markdown

sboeuf commented Mar 25, 2019

LGTM

@teawater teawater merged commit 48dd1c0 into kata-containers:master Mar 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

6 more reviewers

@gnawux gnawux gnawux left review comments

@teawater teawater teawater left review comments

@bergwolf bergwolf bergwolf approved these changes

@mcastelino mcastelino mcastelino approved these changes

@jodh-intel jodh-intel jodh-intel approved these changes

@sboeuf sboeuf sboeuf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@zhabinecho @jodh-intel @grahamwhaley @teawater @sboeuf @gnawux @bergwolf @mcastelino