upgrade: upgrade the vendor of cri-o/ocicni #2282
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #2282 +/- ##
==========================================
+ Coverage 66.97% 67.05% +0.08%
==========================================
Files 211 211
Lines 17306 17315 +9
==========================================
+ Hits 11591 11611 +20
+ Misses 4312 4304 -8
+ Partials 1403 1400 -3
|
starnop
force-pushed
the
cni-upgrade
branch
from
49588f5 to
f27873b
Compare
fuweid
reviewed
Sep 28, 2018
cri/ocicni/cni_manager.go
Outdated
| return ip, nil | ||
| if len(results) > 0 { | ||
| // NOTE: only get the first IP of the first network at present | ||
| result := results[0].(*cnicurrent.Result) |
There was a problem hiding this comment.
could we use (val, ok) to do assertion? because it's safe 😅
starnop
force-pushed
the
cni-upgrade
branch
2 times, most recently
from
f57f394 to
9af1907
Compare
starnop
changed the title
|
Any update on this? Still WIP? @starnop |
starnop
force-pushed
the
cni-upgrade
branch
3 times, most recently
from
d361f82 to
191dfca
Compare
starnop
changed the title
|
@allencloud has been updated. :) |
|
@fuweid @YaoZengzeng PTAL. ^_^ |
fuweid
reviewed
Oct 11, 2018
cri/ocicni/cni_manager.go
Outdated
| if len(results) > 0 { | ||
| // NOTE: only get the first IP of the first network at present | ||
| // TODO: support multiple network | ||
| if result, ok := results[0].(*cnicurrent.Result); ok { |
There was a problem hiding this comment.
so, is the first one the *cnicurrent.Result?
There was a problem hiding this comment.
Yeah. as comments mentioned, we only get the first IP of the first network at present. :)
fuweid
reviewed
Oct 11, 2018
| @@ -32,7 +33,7 @@ func NewCniManager(cfg *config.Config) (CniMgr, error) { | |||
| } | |||
| } | |||
|
|
|||
| plugin, err := ocicni.InitCNI(networkPluginConfDir, networkPluginBinDir) | |||
| plugin, err := ocicni.InitCNI("", networkPluginConfDir, networkPluginBinDir) | |||
There was a problem hiding this comment.
@starnop could you comment here for the reason why we set it empty?
allencloud
reviewed
Oct 12, 2018
cri/ocicni/cni_manager.go
Outdated
| if err != nil { | ||
| return "", fmt.Errorf("failed to get pod network status: %v", err) | ||
| } | ||
|
|
||
| return ip, nil | ||
| if len(results) > 0 { |
There was a problem hiding this comment.
I suggest that we could use return fast to ident less.
if len(results)==0{
return "", fmt.Errorf("failed to get pod network status for nil result")
}
if result, ok := results[0].(*cnicurrent.Result); !ok {
return "", fmt.Errorf("failed to get pod network status for wrong result: %+v", results[0])
}
if len(result.IPs) == 0 {
return "", fmt.Errorf("failed to get pod network status for nil IP")
}
ip := result.IPs[0].Address.IP.String()
return ip, nil
|
I think we could merge this until we finish some minor change. @starnop |
Signed-off-by: Starnop <starnop@163.com>
|
All above mentioned has been updated. :) |
|
LGTM by @fuweid |
pouchrobot
added
the
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Starnop starnop@163.com
Ⅰ. Describe what this PR did
At present,
cri-o/ocicniwill watch CNI configuration file through inotify and quit if inofity runs out without any error report, it's not make sense. Fortunately, the issue has been settled at cri-o/ocicni@0831829 . So we update the vendor ofcri-o/ocicniⅡ. Does this pull request fix one issue?
None.
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
Ⅳ. Describe how to verify it
use command
sudo sysctl -w fs.inotify.max_user_instances=10to limit the inotify, and you will get error like this:Ⅴ. Special notes for reviews
BTY, update the vendor of
github.com/containernetworking/cni.