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

Miscellaneous fixes to vCPUs warnings #3160

Merged
merged 6 commits into from
Jul 10, 2023

Conversation

danilo-delbusso
Copy link
Member

@danilo-delbusso danilo-delbusso commented Jun 16, 2023

Found these while writing test scenarios for the changes.

  • The _ovfVCpusCount was being cleared at every iteration and only one CPU count remained at the end of it. Caused the > 32 vCPUs to not appear on multi-VM imports with mixed vCPU counts
  • Ensure the warning is correctly calculated when moving back and fourth within pages
  • Ensure warning takes into account every host selected in the selection page. Previously it was only taking into account the SelectedTarget, which is the last item that was selected. As part of this, I also added a new message and modified an existing one.

Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>
@danilo-delbusso danilo-delbusso added the ASAP PR should be reviewed as soon as possible label Jun 16, 2023
@danilo-delbusso danilo-delbusso self-assigned this Jun 16, 2023
Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>
CitrixChris
CitrixChris previously approved these changes Jun 19, 2023
@danilo-delbusso danilo-delbusso added the do not merge Do not merge the PR label Jun 19, 2023
jijiang
jijiang previously approved these changes Jun 19, 2023
@danilo-delbusso danilo-delbusso marked this pull request as draft June 19, 2023 10:59
@danilo-delbusso danilo-delbusso removed the do not merge Do not merge the PR label Jun 19, 2023
@danilo-delbusso danilo-delbusso dismissed stale reviews from jijiang and CitrixChris June 19, 2023 10:59

PR not ready yet

@danilo-delbusso danilo-delbusso removed the ASAP PR should be reviewed as soon as possible label Jun 19, 2023
Adds a new field in `SelectMultipleVMDestinationPage`: `AllSelectedTargets`.

This new field contains a list of all hosts that have been selected. This way `ImportSelectHostPage` can check for each `Host` that has been selected individually and add more relevant messages

Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>
@danilo-delbusso danilo-delbusso changed the title Move _ovfVCpusCount.Clear outside content check loop Miscellaneous fixes to vCPUs warnings Jun 19, 2023
@danilo-delbusso danilo-delbusso marked this pull request as ready for review June 19, 2023 12:48
@danilo-delbusso danilo-delbusso added the ASAP PR should be reviewed as soon as possible label Jun 19, 2023
@danilo-delbusso danilo-delbusso added ITE PR should be reviewed within this iteration and removed ASAP PR should be reviewed as soon as possible labels Jun 19, 2023
@danilo-delbusso danilo-delbusso added 1 approval PR has been approved by one reviewer ASAP PR should be reviewed as soon as possible and removed ITE PR should be reviewed within this iteration labels Jun 26, 2023
Copy link
Contributor

@kc284 kc284 left a comment

Choose a reason for hiding this comment

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

Only minor comments.

XenModel/Messages.resx Outdated Show resolved Hide resolved
XenModel/Messages.resx Outdated Show resolved Hide resolved
@kc284 kc284 added the needs updating A reviewer has requested changes label Jul 3, 2023
Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>
Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>
@danilo-delbusso danilo-delbusso added updated Changes completed, please review needs answer PR contains question that needs to be answered and removed needs updating A reviewer has requested changes labels Jul 3, 2023
@kc284 kc284 removed the needs answer PR contains question that needs to be answered label Jul 10, 2023
Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>
@kc284 kc284 merged commit 33d0b72 into xenserver:master Jul 10, 2023
1 check passed
@kc284 kc284 added 2 approvals PR has been approved by two reviewers and removed 1 approval PR has been approved by one reviewer updated Changes completed, please review labels Jul 10, 2023
@danilo-delbusso danilo-delbusso deleted the dev/vcpus-warnings branch July 10, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 approvals PR has been approved by two reviewers ASAP PR should be reviewed as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants