Add CPU Pinning modal and functionality #3805

glekner · 2019-12-18T15:40:13Z

Add CPU Pinning modal and functionality

glekner · 2019-12-19T12:43:33Z

---- in Overview -----

---- Modal -----

Some screenshots @Ranelim @matthewcarleton . thoughts?
Also, what should we display in case of no changes to the VM (e.g. the checkbox is unchecked)? just a blank space?

@suomiy @mareklibra @yaacov please review

yaacov · 2019-12-19T12:48:00Z

frontend/.vscode/settings.json

nice, but should it be in this PR ?

this was automatically changed by VS Code..everytime i revert the changes it creates the patch again immediately. does it happen to anyone else?

matthewcarleton · 2019-12-19T16:22:17Z

@glekner I'd like @Ranelim to weigh in here but I think this should be called "Resource scheduling" It's more descriptive and I don't think we want to use the term "Device" here. It will conflict with virtual hardware.
If we go that route then the empty state could be "No resource scheduling applied" or something similar.

matthewcarleton · 2019-12-19T16:40:25Z

...tend/packages/kubevirt-plugin/src/components/modals/cpu-pinning-modal/cpu-pinning-modal.scss

Do you need this wrapping div?

atiratree · 2019-12-19T16:35:32Z

frontend/packages/kubevirt-plugin/src/k8s/patches/vm/vm-cpu-patches.ts

we should not discard other CPU attributes with this operation.

maybe better to call patch on just dedicatedCpuPlacement ? and create a new default entry if the CPU and resources.requests.cpu is missing -

atiratree · 2019-12-19T16:41:12Z

frontend/packages/kubevirt-plugin/src/selectors/vm/selectors.ts

can we keep the naming the same? - easier to distinguish what this selector does

How would you name it?

like the yaml name?

atiratree · 2019-12-19T16:42:44Z

frontend/packages/kubevirt-plugin/src/k8s/patches/vm/vm-cpu-patches.ts

vm is passed in the (vm) => patches function so you don't have to call asVM in the beggining

asVM is for the getCPU

i'm going one level deeper, patching just dedicatedCpuPlacement so no need for cpu parsing

this will not work if the cpu field is missing

atiratree · 2019-12-19T16:50:09Z

frontend/packages/kubevirt-plugin/src/k8s/patches/vm/vm-cpu-patches.ts

we should take resources.requests.cpu into account when making the patches: please see https://kubevirt.io/user-guide/docs/latest/creating-virtual-machines/dedicated-cpu.html

atiratree · 2019-12-19T16:52:55Z

frontend/packages/kubevirt-plugin/src/components/modals/cpu-pinning-modal/cpu-pinning-modal.tsx

cpu pinning is tied together with number of cpus/cores/sockets. We probably should edit these attributes and the pinning at once.

There is a significant overlap with the flavor modal - it would probably make sense to unite these

@jelkosz @matthewcarleton

I agree @suomiy but I think @jelkosz felt like this is more advanced than flavor. @Ranelim have an effort this sprint to look at topology and how it could align with this approach.

atiratree · 2019-12-19T17:00:54Z

frontend/packages/kubevirt-plugin/src/components/modals/cpu-pinning-modal/cpu-pinning-modal.tsx

we probably should check the nodes if there are some with cpumanager=true label before we start offering the pinning. Or maybe some warning?

can you elaborate?

https://kubevirt.io/user-guide/docs/latest/creating-virtual-machines/dedicated-cpu.html#identifying-nodes-with-a-running-cpu-manager

atiratree · 2019-12-19T17:03:37Z

frontend/packages/kubevirt-plugin/src/components/modals/cpu-pinning-modal/cpu-pinning-modal.tsx

why is this necessary? can't we just use the errorMessage?

This is the new modal footer, took example from boot-order-modal@122

matthewcarleton · 2019-12-19T18:55:56Z

frontend/packages/kubevirt-plugin/src/components/modals/cpu-pinning-modal/cpu-pinning-modal.tsx

It doesn't look like this working properly. I'm seeing an h1 which we should avoid because we already have an h1 for the title of the modal.

is span acceptable here?

I'd stick to using headings. You could use an h2.

matthewcarleton · 2019-12-19T18:59:18Z

frontend/packages/kubevirt-plugin/src/components/modals/cpu-pinning-modal/cpu-pinning-modal.tsx

looks like the alignment of the footer is a bit off. I'm seeing a wrapping div around the footer which shouldn't be needed.
Do we need footer={footer}?

yes, custom actions and loading animations. ill fix the left padding

atiratree · 2020-01-02T19:17:53Z

frontend/packages/kubevirt-plugin/src/selectors/vm/selectors.ts

like the yaml name?

atiratree · 2020-01-02T21:19:08Z

frontend/packages/kubevirt-plugin/src/components/modals/cpu-pinning-modal/cpu-pinning-modal.tsx

https://kubevirt.io/user-guide/docs/latest/creating-virtual-machines/dedicated-cpu.html#identifying-nodes-with-a-running-cpu-manager

atiratree · 2020-01-02T21:48:30Z

frontend/packages/kubevirt-plugin/src/k8s/patches/vm/vm-cpu-patches.ts

this will not work if the cpu field is missing

glekner · 2020-01-05T21:14:46Z

@suomiy I fixed your comments.

regarding this section https://kubevirt.io/user-guide/docs/latest/creating-virtual-machines/dedicated-cpu.html#identifying-nodes-with-a-running-cpu-manager

does this label check require us to search inside all nodes for that specific label? wouldn't it hurt performance?

atiratree · 2020-01-06T12:15:25Z

@suomiy I fixed your comments.

#3805 (comment) still needs some attention

regarding this section https://kubevirt.io/user-guide/docs/latest/creating-virtual-machines/dedicated-cpu.html#identifying-nodes-with-a-running-cpu-manager

does this label check require us to search inside all nodes for that specific label? wouldn't it hurt performance?

I think it should be okay performance-wise since we would be fetching just the nodes inside the modal.
I guess it is not a hard requirement but a warning could be nice to have, IMO.

glekner · 2020-01-07T12:59:01Z

@suomiy sorry I was pushing changes to the wrong branch.

glekner · 2020-01-07T13:01:12Z

frontend/packages/kubevirt-plugin/src/k8s/patches/vm/vm-cpu-patches.ts

@suomiy in case cpu is not available, do we even want to give an option to patch dedicatedCpuPlacement?

I don't see a reason why not. We should just ensure there is no overlap between resources.requests.cpu and domain.cpu and have CPUs in just one place.

glekner · 2020-01-15T10:10:49Z

Patch now considers spec.domain.resources.[requests/limits].cpu
Added an Alert to the modal with the cpumanager=true warning.

@matthewcarleton @Ranelim are we ok with this for now? @suomiy please review.

matthewcarleton · 2020-01-15T14:59:09Z

@glekner is this a warning or info alert? As a warning it feels like I'm being warned that it isn't going to happen, Info seems more accurate if I'm understanding this correctly.
Also, nit: A node with the cpumanager=true label is required. I also wonder if we should highlight the label. @andybraren do you know if in the console anywhere we treat labels differently when they are inline like this?

glekner · 2020-01-15T15:33:33Z

@matthewcarleton changed alert to info

atiratree · 2020-01-21T09:49:07Z

/lgtm cancel

atiratree · 2020-01-21T09:49:47Z

frontend/packages/console-shared/src/selectors/common.ts

missing a type

atiratree · 2020-01-21T10:43:04Z

/lgtm

rawagner · 2020-01-21T10:45:05Z

frontend/packages/console-shared/src/selectors/common.ts

Suggested change

defaultValue?: undefined,

defaultValue?: string,

rawagner · 2020-01-21T10:47:44Z

...ubevirt-plugin/src/components/modals/dedicated-resources-modal/dedicated-resources-modal.tsx

can be

Suggested change

const ResourceModal = withHandlePromise(

const ResourceModal = withHandlePromise<ResourceModalProps>(

and you wont have to specify that props type is ResourceModalProps a few lines below

rawagner · 2020-01-21T10:53:11Z

...ubevirt-plugin/src/components/modals/dedicated-resources-modal/dedicated-resources-modal.tsx

Suggested change

onChange={(flag) => setIsPinned(flag)}

onChange={setIsPinned}

rawagner · 2020-01-21T10:53:38Z

...ubevirt-plugin/src/components/modals/dedicated-resources-modal/dedicated-resources-modal.tsx

no need to disable this rule

rawagner · 2020-01-21T10:57:03Z

...ubevirt-plugin/src/components/modals/dedicated-resources-modal/dedicated-resources-modal.tsx

it seems that its better to pass onClose which wont take any parameter as input as you always call setOpen(false).

actually pass the name should be close as defined in ModalComponentProps

rawagner · 2020-01-21T11:05:57Z

frontend/packages/kubevirt-plugin/src/components/modals/modal/modal-footer.tsx

it would be better to use classNames('co-m-btn-bar modal-footer kubevirt-create-nic-modal__buttons', className) for the footer and leave className prop empty.

<footer className={classNames('co-m-btn-bar modal-footer kubevirt-create-nic-modal__buttons', className)}>

rawagner · 2020-01-21T11:07:39Z

frontend/packages/kubevirt-plugin/src/components/modals/modal/modal-footer.tsx

why all the null and false values as defaults ? you can keep the these props empty and the result will be the same and the function declaration more tidy

rawagner · 2020-01-21T11:11:36Z

frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template-resource.tsx

as metioned above, it would be better to pass onClose callback

const onClose = React.useCallback(() => setDedicatedResourcesModalOpen(false), []); ... <DedicatedResourcesModal vmLikeEntity={template} isOpen={isDedicatedResourcesModalOpen} onClose={onClose}

rawagner · 2020-01-21T11:13:06Z

frontend/packages/kubevirt-plugin/src/k8s/patches/vm/vm-cpu-patches.ts

its better to add return type to have clear function contract

Suggested change

) => {

): Patch[] => {

rawagner · 2020-01-21T11:13:50Z

frontend/packages/kubevirt-plugin/src/selectors/vm/selectors.ts

you can use TS 3.7 optional chaining for all _.get usages

rawagner · 2020-01-21T13:22:00Z

/lgtm

openshift-ci-robot · 2020-01-21T13:22:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, rawagner, suomiy

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [rawagner]

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

openshift-bot · 2020-01-21T15:49:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-21T19:32:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-21T19:50:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett · 2020-01-21T23:18:46Z

/hold
the merge queue is blocked

spadgett · 2020-01-22T01:32:33Z

/hold cancel
/retest

openshift-bot · 2020-01-22T02:46:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-22T03:36:35Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-01-22T04:54:28Z

@glekner: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-console	`ffca7fc`	link	`/test e2e-gcp-console`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 18, 2019

openshift-ci-robot requested review from spadgett and vojtechszocs December 18, 2019 15:41

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Dec 18, 2019

glekner force-pushed the cpu-pinning-modal branch 3 times, most recently from 33565bd to e12c553 Compare December 18, 2019 16:03

yaacov reviewed Dec 19, 2019

View reviewed changes

matthewcarleton reviewed Dec 19, 2019

View reviewed changes

atiratree suggested changes Dec 19, 2019

View reviewed changes

matthewcarleton reviewed Dec 19, 2019

View reviewed changes

glekner force-pushed the cpu-pinning-modal branch from e12c553 to 36933d8 Compare December 23, 2019 17:09

atiratree suggested changes Jan 2, 2020

View reviewed changes

glekner force-pushed the cpu-pinning-modal branch from 36933d8 to 2b74cbf Compare January 7, 2020 12:58

glekner force-pushed the cpu-pinning-modal branch 2 times, most recently from fa5f021 to bc7443d Compare January 7, 2020 13:00

glekner commented Jan 7, 2020

View reviewed changes

glekner force-pushed the cpu-pinning-modal branch from bc7443d to 5dcc307 Compare January 15, 2020 10:08

glekner force-pushed the cpu-pinning-modal branch 2 times, most recently from e2ea0e2 to 7fbb9a9 Compare January 15, 2020 10:18

glekner force-pushed the cpu-pinning-modal branch from 7fbb9a9 to 824c6f7 Compare January 15, 2020 15:33

openshift-ci-robot assigned atiratree Jan 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

atiratree reviewed Jan 21, 2020

View reviewed changes

frontend/packages/console-shared/src/selectors/common.ts Outdated

Copy link

Member

atiratree Jan 21, 2020

Choose a reason for hiding this comment

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

missing a type

glekner force-pushed the cpu-pinning-modal branch from 49990f8 to 76fba54 Compare January 21, 2020 10:42

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

rawagner suggested changes Jan 21, 2020

View reviewed changes

add cpu pinning modal and functionality

ffca7fc

glekner force-pushed the cpu-pinning-modal branch from 76fba54 to ffca7fc Compare January 21, 2020 13:19

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

openshift-ci-robot assigned rawagner Jan 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2020

rawagner mentioned this pull request Jan 21, 2020

Add unpause VM feature #3985

Merged

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2020

openshift-merge-robot merged commit 342c6a3 into openshift:master Jan 22, 2020

spadgett added this to the v4.4 milestone Jan 27, 2020

	const ResourceModal = withHandlePromise(
	const ResourceModal = withHandlePromise<ResourceModalProps>(

	onChange={(flag) => setIsPinned(flag)}
	onChange={setIsPinned}

Add CPU Pinning modal and functionality #3805

Add CPU Pinning modal and functionality #3805

Uh oh!

Conversation

glekner commented Dec 18, 2019

Uh oh!

glekner commented Dec 19, 2019

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!

matthewcarleton commented Dec 19, 2019

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!

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!

glekner Dec 22, 2019 • 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!

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!

glekner commented Jan 5, 2020

Uh oh!

atiratree commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glekner commented Jan 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

glekner Dec 22, 2019 •

edited

Loading

atiratree commented Jan 6, 2020 •

edited

Loading

rawagner Jan 21, 2020 •

edited

Loading