Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 2 additions & 25 deletions virtcontainers/cgroups.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,31 +221,8 @@ func (s *Sandbox) constrainHypervisor(cgroup cgroups.Cgroup) error {
return fmt.Errorf("Could not add hypervisor PID %d to cgroup %v: %v", pid, path, err)
}

// when new container joins, new CPU could be hotplugged, so we
// have to query fresh vcpu info from hypervisor for every time.
tids, err := s.hypervisor.getThreadIDs()
if err != nil {
return fmt.Errorf("failed to get thread ids from hypervisor: %v", err)
}
if tids == nil || len(tids.vcpus) == 0 {
// If there's no tid returned from the hypervisor, this is not
// a bug. It simply means there is nothing to constrain, hence
// let's return without any error from here.
return nil
}

// We are about to move just the vcpus (threads) into cgroups with constraints.
// Move whole hypervisor process whould be easier but the IO/network performance
// whould be impacted.
for _, i := range tids.vcpus {
// In contrast, AddTask will write thread id to `tasks`
// After this, vcpu threads are in "vcpu" sub-cgroup, other threads in
// qemu will be left in parent cgroup untouched.
if err := cgroup.AddTask(cgroups.Process{
Pid: i,
}); err != nil {
return err
}
if err := cgroup.Add(cgroups.Process{Pid: pid}); err != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The pid is also added above to a no constraints cgoup. There is no need to do it anymore.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no constrains cgroup only adds memory subsystem, to actually limit qemu in the rest of subsystems we need to add to this cgroup.

But makes me wonder: if it is better to just add qemu to only to the cpuset subystem and not all the subsystems that has cgroup.? This way when a container is limited by cpu qouta and but not cpuset will not have performance limitations.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, we should not share container CPU quota with the qemu process. Please take a look at #878 (comment).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is an interesting idea. This require push make k8s a bit more aware of kata containers. Sounds like a more longer final solution. But always that the ecosystem consider this limitations we have in Kata is better. I can bring this topic to next architecture meeting.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @jcvenegas - did this happen? If so, can you summarise the discussion here?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ping @jcvenegas.

return fmt.Errorf("Could not add hypervisor PID %d to cgroup %v: %v", pid, path, err)
}

return nil
Expand Down