virtcontainers/sandbox: calculate container's CPU from sandbox.contai… #2130
Conversation
virtcontainers/sandbox.go
Outdated
| @@ -1873,8 +1873,8 @@ func (s *Sandbox) calculateSandboxMemory() int64 { | |||
| func (s *Sandbox) calculateSandboxCPUs() uint32 { | |||
| mCPU := uint32(0) | |||
|
|
|||
| for _, c := range s.config.Containers { | |||
| if cpu := c.Resources.CPU; cpu != nil { | |||
| for _, c := range s.containers { | |||
There was a problem hiding this comment.
Here I think you can't simply using sandbox containers, since when creating a container in sandbox, the s.containers is updated later than calculating resources, thus you could lost accounting the created container's resources.
IMO the root cause is that the container's ContainerConfig pointer doesn't point to the instance located in sandbox's SandboxConfig's map of ContainerConfig, which caused the configure's not sync.
There was a problem hiding this comment.
It sounds like we need some more unit tests in this area.
There was a problem hiding this comment.
Yes, I think there are issues for both of the cpu and memory resource updating.
There was a problem hiding this comment.
@lifupan Yes, the root cause is that the container's ContainerConfig pointer doesn't point to the instance located in sandbox's SandboxConfig's map of ContainerConfig.
If we want to fix this way, we need to modify code in fetchSandbox->fetchContainers->newContainer, pass by reference, and more tests.
Or we can just update sandbox's SandboxConfig's map of ContainerConfig at container.go:update(), which need a add method such like
func (s *Sandbox) updateConfig() error
Which way do you perfer?
There was a problem hiding this comment.
@lifupan Yes, the root cause is that the container's ContainerConfig pointer doesn't point to the instance located in sandbox's SandboxConfig's map of ContainerConfig.
If we want to fix this way, we need to modify code in fetchSandbox->fetchContainers->newContainer, pass by reference, and more tests.
I'd prefer this one, thus we can keep only one instance of container's config in memory and changed it at one place would be saw in other place.
virtcontainers/container.go
Outdated
| @@ -785,7 +785,7 @@ func newContainer(sandbox *Sandbox, contConfig ContainerConfig) (*Container, err | |||
| return nil, err | |||
| } | |||
|
|
|||
| if err = c.createDevices(contConfig); err != nil { | |||
| if err = c.createDevices(*contConfig); err != nil { | |||
There was a problem hiding this comment.
For c.createDevice function, I think you can also change it's parameter from instance to pointer.
There was a problem hiding this comment.
Yes, we can change it in code. But I don't think we can change its semantics, we cannot change contConfig at createDevices procedure.
There was a problem hiding this comment.
What I mean is change it in code, since there is no need to pass an config instance here.
There was a problem hiding this comment.
OK, I'll change it.
Should I change and force-push all this two commits again, or just add a new commit?
which one is better?
There was a problem hiding this comment.
It's better to change them in a single commit and do a force push
container.config does not point to sandbox.config.Containers.ContainerConfig which caused the ContainerConfig not sync. Fixes: kata-containers#2129 Signed-off-by: Wang Liang <wangliangzz@inspur.com>
|
/test |
Codecov Report
@@ Coverage Diff @@
## master #2130 +/- ##
=========================================
Coverage ? 51.35%
=========================================
Files ? 109
Lines ? 15074
Branches ? 0
=========================================
Hits ? 7742
Misses ? 6380
Partials ? 952 |
|
@bergwolf There are two tests failed. They seem to have nothing to do with this commit. |
|
virtio-fs CI failure is the dnf issue which is known. |
…ners
sandbox.config.Containers is not up to date, so use container.config directly
Fixes: #2129
Signed-off-by: Wang Liang wangliangzz@inspur.com