feat(xo-server-ipmi-sensors) : Created the plugin by All-Ki · Pull Request #9724 · vatesfr/xen-orchestra

All-Ki · 2026-04-15T09:46:36Z

Description

Move ipmi sensor discovery to it's own plugin and removed old logic, ships with a default configuration with the same behaviour as the old system.
The plugin can be configured via the UI, accepts an exact match for the vendor and regexes for the ipmi matching.

Tested via unit tests.

Tested with R620-L1 host on xo-lab by adding and removing Dell from configured vendors

Screenshots

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

If you are an external contributor, you can skip this part. Simply create the pull request, and we'll get back to you as soon as possible.

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.

spacotte-vates · 2026-04-15T12:31:26Z

+    const callIpmiPlugin = async function <T>(fn: string): Promise<T> {
+      return await xApiHost.$call<T>('call_plugin', 'ipmitool.py', fn, {})
+    }


This is some complicated code to be able call ipmi plugin functions, I understand this is the existing solution but since there are not a lot of functions I think it would be better to either call them directly without this intermediary function:
await (xApiHost.$call('call_plugin', 'ipmitool.py`, 'is_ipmi_device_available, {})) as string

0r externalize them in a separate ipmi-tool.mts file:

function isIpmiDeviceAvailable(): Promise<string>{ return xApiHost.$call('call_plugin', 'ipmitool.py`, 'is_ipmi_device_available, {}) }

Maybe this is outside of the card scope.

I'd think that the best would be an external util across all XO rather than a file specific one, agree with the idea though i'd say out of scope

Fair enough, I just want to get the opinion of Florent before resolving.

this would also need arguments , for example in another part of the codebase

const result = await this.call('host.call_plugin', this.pool.master, 'trim', 'do_trim', { sr_uuid: await this.getField('SR', srRef, 'uuid'), }) ``` it will be tricky to type though, since we can be connected to multiple host/pools, and they can have different versions of the plugin

I guess that's an issue for another day then, and might not be worth it since it's not used a lot across the codebase

spacotte-vates · 2026-04-16T08:11:32Z

Do a MAJ+CTRL+F search for "IMPI", there are still some not fixed.

spacotte-vates · 2026-04-16T08:14:02Z

I think you are also missing some function return types.

MathieuRA · 2026-04-16T08:18:05Z

+   * Load and start the plugin.
+   */
+  async load(): Promise<void> {
+    this.#removeApiMethod = (this.#xo as any).addApiMethod(


Why as any?
this.#xo type is correct

MathieuRA · 2026-04-16T08:34:10Z

+      }
+
+      addIpmiSensorDataType(ipmiSensor, productName, this.#configuredRulesByProduct)
+      const dataType = ipmiSensor.dataType as IMPI_SENSOR_DATA_TYPE_STRINGS


Avoid to use as as much as possible.

Here, addIpmiSensorDataType always injects the dataType property into your ipmiSensor object.
You can type addIpmiSensorDataType correctly to reflect what the function does.

export const addIpmiSensorDataType: ( data: ReturnedSensorData, productName: string, configuredRules: SensorRegexByProduct[] ) => asserts data is ReturnedSensorData & { dataType: IMPI_SENSOR_DATA_TYPE_STRINGS } = ( data, productName, configuredRules ) => {. .. }

Or using function instead anonymous function

export function addIpmiSensorDataType( data: ReturnedSensorData, productName: string, configuredRules: SensorRegexByProduct[] ): asserts data is ReturnedSensorData & { dataType: IMPI_SENSOR_DATA_TYPE_STRINGS } {...}

so, when you do addIpmiSensorDataType(foo, ...)
then foo.dataType will no longer be considered undefined

Oh thanks for this one

MathieuRA · 2026-04-16T08:36:51Z

+    },
+  }
+
+export const addCustomIpmiSensors = (ipmiSensorsByDataType: any, productName: string) => {


No any.
At least unknown if you can't type it. (or any but with a comment explaining why)
But based on the parameter name, I think it's possible to type it.

spacotte-vates · 2026-04-17T15:00:53Z

 - [i18n] Update Chinese (Simplified Han script), Czech, Danish, Dutch, Finnish, German, Italian, Korean, Norwegian, Persian, Polish, Portuguese, Portuguese (Brasil), Russian, Slovak and Spanish translations (PR [#9649](https://github.com/vatesfr/xen-orchestra/pull/9649))
 - [OpenMetrics] Add 9 missing host RRD metrics: `hostload`, `memory_reclaimed`, `memory_reclaimed_max`, `running_vcpus`, `pif_aggr_rx`, `pif_aggr_tx`, `iops_total`, `io_throughput_total`, `latency` per SR (PR [#9696](https://github.com/vatesfr/xen-orchestra/pull/9696))
 - [Netbox] Use platform hierarchy to assign versioned OS names (e.g. "Debian 12" instead of "Debian") when the major version is known (requires Netbox >= 4.4) #7773 (PR [#9644](https://github.com/vatesfr/xen-orchestra/pull/9644))
+- [IPMI] Add a plugin for configuration of ipmi rules and vendors [#9724] (https://github.com/vatesfr/xen-orchestra/pull/9724)


Suggested change

- [IPMI] Add a plugin for configuration of ipmi rules and vendors [#9724] (https://github.com/vatesfr/xen-orchestra/pull/9724)

- [IPMI] Add a plugin for configuration of ipmi rules and vendors ( PR [#9724] (https://github.com/vatesfr/xen-orchestra/pull/9724))

fbeauchamp

nice work, that's a very good first PR. Additionnally to my comments, there is a unknown at the bottom of the fields list. I don't understand what is its goal ?

fbeauchamp · 2026-04-21T11:45:40Z

+        result[key] = propSchema.default
+        hasDefaults = true
+      } else if (propSchema.type === 'object') {
+        const nested = getSchemaDefaults(propSchema)


I think we need this

Suggested change

const nested = getSchemaDefaults(propSchema)

const nested = this.getSchemaDefaults(propSchema)

fbeauchamp · 2026-04-21T11:47:14Z

+        this.getIpmiSensors.bind(this)
+      ),
+      // replacement for the method in packages/xo-server/src/api/host.mjs
+      this.#xo.addApiMethod<[{ id: XoHost | Branded<'host'> }], FinalSensorData>(


the original method had a permission check and a resolve property
this is important to keep them

fbeauchamp · 2026-04-21T11:50:47Z

+  const match = str.match(/^\/(.+)\/([a-z]*)$/i)
+  if (!match) return undefined
+  const [, pattern, flags] = match
+  if (pattern === undefined) return undefined


I think a failed regexp compilation shouldn't fail silently. Since it will make the plugin unusable, I am ok with throwing an error here

fbeauchamp · 2026-04-21T11:52:33Z

+  const customSensorsByDataType = IPMI_SENSOR_CUSTOM_BY_DATA_TYPE_BY_SUPPORTED_PRODUCT_NAME[productName]
+  for (const dataType in customSensorsByDataType) {
+    const dataTypeKey = dataType as IMPI_SENSOR_DATA_TYPE_STRINGS
+    const fn = customSensorsByDataType[dataTypeKey]!! // we know this exists since we're iterating over the keys of customSensorsByDataType


I think we only need on !

Suggested change

const fn = customSensorsByDataType[dataTypeKey]!! // we know this exists since we're iterating over the keys of customSensorsByDataType

const fn = customSensorsByDataType[dataTypeKey]! // we know this exists since we're iterating over the keys of customSensorsByDataType

fbeauchamp · 2026-04-21T11:53:41Z

+    if (systemManufacturer.includes('dell')) productName = 'dell'
+    if (systemManufacturer.includes('lenovo')) productName = 'lenovo'
+
+    const data = this.#configuredRulesByProduct || []


I don't think #configuredRulesByProduct can be undefined, since it's instantiated in the constructor and not accessible by exterior code

Suggested change

const data = this.#configuredRulesByProduct || []

const data = this.#configuredRulesByProduct

fbeauchamp · 2026-04-21T11:54:17Z

-getIpmiSensors.resolve = {
-  host: ['id', 'host', 'administrate'],
-}
+// host.getApiMethod moved to packages/xo-server-ipmi-sensors/src/index.mts


Suggested change

// host.getApiMethod moved to packages/xo-server-ipmi-sensors/src/index.mts

// host.getIpmiSensors moved to packages/xo-server-ipmi-sensors/src/index.mts

fbeauchamp · 2026-04-21T11:57:05Z

+// ============================================================================
+
+class IpmiSensorsPlugin {
+  #configuration: PluginConfiguration | undefined


this looks unused, is it intended ?

I wasnt sure if configuration is a required parameter or not, i thought it was automatically set by the framework

All-Ki requested a review from spacotte-vates April 15, 2026 09:46

All-Ki force-pushed the feat/xo-server-ipmi-sensors branch 3 times, most recently from 07394cb to 22e797a Compare April 15, 2026 10:29

All-Ki marked this pull request as ready for review April 15, 2026 11:35

spacotte-vates requested changes Apr 15, 2026

View reviewed changes

Comment thread packages/xo-server-ipmi-sensors/README.md Outdated

All-Ki force-pushed the feat/xo-server-ipmi-sensors branch 6 times, most recently from 105774b to 17e496d Compare April 15, 2026 15:30

spacotte-vates self-requested a review April 16, 2026 08:13

spacotte-vates approved these changes Apr 16, 2026

View reviewed changes

spacotte-vates requested a review from fbeauchamp April 16, 2026 08:13

MathieuRA reviewed Apr 16, 2026

View reviewed changes

All-Ki force-pushed the feat/xo-server-ipmi-sensors branch 4 times, most recently from 0e719b4 to 4f063fc Compare April 17, 2026 12:58

Moved ipmi to it's own plugin and removed obsolete code

1feb8b6

All-Ki force-pushed the feat/xo-server-ipmi-sensors branch from 4f063fc to 1feb8b6 Compare April 17, 2026 13:05

spacotte-vates reviewed Apr 17, 2026

View reviewed changes

fbeauchamp reviewed Apr 21, 2026

View reviewed changes

	- [IPMI] Add a plugin for configuration of ipmi rules and vendors [#9724] (https://github.com/vatesfr/xen-orchestra/pull/9724)
	- [IPMI] Add a plugin for configuration of ipmi rules and vendors ( PR [#9724] (https://github.com/vatesfr/xen-orchestra/pull/9724))

	const nested = getSchemaDefaults(propSchema)
	const nested = this.getSchemaDefaults(propSchema)

	const fn = customSensorsByDataType[dataTypeKey]!! // we know this exists since we're iterating over the keys of customSensorsByDataType
	const fn = customSensorsByDataType[dataTypeKey]! // we know this exists since we're iterating over the keys of customSensorsByDataType

	const data = this.#configuredRulesByProduct \|\| []
	const data = this.#configuredRulesByProduct

	// host.getApiMethod moved to packages/xo-server-ipmi-sensors/src/index.mts
	// host.getIpmiSensors moved to packages/xo-server-ipmi-sensors/src/index.mts

Conversation

All-Ki commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots

Checklist

Review process

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spacotte-vates Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spacotte-vates commented Apr 16, 2026

Uh oh!

spacotte-vates commented Apr 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbeauchamp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

All-Ki commented Apr 15, 2026 •

edited

Loading

spacotte-vates Apr 15, 2026 •

edited

Loading