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

MGMT-15054: FCP devices (multipath) do not show correct size #2286

Merged
merged 8 commits into from
Aug 24, 2023

Conversation

jgyselov
Copy link
Collaborator

@jgyselov jgyselov commented Aug 4, 2023

https://issues.redhat.com/browse/MGMT-15054

If a disk has defined holders, it is not included in the total storage size calculations.

@jgyselov jgyselov requested a review from a team as a code owner August 4, 2023 09:23
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 4, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 4, 2023

@jgyselov: This pull request references MGMT-15054 which is a valid jira issue.

In response to this:

https://issues.redhat.com/browse/MGMT-15054

If a disk has defined holders, it is not included in the total storage size calculations.

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.

@jgyselov jgyselov added api-review Categorizes an issue or PR as actively needing an API review. and removed jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Aug 4, 2023
@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 4, 2023
@jgyselov jgyselov added OCM and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. api-review Categorizes an issue or PR as actively needing an API review. labels Aug 4, 2023
@jgyselov jgyselov added this to the v2.25 milestone Aug 4, 2023
Copy link
Contributor

@celdrake celdrake left a comment

Choose a reason for hiding this comment

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

The change looks good and the tests changes too.

However, I have noticed that the function createHostInventoryWithDiskHolders does not really create fixtures with the expected master/workerDisk. It overwrites the disks with fixed data defined as disksWithHolders.

I suggest to fix it, perhaps by changing the function createHostInventoryWithDiskHolders (removing the declared diskSpace param, and send a dummy diskSpace value (eg. 0) to createHostInventory.). Or make diskHolders dynamic and use the received diskSpace.

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 7, 2023
@jgyselov
Copy link
Collaborator Author

jgyselov commented Aug 7, 2023

However, I have noticed that the function createHostInventoryWithDiskHolders does not really create fixtures with the expected master/workerDisk. It overwrites the disks with fixed data defined as disksWithHolders.

I suggest to fix it, perhaps by changing the function createHostInventoryWithDiskHolders (removing the declared diskSpace param, and send a dummy diskSpace value (eg. 0) to createHostInventory.). Or make diskHolders dynamic and use the received diskSpace.

I decided to remove the diskSpace parameter. I don't have the best understanding of how exactly the host inventory data works, I'm just using mock data provided here for the disks with holders.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2023
@openshift-ci
Copy link

openshift-ci bot commented Aug 24, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ammont82, jgyselov, jkilzi

The full list of commands accepted by this bot can be found 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

@jkilzi jkilzi merged commit 79e4cef into openshift-assisted:master Aug 24, 2023
7 checks passed
jgyselov added a commit to jgyselov/assisted-installer-ui that referenced this pull request Aug 24, 2023
…ft-assisted#2286)

* Do not include disks with holders in the total storage size calculations

* Total disk storage calculation tests

* Simplify storage host inventory fixtures

---------

Co-authored-by: Jonathan Kilzi <[email protected]>
rawagner pushed a commit to rawagner/facet-lib that referenced this pull request Sep 13, 2023
…ft-assisted#2286)

* Do not include disks with holders in the total storage size calculations

* Total disk storage calculation tests

* Simplify storage host inventory fixtures

---------

Co-authored-by: Jonathan Kilzi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. OCM size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants