lxd: sync CPU features across LXD cluster for VMs with migration.stateful=true (from Incus) #15351
Conversation
github-actions
bot
added
Documentation
mihalicyn
force-pushed
the
cpu-flags-incus-cherry-picks
branch
from
2263743 to
fa72e8e
Compare
Signed-off-by: Yueyuanmei Zhang <[email protected]> (cherry picked from commit 6799c405a32e132bf43cf475cfddb8ef0a352b81) Signed-off-by: Kadin Sayani <[email protected]> Signed-off-by: Gabriel Mougard <[email protected]> License: Apache-2.0
Signed-off-by: Yueyuanmei Zhang <[email protected]> (cherry picked from commit f240048ec931554ca6440c03a9d58436fc5b57ed) Signed-off-by: Kadin Sayani <[email protected]> Signed-off-by: Gabriel Mougard <[email protected]> License: Apache-2.0
Signed-off-by: Gabriel Mougard <[email protected]>
mihalicyn
force-pushed
the
cpu-flags-incus-cherry-picks
branch
from
fa72e8e to
e5d9246
Compare
Signed-off-by: Yueyuanmei Zhang <[email protected]> (cherry picked from commit 7399e88f837cbaa0f8e4d31ad740512765efe30e) Signed-off-by: Kadin Sayani <[email protected]> Signed-off-by: Gabriel Mougard <[email protected]> License: Apache-2.0
Signed-off-by: Stéphane Graber <[email protected]> Sponsored-by: Luizalabs (https://luizalabs.com) (cherry picked from commit d0b9b9931bbd0d0991d6068c4ef3e2d18657ff1f) Signed-off-by: Kadin Sayani <[email protected]> Signed-off-by: Gabriel Mougard <[email protected]> License: Apache-2.0
Signed-off-by: Stéphane Graber <[email protected]> Sponsored-by: Luizalabs (https://luizalabs.com) (cherry picked from commit edd7a4cbf9437e1aeb9e444f1beff4540c88ba63) Signed-off-by: Kadin Sayani <[email protected]> Signed-off-by: Gabriel Mougard <[email protected]> License: Apache-2.0
Signed-off-by: Stéphane Graber <[email protected]> Sponsored-by: Luizalabs (https://luizalabs.com) (cherry picked from commit b2be9b9d88fb2b6cf67fb24211740d1fd9bbc7cc) Signed-off-by: Kadin Sayani <[email protected]> Signed-off-by: Gabriel Mougard <[email protected]> License: Apache-2.0
Signed-off-by: Stéphane Graber <[email protected]> Sponsored-by: Luizalabs (https://luizalabs.com) (cherry picked from commit 4d43430e13e6f5b0c0c665bde317f8243709c4d3) Signed-off-by: Kadin Sayani <[email protected]> Signed-off-by: Gabriel Mougard <[email protected]> License: Apache-2.0
The "kvm64" CPU definition doesn't exist on aarch64 and possibly on other platforms, so restrict the logic to x86_64 for the time being. Signed-off-by: Stéphane Graber <[email protected]> (cherry picked from commit ee24c49a275268f08f9dd060fba1256699fee65e) Signed-off-by: Kadin Sayani <[email protected]> Signed-off-by: Gabriel Mougard <[email protected]> License: Apache-2.0
mihalicyn
force-pushed
the
cpu-flags-incus-cherry-picks
branch
from
e5d9246 to
2edcb86
Compare
tomponline
changed the title
| @@ -2435,6 +2435,55 @@ func (d *Daemon) heartbeatHandler(w http.ResponseWriter, r *http.Request, isLead | |||
|
|
|||
| logger.Info("Partial heartbeat received", logger.Ctx{"local": localClusterAddress}) | |||
| } | |||
|
|
|||
| // Refresh cluster member resource info cache. | |||
There was a problem hiding this comment.
This function heartbeatHandler is already quite long and intricate so am not too keen on making it more so.
Please can we split this logic out into a separate function we can call from here instead.
This is something the TIOBE TICS scoring system is also looking for (less complex functions).
| } | ||
|
|
||
| go func(name string, address string) { | ||
| muRefresh.Lock() |
There was a problem hiding this comment.
I'm quite confused by this lock.
We seem to be starting a go routine per online cluster member (so presumably there is some intention for concurrency), and then we serialise the whole function it via a mutex.
Is the intention to launch a cluster CPU flags refresh task and not hold up the rest of the heartbeatHandler function?
If so, we could move this logic into its own function (as suggested above) and then we can run it via go newfunction() and then have the actual refresh loop done sequentially.
| } | ||
|
|
||
| // Connect to the server. | ||
| client, err := cluster.Connect(address, s.Endpoints.NetworkCert(), s.ServerCert(), nil, true) |
There was a problem hiding this comment.
Given that the leader initiates heartbeat requests to every cluster member concurrently, does this means that on the hour (after the cache has expired) this section triggers every cluster member to connect to every cluster member to retrieve their CPU flags at the same time?
This sounds like something that might cause regularly periodic load spikes.
There was a problem hiding this comment.
Also this hits client.GetServerResources() which as well as the CPU info, pulls a lot more hardware resource info.
lxd/instance/drivers/driver_qemu.go
Outdated
| @@ -8929,6 +9035,24 @@ func (d *qemu) checkFeatures(hostArch int, qemuPath string) (map[string]any, err | |||
| features["vhost_net"] = struct{}{} | |||
| } | |||
|
|
|||
| // Get the host CPU model. | |||
| model, err := monitor.QueryCPUModel("kvm64") | |||
There was a problem hiding this comment.
@mihalicyn I'd like to understand the decision for kvm64 here (and document it) as looking into the query-cpu-model-expansion command it seems it can also take variants like:
Gathered by qemu-system-x86_64 -cpu help
x86 host processor with all supported host features
x86 max Enables all features supported by the accelerator in the current host
and with kvm64 it has a variant too:
x86 kvm64 (alias configured by machine type)
x86 kvm64-v1 Common KVM processor
|
|
||
| // Get the host flags. | ||
| info := DriverStatuses()[instancetype.VM].Info | ||
| hostFlags, ok := info.Features["flags"].(map[string]bool) |
There was a problem hiding this comment.
@mihalicyn do you foresee any potential mismatch errors by comparing the CPU flags as returned from /proc/cpuinfo (from the resources yaml files here) with the CPU flags returned from QEMU's QMP query-cpu-model-expansion command?
I'm wondering why query-cpu-model-expansion wasnt used for both?
| return err | ||
| } | ||
|
|
||
| cpuType = "kvm64" |
There was a problem hiding this comment.
this is hardcoded in at least 2 places so far, so bearing in mind 7f22559#r2034758971 we should at least have a constant for it.
| @@ -410,7 +410,7 @@ func (d *qemu) getClusterCPUFlags() ([]string, error) { | |||
|
|
|||
| for _, node := range nodes { | |||
| // Attempt to load the cached resources. | |||
| resourcesPath := internalUtil.CachePath("resources", fmt.Sprintf("%s.yaml", node.Name)) | |||
| resourcesPath := shared.CachePath("resources", fmt.Sprintf("%s.yaml", node.Name)) | |||
There was a problem hiding this comment.
this change isnt related to the commit message, probably needs to be included in an earlier commit.
| @@ -1574,8 +1574,8 @@ func (d *qemu) start(stateful bool, op *operationlock.InstanceOperation) error { | |||
|
|
|||
| cpuType := "host" | |||
|
|
|||
| // Get CPU flags if clustered and migration is enabled. | |||
| if d.state.ServerClustered && util.IsTrue(d.expandedConfig["migration.stateful"]) { | |||
| // Get CPU flags if clustered and migration is enabled (x86_64 only for now). | |||
There was a problem hiding this comment.
maybe using max or host would give us support for arm64 too?
There was a problem hiding this comment.
Thanks for this @gabrielmougard and @mihalicyn !
I've done a first pass and have some concerns around how much, how often and how concurrently the cluster member CPU data is gathered.
Also I would like us to explore the actual implementation, especially the considerations around the use of:
- Storing full hardware resources, including the CPU flags from /proc/cpuinfo for every cluster member - could we instead just store the QEMU CPU flags in the DB?
- Whether they can be reliably compared to the ones from
query-cpu-model-expansion. - What drove the use of
kvm64and is that an appropriate choice (rather thanhostormax)? - We should consider testing of this. Can we use outer LXD VMs with manual
raw.qemu: cpusettings to setup a cluster with differing CPUs?
Also whilst we are working in this area we should also consider:
|
@mihalicyn ill close this for now whilst we continue the investigation. Thanks |
Resubmission of #15114 after testing
JIRA ticket: https://warthogs.atlassian.net/browse/LXD-2078