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

sandbox: change container slice to a map #503

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

bergwolf
Copy link
Member

ContainerID is supposed to be unique within a sandbox. It is better to use
a map to describe containers of a sandbox.

Fixes: #502

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 160474 KB
Proxy: 5008 KB
Shim: 9035 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007228 KB

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 164803 KB
Proxy: 4930 KB
Shim: 9041 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007220 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 23, 2018

Build succeeded (third-party-check pipeline).

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 23, 2018

Build succeeded (third-party-check pipeline).

s.containers = append(s.containers[:idx], s.containers[idx+1:]...)
return nil
}
if s.containers[containerID] == nil {
Copy link

Choose a reason for hiding this comment

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

the way to write this in Golang is:

if _, exist := s.containers[containerID]; !exist {
        return fmt.Errorf("Could not remove the container %q from the sandbox %q containers list", containerID, s.id)
}

@@ -978,7 +980,10 @@ func (s *Sandbox) startVM() error {
}

func (s *Sandbox) addContainer(c *Container) error {
s.containers = append(s.containers, c)
if s.containers[c.id] != nil {
Copy link

Choose a reason for hiding this comment

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

Same thing here to check a container entry.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Fixed both. Thanks!

@codecov
Copy link

codecov bot commented Jul 23, 2018

Codecov Report

Merging #503 into master will decrease coverage by 0.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
- Coverage   65.98%   65.96%   -0.02%     
==========================================
  Files          93       93              
  Lines        9484     9488       +4     
==========================================
+ Hits         6258     6259       +1     
- Misses       2554     2555       +1     
- Partials      672      674       +2
Impacted Files Coverage Δ
virtcontainers/sandbox.go 64.51% <75%> (-0.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b244410...f9d5072. Read the comment docs.

Copy link

@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.

Thanks @bergwolf !

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 162655 KB
Proxy: 4965 KB
Shim: 9110 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007096 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 23, 2018

Build succeeded (third-party-check pipeline).

ContainerID is supposed to be unique within a sandbox. It is better to use
a map to describe containers of a sandbox.

Fixes: kata-containers#502

Signed-off-by: Peng Tao <bergwolf@gmail.com>
@opendev-zuul
Copy link

opendev-zuul bot commented Jul 24, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 162822 KB
Proxy: 4999 KB
Shim: 8975 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007220 KB

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.

nice patch!

@egernst egernst merged commit 2006627 into kata-containers:master Jul 24, 2018
@sboeuf sboeuf added the enhancement Improvement to an existing feature label Sep 12, 2018
@bergwolf bergwolf deleted the container branch September 13, 2018 03:26
zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019
Revert vendor: Update libcontainer vendoring
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants