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

feat(xo-server/xo-web): in host advanced tab, display mdadm status #8190

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

> Users must be able to say: “Nice enhancement, I'm eager to test it”
- [Host] In host and advanced view, display MDADM health information (PR [#8190](https://github.com/vatesfr/xen-orchestra/pull/8190))

Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- [Host] In host and advanced view, display MDADM health information (PR [#8190](https://github.com/vatesfr/xen-orchestra/pull/8190))
- [Host/Advanced] In host's advanced tab, display MDADM health information (PR [#8190](https://github.com/vatesfr/xen-orchestra/pull/8190))

### Bug fixes

> Users must be able to say: “I had this issue, happy to know it's fixed”
Expand All @@ -39,6 +41,7 @@
- @xen-orchestra/web-core minor
- @xen-orchestra/xapi patch
- xen-api minor
- xo-server patch
- xo-server minor
- xo-web minor

<!--packages-end-->
18 changes: 18 additions & 0 deletions packages/xo-server/src/api/host.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ const CACHE_2CRSI = new TTLCache({
ttl: 6e4,
})

const CACHE_MDADM = new TTLCache({
ttl: 6e5,
})

const log = createLogger('xo:api:host')

// ===================================================================
Expand Down Expand Up @@ -624,6 +628,20 @@ getSmartctlInformation.resolve = {
host: ['id', 'host', 'view'],
}

export function getMdadmHealth({ host }) {
return this.getXapi(host).getHostMdadmHealth(host._xapiId, { cache: CACHE_MDADM })
}

getMdadmHealth.description = 'get mdadm health status'
Copy link
Member

Choose a reason for hiding this comment

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

get mdadm health status sound like it returns a simple string..


getMdadmHealth.params = {
id: { type: 'string' },
}

getMdadmHealth.resolve = {
host: ['id', 'host', 'view'],
}

export async function getBlockdevices({ host }) {
const xapi = this.getXapi(host)
if (host.productBrand !== 'XCP-ng') {
Expand Down
20 changes: 20 additions & 0 deletions packages/xo-server/src/xapi/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -1569,4 +1569,24 @@ export default class Xapi extends XapiBase {

return { currentBiosVersion, latestBiosVersion, biosLink, isUpToDate }
}

async getHostMdadmHealth(hostId, { cache } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

Why put a cache in this method?
check_raid_pool takes a long time to respond?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was suggested by Olivier in the issue

if (cache?.has(hostId)) {
return cache.get(hostId)
}
try {
const result = await this.call('host.call_plugin', this.getObject(hostId).$ref, 'raid.py', 'check_raid_pool', {})
Copy link
Member

Choose a reason for hiding this comment

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

If the call take more than a second, use this.callAsync

const parsedResult = JSON.parse(result)

cache?.set(hostId, parsedResult)

return parsedResult
} catch (error) {
if (error.code === 'XENAPI_MISSING_PLUGIN' || error.code === 'UNKNOWN_XENAPI_PLUGIN_FUNCTION') {
return null
} else {
throw error
}
}
}
Comment on lines +1573 to +1591
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, this method can be moved into @xen-orchestra/xapi/host.mjs

}
3 changes: 3 additions & 0 deletions packages/xo-web/src/common/intl/messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -1144,6 +1144,9 @@ const messages = {
supplementalPackInstallSuccessTitle: 'Installation success',
supplementalPackInstallSuccessMessage: 'Supplemental pack successfully installed.',
systemDisksHealth: 'System disks health',
raidHealthy: 'All mdadm RAID are healthy ✅',
raidStateWarning: 'Raid state needs your attention : { state }',
raidStatus: 'Raid Status',
uniqueHostIscsiIqnInfo: 'The iSCSI IQN must be unique. ',
vendorId: 'Vendor ID',
// ----- Host net tabs -----
Expand Down
2 changes: 2 additions & 0 deletions packages/xo-web/src/common/xo/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1403,6 +1403,8 @@ export const getSmartctlHealth = host => _call('host.getSmartctlHealth', { id: r
export const getSmartctlInformation = (host, deviceNames) =>
_call('host.getSmartctlInformation', { id: resolveId(host), deviceNames })

export const getMdadmHealth = host => _call('host.getMdadmHealth', { id: resolveId(host) })

export const installCertificateOnHost = (id, props) => _call('host.installCertificate', { id, ...props })

export const setControlDomainMemory = (id, memory) => _call('host.setControlDomainMemory', { id, memory })
Expand Down
24 changes: 23 additions & 1 deletion packages/xo-web/src/xo-app/home/host-item.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
addTag,
editHost,
fetchHostStats,
getMdadmHealth,
isHostTimeConsistentWithXoaTime,
isPubKeyTooShort,
removeTag,
Expand Down Expand Up @@ -63,6 +64,7 @@ export default class HostItem extends Component {
state = {
isHostTimeConsistentWithXoaTime: true,
isPubKeyTooShort: false,
mdadmHealth: null,
Copy link
Member

Choose a reason for hiding this comment

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

Why initialize as null instead of undefined?

}

componentWillMount() {
Expand All @@ -72,6 +74,13 @@ export default class HostItem extends Component {
isHostTimeConsistentWithXoaTime: value,
})
)

this.fetchMdadmHealth()
Copy link
Member

Choose a reason for hiding this comment

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

Why not using subscription?

}

async fetchMdadmHealth() {
const mdadmHealth = await getMdadmHealth(this.props.item).catch(() => null)
this.setState({ mdadmHealth })
Comment on lines +82 to +83
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const mdadmHealth = await getMdadmHealth(this.props.item).catch(() => null)
this.setState({ mdadmHealth })
const mdadmHealth = await getMdadmHealth(this.props.item).catch(console.error)
if(mdadmHealth != null) {
this.setState({ mdadmHealth })
}

}

get _isRunning() {
Expand Down Expand Up @@ -144,14 +153,16 @@ export default class HostItem extends Component {
this._getAreHostsVersionsEqual,
() => this.props.state.hostsByPoolId[this.props.item.$pool],
() => this.state.isPubKeyTooShort,
() => this.state.mdadmHealth,
(
needsRestart,
host,
isMaintained,
isHostTimeConsistentWithXoaTime,
areHostsVersionsEqual,
poolHosts,
isPubKeyTooShort
isPubKeyTooShort,
mdadmHealth
) => {
const alerts = []

Expand Down Expand Up @@ -262,6 +273,17 @@ export default class HostItem extends Component {
})
}

if (mdadmHealth?.raid?.State && !['clean', 'active'].includes(mdadmHealth.raid.State)) {
Copy link
Member

@MathieuRA MathieuRA Dec 16, 2024

Choose a reason for hiding this comment

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

Always explicit condition. Exepct for boolean

Suggested change
if (mdadmHealth?.raid?.State && !['clean', 'active'].includes(mdadmHealth.raid.State)) {
if (mdadmHealth?.raid?.State !== undefined && !['clean', 'active'].includes(mdadmHealth.raid.State)) {

alerts.push({
level: 'danger',
render: (
<span>
<Icon icon='alarm' className='text-danger' /> {_('raidStateWarning', { state: mdadmHealth.raid.State })}
</span>
),
})
}

return alerts
}
)
Expand Down
13 changes: 13 additions & 0 deletions packages/xo-web/src/xo-app/host/tab-advanced.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
isPciHidden,
isPciPassthroughAvailable,
isNetDataInstalledOnHost,
getMdadmHealth,
getPlugin,
getSmartctlHealth,
getSmartctlInformation,
Expand Down Expand Up @@ -319,13 +320,17 @@ export default class extends Component {
)
}

const mdadmHealth = await getMdadmHealth(host).catch(console.error)

this.setState({
isHtEnabled: await isHyperThreadingEnabledHost(host).catch(() => null),
isSmartctlHealthEnabled,
pciStateById,
smartctlUnhealthyDevices,
unhealthyDevicesAlerts,
isPciPassthroughAvailable: _isPciPassthroughAvailable,
mdadmHealthAvailable: mdadmHealth?.raid ? mdadmHealth : null,
Copy link
Member

Choose a reason for hiding this comment

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

Why null instead of undefined?

Copy link
Member

Choose a reason for hiding this comment

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

mdadmHealthAvailable seems to indicate that we are expecting a boolean.

Suggested change
mdadmHealthAvailable: mdadmHealth?.raid ? mdadmHealth : null,
mdadmHealthAvailable: mdadmHealth != null ? true : false,

mdadmState: mdadmHealth.raid?.State,
Copy link
Member

Choose a reason for hiding this comment

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

      mdadmState: mdadmHealthAvailable ? mdadmHealth.raid?.State : undefined,

})
}

Expand Down Expand Up @@ -394,6 +399,8 @@ export default class extends Component {
isSmartctlHealthEnabled,
unhealthyDevicesAlerts,
smartctlUnhealthyDevices,
mdadmHealthAvailable,
mdadmState,
} = this.state

const _isXcpNgHost = host.productBrand === 'XCP-ng'
Expand Down Expand Up @@ -688,6 +695,12 @@ export default class extends Component {
))}
</td>
</tr>
{mdadmHealthAvailable && (
<tr>
<th>{_('raidStatus')}</th>
<td>{mdadmState === 'clean' || mdadmState === 'active' ? _('raidHealthy') : mdadmState}</td>
</tr>
)}
Comment on lines +698 to +703
Copy link
Member

Choose a reason for hiding this comment

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

Avoid to hide information, add a message that explain "Why mdadmHealthAvailable is false".
It will probably depend of the mdadmHealth, but it can be plugin not installed, or no raid on the host

</tbody>
</table>
<br />
Expand Down
Loading