VM: Wait until hotplugged vCPUs are visible

[mihalicyn]

Fixes: #13295

[tomponline]

Do we know why :

// Get the list of PIDs from the VM.
	pids, err := monitor.GetCPUs()
	if err != nil {
		op.Done(err)
		return err
	}
	err = d.setCoreSched(pids)
	if err != nil {
		err = fmt.Errorf("Failed to allocate new core scheduling domain for vCPU threads: %w", err)
		op.Done(err)
		return err
	}

just above works OK, but pids, err := monitor.GetCPUs() later is problematic?

[mihalicyn]

just above works OK, but pids, err := monitor.GetCPUs() later is problematic?

GetCPUs() call itself is not problematic in both cases. What is problematic is that return value of GetCPUs() call is not aligned with cpu.limit value. setCoreSched(pids) is not checking anything, it just takes a list of PIDs and set core scheduling cookie for them. While SetAffinity(set) call operates on set of CPUs and, for this reason, checks that given vCPU pinning set is aligned with what we have in pids list from GetCPUs().

[tomponline]

just above works OK, but pids, err := monitor.GetCPUs() later is problematic?

GetCPUs() call itself is not problematic in both cases. What is problematic is that return value of GetCPUs() call is not aligned with cpu.limit value. setCoreSched(pids) is not checking anything, it just takes a list of PIDs and set core scheduling cookie for them. While SetAffinity(set) call operates on set of CPUs and, for this reason, checks that given vCPU pinning set is aligned with what we have in pids list from GetCPUs().

Do you know why its not aligned? I tend to be wary of sleep loops for an arbitrary amount of time.

[mihalicyn]

Do you know why its not aligned? I tend to be wary of sleep loops for an arbitrary amount of time.

Because inst.SetAffinity(set) is getting called from deviceTaskBalance with the set based on the conf["limits.cpu"] data.
While internally, SetAffinity have to obtain QEMU process threads list using GetCPUs(). And it takes a reasonable time when GetCPUs() return a list with the length equal to conf["limits.cpu"].

[tomponline]

Do you know why its not aligned? I tend to be wary of sleep loops for an arbitrary amount of time.

Because inst.SetAffinity(set) is getting called from deviceTaskBalance with the set based on the conf["limits.cpu"] data. While internally, SetAffinity have to obtain QEMU process threads list using GetCPUs(). And it takes a reasonable time when GetCPUs() return a list with the length equal to conf["limits.cpu"].

I'm trying to understand in what situation this occurs. Am I right in thinking that this doesn't occur when deviceTaskBalance is called when an instance is starting, but that it might occur if deviceTaskBalance is called by something else midway through an instance starting up?

[mihalicyn]

I'm trying to understand in what situation this occurs. Am I right in thinking that this doesn't occur when deviceTaskBalance is called when an instance is starting, but that it might occur if deviceTaskBalance is called by something else midway through an instance starting up?

No, no, this issue happens when we are doing a CPU hotplug in setCPUs. Just after this we call deviceTaskBalance. But CPU hotplug is not instant. It takes time between you call monitor.AddDevice (inside setCPUs) and when GetCPUs() show you this new (updated) CPU list. The idea of this sync is to make setCPUs to be a synchronous in terms of cpu list update, that we have a gurantee that all the necessary QEMU thread were spawned by the time when setCPUs call finishes.

You can reproduce this effect if you do:

lxc config set inst limits.cpu=1
lxc config set inst limits.cpu=3
lxc config set inst limits.cpu=5
lxc config set inst limits.cpu=1

you will see that without this fix GetCPUs() returns an old CPU configuration, because threads for new processors are not started yet (or killed if you make limits.cpu less than it was).

[tomponline]

@mihalicyn as discussed in our call this morning lets:

• Ensure that the setCoreSched is using the correct list of CPU pids (i.e move it after the call to `setCPUs).
• Consider whether the CPU balancing change is actually working against the original aims of using setCoreSched in the first place.

[mihalicyn]

• Ensure that the setCoreSched is using the correct list of CPU pids (i.e move it after the call to `setCPUs).

Fixed. At the same time fixed core scheduling setup on a limits.cpu live-update codepath. Core scheduling setup wasn't updated at all.

[mihalicyn]

• Consider whether the CPU balancing change is actually working against the original aims of using setCoreSched in the first place.

From what I see it is to some extent. I would say that we need to introduce per-instance config option like "limits.cpu.allow_autopin" (false by default) and do autopinning only for those instances which has this enabled.