feat(xo-server): remember server's pool name and description #8206

b-Nollet · 2024-12-19T15:10:59Z

Description

Remember server's pool name and description, and display the pool name as server default label.

I did multiple commits to keep the original changes around, but I would advise not to review by commit.

Checklist

Commit
- Title follows commit conventions
- Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
- If bug fix, add Introduced by
Changelog
- If visible by XOA users, add changelog entry
- Update "Packages to release" in CHANGELOG.unreleased.md
PR
- If UI changes, add screenshots
- If not finished or not tested, open as Draft

Review process

This 2-passes review process aims to:

develop skills of junior reviewers

limit the workload for senior reviewers

limit the number of unnecessary changes by the author

The author creates a PR.
Review process:
1. The author assigns the junior reviewer.
2. The junior reviewer conducts their review:
  - Resolves their comments if they are addressed.
  - Adds comments if necessary or approves the PR.
3. The junior reviewer assigns the senior reviewer.
4. The senior reviewer conducts their review:
  - If there are no unresolved comments on the PR → merge.
  - Otherwise, we continue with 3.
The author responds to comments and/or makes corrections, and we go back to 2.

Notes:

The author can request a review at any time, even if the PR is still a Draft.
In theory, there should not be more than one reviewer at a time.
The author should not make any changes:
- When a reviewer is assigned.
- Between the junior and senior reviews.

pdonias · 2024-12-19T16:20:30Z

packages/xo-server/src/api/pool.mjs

+  if (nameLabel !== undefined || nameDescription !== undefined) {
+    const serverId = this.getXenServerIdByObject(pool.uuid, 'pool')
+    await this.updateXenServer(serverId, { poolNameDescription: nameDescription, poolNameLabel: nameLabel })
+  }


A pool's name may be changed by another client. Also, I think the most common use case for this feature would be to get the name of the pool as soon as the user connects to the server. Updating it when the pool's name_label changes is almost a bonus because it doesn't change that often.

So I think a better approach for this would probably be a sort of hook on the XAPI events to catch any changes of the pools' names and update their servers accordingly.

What I did does display the name of the pool when a server is added or connected to.

But you're right, I did not realize the pool's name could be changed by another client.

Yes, you need to watch objects and update the record if there was a change.

A good place for this coold be:

xen-orchestra/packages/xo-server/src/xo-mixins/xen-servers.mjs

Line 219 in 93a11cf

// handle pool UUID change

fbeauchamp · 2025-01-16T15:36:01Z

packages/xo-server/src/xo-mixins/xen-servers.mjs

+        await this.updateXenServer(id, {
+          poolNameLabel: xapi.pool.name_label,
+          poolNameDescription: xapi.pool.name_description,
+        })


is it also possible to not update it if the value do not change ?
Are we sure the xapi.pool.name_xx are correctly loaded here ?

julien-f · 2025-01-20T11:30:07Z

packages/xo-server/src/xo-mixins/xen-servers.mjs

@@ -223,6 +262,11 @@ export default class XenServers {
        serverIdsByPool[xapiObject.$id] = conId
      }

+      // save pool name and description in server properties
+      if (xapiObject.$type === 'pool') {
+        updatePoolServer(xapiObject, xapiId)


What happens if this function reject?

The function is not awaited, so if it rejects it does not prevent the rest of the code from executing.

Promise rejections should always be handled, either:

await them

log the error

explicitly ignore it .catch(noop) or better ignoreErrors.call(promise) (which does not ignore programming errors)

pdonias · 2025-01-20T13:55:10Z

CHANGELOG.unreleased.md

Don't forget to change xo-server patch to xo-server minor in "Packages to release".

pdonias · 2025-01-20T14:17:44Z

packages/xo-server/src/xo-mixins/xen-servers.mjs

+          poolNameDescription: xapiObject.name_description,
+        })
+      }
+    }.bind(this)


Why not using an arrow function?

pdonias · 2025-01-20T14:18:37Z

packages/xo-server/src/xo-mixins/xen-servers.mjs

+      const xenServer = await this.getXenServer(serverId)
+
+      const serverPropertiesUpdate = {
+        ...(xapiObject.name_label !== undefined &&


Can a pool's name_label be undefined?

I'm not sure, I'll have to check

pdonias · 2025-01-20T14:18:55Z

packages/xo-server/src/xo-mixins/xen-servers.mjs

+      const serverPropertiesUpdate = {
+        ...(xapiObject.name_label !== undefined &&
+          xapiObject.name_label !== xenServer.poolNameLabel && { poolNameLabel: xapiObject.name_label }),
+        ...(xapiObject.name_description !== undefined &&
+          xapiObject.name_description !== xenServer.poolNameDescription && {
+            poolNameDescription: xapiObject.name_description,
+          }),
+        ...(xenServer.poolNameLabel === undefined &&
+          xapiObject.name_label !== undefined && { label: xapiObject.name_label }),
+      }


I find it easier to read with explicit ifs and assignments:

const serverPropertiesUpdate = {} if (xapiObject.name_label !== xenServer.poolNameLabel) { serverPropertiesUpdate.poolNameLabel = xapiObject.name_label } // ...

What do you think?

I don't really like it as it uses a lot of lines for a simple purpose, but if you had trouble reading what I did then I guess you're right

pdonias · 2025-01-21T08:35:04Z

packages/xo-server/src/xo-mixins/xen-servers.mjs

+    if (poolNameDescription !== undefined) server.poolNameDescription = poolNameDescription || undefined
+    if (poolNameLabel !== undefined) server.poolNameLabel = poolNameLabel || undefined
+    // using pool name as default server name
+    if (server.label === undefined) server.label = server.poolNameLabel


Then if the pool's name_label changes twice, the server's label will keep the pool's old name?

Yes, but I don't think we have much other choice if we still want the server label to be directly configurable. (we could maybe use a boolean to tell if the pool label has ever been directly modified, but it would look really ugly)

b-Nollet force-pushed the remember-pool-name branch from d233b11 to b05365a Compare December 19, 2024 15:39

b-Nollet marked this pull request as ready for review December 19, 2024 15:40

b-Nollet requested a review from pdonias December 19, 2024 15:46

pdonias requested changes Dec 19, 2024

View reviewed changes

b-Nollet requested a review from fbeauchamp December 19, 2024 16:28

fbeauchamp requested changes Jan 16, 2025

View reviewed changes

b-Nollet force-pushed the remember-pool-name branch from 4650fd4 to e1b5325 Compare January 20, 2025 09:58

b-Nollet requested a review from pdonias January 20, 2025 11:03

b-Nollet added 3 commits January 20, 2025 12:12

feat(xo-server): remember server's pool name and description

bb5144b

reverting previous changes

68c84ac

suggested changes

1623ae1

b-Nollet force-pushed the remember-pool-name branch from e1b5325 to 1623ae1 Compare January 20, 2025 11:13

julien-f reviewed Jan 20, 2025

View reviewed changes

pdonias requested changes Jan 21, 2025

View reviewed changes

suggested changes

cf00010

b-Nollet requested a review from pdonias January 24, 2025 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(xo-server): remember server's pool name and description #8206

feat(xo-server): remember server's pool name and description #8206

b-Nollet commented Dec 19, 2024 •

edited

Loading

pdonias Dec 19, 2024

b-Nollet Dec 19, 2024

julien-f Jan 6, 2025

fbeauchamp Jan 16, 2025

julien-f Jan 20, 2025

b-Nollet Jan 20, 2025

julien-f Jan 20, 2025

pdonias Jan 20, 2025

pdonias Jan 20, 2025

pdonias Jan 20, 2025

b-Nollet Jan 21, 2025

pdonias Jan 20, 2025

b-Nollet Jan 21, 2025 •

edited

Loading

pdonias Jan 21, 2025

b-Nollet Jan 21, 2025 •

edited

Loading

feat(xo-server): remember server's pool name and description #8206

Are you sure you want to change the base?

feat(xo-server): remember server's pool name and description #8206

Conversation

b-Nollet commented Dec 19, 2024 • edited Loading

Description

Checklist

Review process

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-Nollet Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-Nollet Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

b-Nollet commented Dec 19, 2024 •

edited

Loading

b-Nollet Jan 21, 2025 •

edited

Loading

b-Nollet Jan 21, 2025 •

edited

Loading