Per cluster member resource reservations

[MggMuggins]

As described in LX089.

The general approach (as with the other limits validation) is to query the current state of the cluster from the database, apply the delta (create an instance, update the cluster config or update a cluster member config), and then validate the resulting configuration.

This is performed in at most only two round-trips to the DB:

• GetMemberConfigWithGlobalDefault gives a column for global and member-specific config for each cluster member
• GetClusterMemberInstanceConfig gives the value of each limit for each instance, including null values when the config key is not set

I don't love the specialized types for each query, but without them the results are not super manageable. I also feel like the permissions checking code could be cleaner.

I'd like to hear thoughts on organization:

• These limits aren't enforced at the project level the same way others are, but I've included them in the lxd/project/limits package because other code there is similar.
• The db query functions are relevant to cluster members but could fit in any of a few files

TODO:

• Refactor GetClusterInstanceConfig (this was the bug I noted in our standup on Tuesday Oct 8)
• Only request GET /cluster/members/{name}/state when cluster update includes reservation keys
• Reservations are not yet taken into account during instance placement
• More tests
• Pass ClusterMemberState to scriptlet so that the scriptlet doesn't need to re-request from the rest of the cluster
• Docs? Not sure if we should call this feature out in a place other than the config keys themselves.

[github-actions]

Heads up @mionaalex - the "Documentation" label was applied to this issue.

[MggMuggins]

I don't love this name; ClusterConfigValue? MemberConfigValue?

[MggMuggins]

@tomponline This should be ready for a first round. I'm not clear on the cause of the test failure but initially it looks unrelated to my changes... I'll investigate further when I'm back to work on Wednesday.

[tomponline]

shouldnt this be an optional integer?

[tomponline]

does this function or any of the functions it calls use the DB? If so, I'd prefer to keep the tx arg and plumb it down so that it uses a single tx.

[tomponline]

Is this function just used for access the effective config, or are you using it to display the config to the end user?

If the former, have you considered returning a map of effective config values for each cluster member, rather than requiring the caller to consult the ClusterValue and the MemberValue.

Also we dont use sqli.NullString for config values, unset keys are just not present in the map.

[tomponline]

This doesnt look workable to me because your not expanding the effective instance config from profile(s).

I dont think you can do this in a single query, you might need to use InstanceList() instead with a custom handler func.

[MggMuggins]

Focusing on this as it's the largest functional deficiency; the current commits are a bit of a mess but my feeling is that InstanceList won't quite work here. In order to add an instance (with expanded config) we need a list of all profiles that will be applied to that instance, which we can't get from InstanceList. Still thinking on this...

[tomponline]

You can use InstanceList for this, we do it commonly. It doesn't expand it automatically but does provide profile info so you can expand if needed. See existing examples

[tomponline]

e.g.

var globalConfigDump map[string]any
if s.GlobalConfig != nil {
    globalConfigDump = s.GlobalConfig.Dump()
}

err = s.DB.Cluster.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error {
    return tx.InstanceList(ctx, func(dbInst db.InstanceArgs, p api.Project) error {
        dbInst.Config = instancetype.ExpandInstanceConfig(globalConfigDump, dbInst.Config, dbInst.Profiles)
        return nil
    }, filter)
})
if err != nil {
    return nil, err
}

[MggMuggins]

Getting the existing instances & expanding their config isn't the problem; when we're validating a new/changed instance, InstanceList won't call our hook (for new instances), and isn't guaranteed to return the correct set of profiles (for instance update). Right now I'm thinking to just do another query to collect the correct set of profiles, even though InstanceList has likely already done so 🫤

[tomponline]

im confused, can you book a quick meeting to discuss as i dont see what the issue is.

[tomponline]

Rather than duplicate the NewNotifer logic, the changes you've needed here sound quite useful to add to NewNotifier and the returned Notifier function definition anyway.

So I'm thinking we should update NewNotifier to:

type Notifier func(member db.NodeInfo, hook func(lxd.InstanceServer) error) error

func NewNotifier(state *state.State, networkCert *shared.CertInfo, serverCert *shared.CertInfo, policy NotifierPolicy, members ...[]db.NodeInfo) (Notifier, error) {

This would make members optional, and if not supplied then all members would be loaded as they are today.

Also there looks to be some optimisations we can make in NewNotifier that you've used here:

1. Avoid the call to tx.GetNodeOfflineThreshold(ctx) and s.GlobalConfig.OfflineThreshold() instead.
2. Change if localClusterAddress == "" { to if s.ServerClustered {.

That way we get to use the more resilient down detection in NewNotifier here too.

[tomponline]

We probably still need the ClusterSysInfo function for convenience, but it can use the updated NewNotifier under the hood.

[MggMuggins]

I had considered it but decided against it while drafting this since updating all the usages was going to take a while. Agree though, I'll put together a PR.

[tomponline]

When you're appending to a slice there's no need to initialise it first, so can just do var clusterMemberNames []string

[tomponline]

I like that you've identified and made some cleanups that arent directly related to this feature.

But I think it would be clearer to include them in a separate earlier PR so we can lay the ground work for the feature change itself separately.

So lets include in a separate PR:

• Any cleanup not related to the feature itself.
• The NewNotifier changes.
• The SysInfo change to add logical CPUs.
• The change the cluster member config update to redirect to the member in question.

[MggMuggins]

Going to close this as the code that isn't in #14294 will get largely rewritten