Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notifier update; MemberState update #14294

Merged
merged 25 commits into from
Oct 21, 2024
Merged

Conversation

MggMuggins
Copy link
Contributor

@MggMuggins MggMuggins commented Oct 17, 2024

This includes preparatory changes as described in #14247:

  • Notifier improvements
    • Use state.ServerClustered
    • Don't query for OfflineThreshold unless needed (see commit msg)
    • Allow passing db.NodeInfo to NewNotifier
    • Pass db.NodeInfo to the notifier hook to reduce the need for extra http calls for basic info (like the target server's name)
  • Member state updates
    • Add CPUThreads to ClusterMemberSysInfo (name tbd)
    • Refactor MemberState to expose local ClusterMemberSysinfo
    • Implement helper to collect member state from all cluster members, using the notifier improvements above
  • Other odds and ends

Performance improvements for project limits will be in a separate PR.

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Oct 17, 2024
Copy link

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

@MggMuggins MggMuggins force-pushed the notifier-update branch 2 times, most recently from 8f382c5 to da0b159 Compare October 18, 2024 05:18
lxd/cluster/notify.go Outdated Show resolved Hide resolved
lxd/cluster/notify.go Outdated Show resolved Hide resolved
lxd/cluster/notify.go Outdated Show resolved Hide resolved
lxd/cluster/notify.go Outdated Show resolved Hide resolved
lxd/cluster/notify.go Outdated Show resolved Hide resolved
@tomponline
Copy link
Member

cluster tests are sad

Unfortunately the notifier is called during database startup before the
global config has been loaded, so we need to fall back on loading the
offline threshold from the database.

Signed-off-by: Wesley Hershberger <[email protected]>
Avoid a query when a caller already has NodeInfo.

This also makes possible using NotifyAll for a subset of cluster members.

Signed-off-by: Wesley Hershberger <[email protected]>
This allows a hook to get some basic info about the cluster member that
it's reaching out to without needing to make an explicit `GetServer` call.

Signed-off-by: Wesley Hershberger <[email protected]>
This correctly initializes the config so that we can grab
cluster.offline_threshold from it during the notify tests.

Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
No longer requires making an http request to the target member to get its
name.

Signed-off-by: Wesley Hershberger <[email protected]>
No longer require making an http request to the target member to get its
name.

I also removed the GetServer during storage pool deletion; it's unclear
to me what the purpose of this extra request was; it was introduced in
fe0d87f but that commit doesn't provide
any context. I'll restore the request if I run into storage-related CI
failures.

Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
memberName implies that this function will work for any cluster member,
not just the local one

Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
The instance options docs (doc/reference/instance_options.md) play a little
fast and loose with terminology: for integer values of limits.cpu it only
refers to "CPU(s)" without specifying vCPU, thread or core. LogicalCPUs
from the golang std NumCPU is probably the best fit here.

Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
This is currently unused but will be utilized for resource reservations

Signed-off-by: Wesley Hershberger <[email protected]>
And set the test state's server name when clustered

Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
This will allow avoiding a GET /1.0/cluster/members/{name}/state when
validating resource reservations, and may be useful for validating
future config keys.

Signed-off-by: Wesley Hershberger <[email protected]>
config.Dump() creates and passes back a new map, so there's no need to
do it a second time.

Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins MggMuggins marked this pull request as ready for review October 18, 2024 22:02
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

LGTM

Please can you add an API extension for the new cluster member state LogicalCPUs field as a follow up PR. Thanks

@tomponline tomponline merged commit d638232 into canonical:main Oct 21, 2024
29 checks passed
MggMuggins added a commit to MggMuggins/lxd that referenced this pull request Oct 21, 2024
Covers changes introduced with canonical#14294

Signed-off-by: Wesley Hershberger <[email protected]>
tomponline added a commit that referenced this pull request Oct 21, 2024
hamistao pushed a commit to hamistao/lxd that referenced this pull request Oct 25, 2024
Covers changes introduced with canonical#14294

Signed-off-by: Wesley Hershberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants