-
Notifications
You must be signed in to change notification settings - Fork 264
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-web): in health tab, hide PVS accelerator in orphan VDIs #8039
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to add CHANGELOG.unreleased.md
@@ -515,7 +515,7 @@ const ALARM_ACTIONS = [ | |||
}, | |||
] | |||
|
|||
const HANDLED_VDI_TYPES = new Set(['system', 'user', 'ephemeral']) | |||
const HANDLED_VDI_TYPES = new Set(['system', 'user', 'ephemeral', 'pvs_cache']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const HANDLED_VDI_TYPES = new Set(['system', 'user', 'ephemeral', 'pvs_cache']) | |
const HANDLED_VDI_TYPES = new Set(['system', 'user', 'ephemeral']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add the pvs_cache
type?
HANDLED_VDI_TYPES
corresponds to the types of VDI
we want to display.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But since 'pvs_cache'
is a VDI_type
, wouldn't it be better to use that instead of the name_label
and name_description
to exclude them? /cc @olivierlambert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this type being used on this forum's message: https://xcp-ng.org/forum/post/81940
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to post the same answer ^^ There's nothing (for what I can see) in the XAPI record to discriminate it outside the name label & description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do see a 'pvs_cache'
type here though: https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/xapi_vdi.ml#L623-L624
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too, but we need to test how it works in real life.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't bet about the behaviour on something not even visible in the printed xe record. Even if a name/description isn't perfect, at least we have a proof on how it works, and we have 0 chances to get a PVS server around to test it. So even if it's counter-intuitive, I found the name solution safer than hopping XAPI is doing its work correctly.
if (vdi.name_label === 'PVS cache VDI' && vdi.name_description === 'PVS cache VDI') { | ||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can mutualize that with the if
above.
Also add a comment that link to github issue.
if(
... ||
vdi.name_label === 'PVS cache VDI' && vdi.name_description === 'PVS cache VDI' // see https://github.com/vatesfr/xen-orchestra/issues/7938
) {...}
@@ -515,7 +515,7 @@ const ALARM_ACTIONS = [ | |||
}, | |||
] | |||
|
|||
const HANDLED_VDI_TYPES = new Set(['system', 'user', 'ephemeral']) | |||
const HANDLED_VDI_TYPES = new Set(['system', 'user', 'ephemeral', 'pvs_cache']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But since 'pvs_cache'
is a VDI_type
, wouldn't it be better to use that instead of the name_label
and name_description
to exclude them? /cc @olivierlambert
Description
Fixes #7938
Checklist
Fixes #007
,See xoa-support#42
,See https://...
)Introduced by
CHANGELOG.unreleased.md
Review process
Notes: