bugfix: put sandbox into store after created to avoid uncontrolled sandbox container #2326
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2326 +/- ##
=========================================
+ Coverage 66.19% 67.6% +1.41%
=========================================
Files 218 218
Lines 17674 17678 +4
=========================================
+ Hits 11699 11951 +252
+ Misses 4609 4339 -270
- Partials 1366 1388 +22
|
starnop
force-pushed
the
pre-store
branch
2 times, most recently
from
d2d1086 to
fada42a
Compare
cri/v1alpha2/cri.go
Outdated
| // If running sandbox failed, clean up the sandbox directory. | ||
| if retErr != nil { | ||
| os.RemoveAll(sandboxRootDir) | ||
| removeErr := os.RemoveAll(sandboxRootDir) |
There was a problem hiding this comment.
use if err := xx; err != nil pattern here?
cri/v1alpha2/cri.go
Outdated
| } | ||
| // should not remove the sandbox container metadata from sandboxStore | ||
| // until it was removed by pouchd. | ||
| removeErr := c.SandboxStore.Remove(id) |
There was a problem hiding this comment.
use if err := xx; err != nil pattern here?
There was a problem hiding this comment.
Yes, it is better to use if err := xx; err != nil, and the err would only take effect in the code block. After that, there is no need to add a new naming removeErr, err is enough.
cri/v1alpha2/cri.go
Outdated
| Runtime: config.Annotations[anno.KubernetesRuntime], | ||
| } | ||
|
|
||
| if _, ok := config.Annotations[anno.LxcfsEnabled]; ok { |
There was a problem hiding this comment.
could we put it before create sandbox? I think the validation should be checked at first
fuweid
left a comment
fuweid
left a comment
There was a problem hiding this comment.
we should do the validation at the first.
cri/v1alpha2/cri.go
Outdated
| } | ||
|
|
||
| // update the metadata of sandbox container after network had been set up successfully. | ||
| sandboxMeta.NetNSPath = netnsPath |
There was a problem hiding this comment.
I think this part should be following c.setupPodNetwork. If there is no need to setup Pod network, are we need to update the NetNSPath here?
…ndbox container Signed-off-by: Starnop <[email protected]>
4 participants
Signed-off-by: Starnop [email protected]
Ⅰ. Describe what this PR did
as title described.
Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
None.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
Cherry pick