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

Add guest agent info to vm resource summary #5711

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Jun 10, 2020

Add guest agent info to vm resource summary

Design: http://openshift.github.io/openshift-origin-design/designs/virtualization/4.x/expose-guest-data/expose%20guest%20data.html

  • add os pretty name
  • add timezone
  • add hostname
  • alert on user-os / guest-agent-os missmatch

Notes:
supported os types - https://github.com/kubevirt/common-templates#templates
downstream struct - https://www.qemu.org/docs/master/qemu-ga-ref.html#index-GuestOSInfo

Screenshot:
Add hostname, timezone and os name:
screenshot-localhost_9000-2020 06 10-14_31_10

Add alert on OS mismatch:
screenshot-localhost_9000-2020 06 11-17_39_13

OS name from GA and from YAML:
Peek 2020-06-11 18-55

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/kubevirt Related to kubevirt-plugin labels Jun 10, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2020
@yaacov yaacov force-pushed the guest-agent-data-in-vm-resource-summary branch 3 times, most recently from b028b65 to 76f1e85 Compare June 10, 2020 11:32
@yaacov
Copy link
Member Author

yaacov commented Jun 10, 2020

@pcbailey please review

import { getVMIApiPath, getVMISubresourcePath } from '../selectors/vmi/selectors';
import { isGuestAgentInstalled } from '../components/dashboards-page/vm-dashboard/vm-alerts';

const guestAgentURL = (vmi: VMIKind) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Can we change this to getGuestAgentURL?

@pcbailey
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2020
@yaacov yaacov force-pushed the guest-agent-data-in-vm-resource-summary branch from 76f1e85 to cf6b25b Compare June 11, 2020 14:32
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2020
@yaacov yaacov changed the title [WIP] Add guest agent info to vm resource summary Add guest agent info to vm resource summary Jun 11, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2020
@yaacov yaacov force-pushed the guest-agent-data-in-vm-resource-summary branch 2 times, most recently from 44aa887 to 766960c Compare June 11, 2020 15:03
@yaacov
Copy link
Member Author

yaacov commented Jun 11, 2020

/test frontend

@@ -78,11 +81,24 @@ export const VMDetails: React.FC<VMDetailsProps> = (props) => {
const vmiLike = kindObj === VirtualMachineModel ? vm : vmi;
const vmServicesData = getServicesForVmi(getLoadedData(props.services, []), vmi);
const canUpdate = useAccessReview(asAccessReview(kindObj, vmiLike || {}, 'patch')) && !!vmiLike;
const [guestAgentInfo] = useGuestAgentInfo({ vmi });

const OSMismatch =
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Can we call this OSMismatchExists?

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 fixed

const OSMismatch =
vmi && guestAgentInfo && isWindows(vmiLike) !== (guestAgentInfo?.os?.id === 'mswindows');
const OSMismatchAlert = OSMismatch && (
<Alert className="co-alert" variant="warning" title="Operating syatem mismatch" isInline>
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo in the title: syatem => system

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks ❗

<Alert className="co-alert" variant="warning" title="Operating syatem mismatch" isInline>
The operating system defined for this virtual machine does not match what is being reported by
the Guest Agent. In order to correct this, you need to re-create the virtual machine with the
currect VM selection. The disks of this virtual machine and be attached to the newly created
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: currect => correct

I think what you're trying to say in the last sentence is "The disks of this virtual machine can be attached to the newly created one.", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes =)

@yaacov yaacov force-pushed the guest-agent-data-in-vm-resource-summary branch from 766960c to 980809b Compare June 11, 2020 15:16
@yaacov yaacov force-pushed the guest-agent-data-in-vm-resource-summary branch from 980809b to 9587148 Compare June 11, 2020 15:20
@pcbailey
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pcbailey, yaacov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 6354589 into openshift:master Jun 11, 2020
@spadgett spadgett added this to the v4.6 milestone Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants