Fix rendered disk size <1Gi #452

mareklibra · 2019-05-15T08:42:17Z

If disk size < 1 Gi in the Storage Tab of CreateVmWizard,
the value is reformatted and appended by a lower matching unit.

Corresponding PVC is created with a value conforming the format
required by Kubernetes.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1708195

mareklibra · 2019-05-15T08:43:36Z

If disk size is < 1 Gi in the Storage Tab of CreateVmWizard, the value is reformatted and appended by a lower matching unit. Corresponding PVC is created with a value conforming the format required by Kubernetes.

coveralls · 2019-05-15T08:53:07Z

Pull Request Test Coverage Report for Build 1858

31 of 37 (83.78%) changed or added relevant lines in 8 files are covered.
15 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.3%) to 84.001%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/Wizard/CreateVmWizard/StorageTab.js	5	7	71.43%
src/k8s/requests/v2v/importVmware.js	3	5	60.0%
src/utils/utils.js	1	3	33.33%

Files with Coverage Reduction	New Missed Lines	%
src/k8s/requests/v2v/startV2VvmwareController.js	1	13.33%
src/components/Form/FormFactory.js	2	92.98%
src/utils/status/vm/vmStatus.js	2	92.37%
src/components/VmStatus/VmStatus.js	3	92.21%
src/selectors/prometheus/storage.js	3	58.82%
src/components/StorageOverview/TopConsumers/TopConsumers.js	4	88.68%

Totals
Change from base Build 1583:	0.3%
Covered Lines:	3962
Relevant Lines:	4519

💛 - Coveralls

mareklibra · 2019-05-15T10:47:45Z

@andybraren , any idea how to better present disk sizes of different units?

In most cases, the size in GiB is ok.
I think it's good to keep the user requesting disks per GiBs.

Anyway, in v2v flow, the disks can be smaller, like 1 KiB.
It's a rare case but should be handled.

atiratree · 2019-05-15T10:41:47Z

src/components/Wizard/CreateVmWizard/StorageTab.js

@@ -428,6 +430,7 @@ export class StorageTab extends React.Component {
              type: POSITIVE_NUMBER,
            }
          : null,
+      readValueFormatter: value => (value < 1 ? getValidK8SSize(value, this.props.units, 'Gi', false) : value),


it probably makes more sense to have this inside the renderConfig

This will be inconstent for ediable fields (showing one size and seeing something different when editing the filed).

the inconsistencies could be resolved for now if we allow this only for STORAGE_TYPE_EXTERNAL_V2V_TEMP, STORAGE_TYPE_EXTERNAL_V2V_VDDK types and explicitely say it should not be editable in the comment

For v2v, it's not editable already.

For other flows, the user is not allowed to enter floating point number to disk size, so effectively the formatting is used in the single relevant case only.

If something, we should handle just the STORAGE_TYPE_EXTERNAL_IMPORT case, not the two mentioned.
Recently, component edit mode is disabled when renderConfig is missing. Changing that semantic would lead to further refactoring which I do not think is needed.

IMO, sort of functional readValueFormatter is good enough. In a follow-up (out of bug-fixing scope) we can merge it with the static addendum if really needed.

For v2v, it's not editable already

I know, but it could be confusing for someone who looks at this in the future

For other flows, the user is not allowed to enter floating point number to disk size, so effectively the formatting is used in the single relevant case only.

The user can have have templates with custom sizes, that would break this.

If something, we should handle just the STORAGE_TYPE_EXTERNAL_IMPORT case, not the two mentioned.
Recently, component edit mode is disabled when renderConfig is missing. Changing that semantic would lead to further refactoring which I do not think is needed.

IMO, sort of functional readValueFormatter is good enough. In a follow-up (out of bug-fixing scope) we can merge it with the static addendum if really needed.

You are right - I forgot about that one.

So it probably should be (STORAGE_TYPE_EXTERNAL_V2V_TEMP, STORAGE_TYPE_EXTERNAL_IMPORT), because there is fixed size for both of them.

We could switch the behaviour in readValueFormatter if one of these two types is detected:

Or rename renderConfig to renderEditConfig and create renderValueConfig for more generic approach. This could be a TODO.

Considering the design suggestion bellow and what is stated here, I have added renderValueConfig, so far containing just single formatter optional prop.

Semantics of the renderConfig() is unchanged but renamed to renderEditConfig().

Size-cells of listed storage types contain always unit.

There's a gap in edit-mode - the unit is not rendered. But the user can get this information within the read-mode. Will be improved when moved to PF4 tables.

atiratree · 2019-05-15T10:50:22Z

src/k8s/objects/pvc/pvc.js

@@ -12,7 +13,7 @@ export const buildPvc = ({ generateName, namespace, storageType, size, storageCl
    volumeMode: 'Filesystem',
    resources: {
      requests: {
-        storage: `${size}Gi`,
+        storage: getValidK8SSize(size, units, 'Gi', true),


this is overly complicated IMO. I think we should pass the size here with the unit already or pass just the bytes.

We can't hardcode to Gi here since i.e. size equal to 1KiB does not match the required pattern.

We can either convert to a reasonable unit or use bytes in all cases.

As I already use this conversion for rendering, I think it's nice to keep it here as well. Just for better readability and debugging of the result. But I do not strongly insist on keeping it, just like it.

src/k8s/objects/pvc/pvc.js should be pretty generic. I would rather send size and unit from above to here.

And would rather include the conversion here in src/k8s/requests/v2v/importVmware.js:

const extImportPvcsPromises = storages.filter(isImportStorage).map(storage => k8sCreate( PersistentVolumeClaimModel, buildPvc({ ...storage, generateName: storage.name, name: undefined, namespace, units, }) ) );

where it is clear because of the import storage type

ok, moved one layer up

atiratree · 2019-05-15T10:58:50Z

src/components/Table/EditableDraggableTable.js

+    if (!editable && readValueFormatter) {
+      result = readValueFormatter(result);
+    }
+


I would rather implement this with addendum approach or combine these approaches altogether (addendum with functional approach). So we don't have two approaches for the similar thing.

Right, it's very similar, the formatter can just do more things than just appending a string.

For flexibility, I am more for the functional approach but refactoring of the addendum is not feasible for the 2.0.0 scope. Can be done later.

If implemented using static addendum, the logic would need to be moved to a higher level while affecting other flows and so potentially introducing new bugs. So I decided to go this safer way.

One more idea: we can keep using addendum as it is and optionally assigning a function to it (instead of recent string). The EdditableDraggableTable would check it's type and act accordingly. But such reusing of single property for two different types introduces more confusion, imo.

Ok, we can leave this for later. Can you please create an issue to unify these two approaches?

Yes better to rewrite it without any magic and use just the functional approach.

atiratree · 2019-05-15T11:02:40Z

src/k8s/request.js

+        k8sCreate,
+        k8sPatch,
+      },
+      units


can we pass this as a object for better enhancement. { units } ?

I guess we should have begin with the object parameters to begin with (maybe TODO for refactoring?).

Only this have to be left the same (because of future possibility of passing EnhancedK8sMethods):

{ k8sCreate, k8sPatch, }

Good idea, will do it.

atiratree · 2019-05-15T11:04:22Z

src/k8s/requests/v2v/startV2VvmwareController.js

@@ -38,6 +38,8 @@ export const startV2VVMWareController = async ({ namespace }, { k8sGet, k8sCreat
        : 'V2V VMWare controller deployment not found, so creating one ...'
    );

+    // TODO: do not fail if i.e. ServiceAccount already exists
+    // TODO: notify user if deployment fails


sidenote: this should be pretty easy to do with the new VM dialog refactoring

Good to know.
I think its a "nice to have" even for 2.0.0 which will not contain the refactoring.

Anyway, it's of lower priority since there is a workaround possible (by deleting role, rolebinding and the service account).

andybraren · 2019-05-15T14:37:26Z

@mareklibra Here's a rough mockup of one idea that attaches a unit dropdown to the text field. The dropdown would default to Gi, but could be changed to Ki or Ti. The Size column title would no longer have (Gi). FYI @matthewcarleton

This combined field is used when creating a PVC in the Console - maybe some of that code could be reused? Here's an old screenshot I took from OpenShift Console #1209 (ignore the incorrect units) that shows the combined field in action:

atiratree · 2019-05-15T15:11:29Z

@mareklibra Here's a rough mockup of one idea that attaches a unit dropdown to the text field. The dropdown would default to Gi, but could be changed to Ki or Ti. The Size column title would no longer have (Gi).

We are planning to do something like this, but this will be much easier once we convert to pf4 tables - so I think it is postponed until then.

This combined field is used when creating a PVC in the Console - maybe some of that code could be reused? Here's an old screenshot I took from OpenShift Console #1209 (ignore the incorrect units) that shows the combined field in action:

this piece is not really reusable currently - we can work on that once we get merged into openshift/console IMO

andybraren · 2019-05-15T16:31:20Z

Hmm, I guess a short-term solution would be to:

Remove (Gi) from the Size column header
Use Ki within the cell when necessary for v2v
Put Gi to the right of the text input for editable disks, and the string becomes 10 Gi when not in edit mode

Would this work for now?

atiratree · 2019-05-15T18:08:54Z

Would this work for now?

+1 this sounds reasonable

mareklibra · 2019-05-16T06:31:55Z

Would this work for now?

Yes, thanks

To be consistent with the newly introduced `rederValueConfig()`

Let's keep using "Gi" as the base unit. In a later version, we will implement proper unit-selector.

kubevirt-bot added the size/M label May 15, 2019

mareklibra changed the title ~~Fix rendered disk size <1Gi~~ WIP Fix rendered disk size <1Gi May 15, 2019

kubevirt-bot added the do-not-merge/work-in-progress label May 15, 2019

Fix rendered disk size <1Gi

5c5dc94

If disk size is < 1 Gi in the Storage Tab of CreateVmWizard, the value is reformatted and appended by a lower matching unit. Corresponding PVC is created with a value conforming the format required by Kubernetes.

mareklibra force-pushed the v2v.bz1708195.diskSize branch from 781ccc1 to 5c5dc94 Compare May 15, 2019 08:48

mareklibra changed the title ~~WIP Fix rendered disk size <1Gi~~ Fix rendered disk size <1Gi May 15, 2019

kubevirt-bot removed the do-not-merge/work-in-progress label May 15, 2019

atiratree suggested changes May 15, 2019

View reviewed changes

Wrap "units" to a "context" object in importVmwareVm()

bfb5eac

mareklibra force-pushed the v2v.bz1708195.diskSize branch from 613964b to bfb5eac Compare May 15, 2019 13:46

mareklibra added 2 commits May 16, 2019 09:47

Introduce renderValueConfig()

860deaa

Rename renderConfig() to renderEditConfig()

193a446

To be consistent with the newly introduced `rederValueConfig()`

kubevirt-bot added size/L and removed size/M labels May 16, 2019

Keep using Gi for non-imported disks

1028c82

Let's keep using "Gi" as the base unit. In a later version, we will implement proper unit-selector.

atiratree approved these changes May 16, 2019

View reviewed changes

atiratree merged commit 40fb63e into kubevirt:master May 16, 2019

mareklibra mentioned this pull request May 16, 2019

Backport: Fix rendered disk size <1Gi #457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rendered disk size <1Gi #452

Fix rendered disk size <1Gi #452

mareklibra commented May 15, 2019 •

edited

Loading

mareklibra commented May 15, 2019

coveralls commented May 15, 2019 •

edited

Loading

mareklibra commented May 15, 2019 •

edited

Loading

atiratree May 15, 2019

atiratree May 15, 2019

mareklibra May 15, 2019

atiratree May 15, 2019

mareklibra May 16, 2019

atiratree May 15, 2019

mareklibra May 15, 2019

atiratree May 15, 2019 •

edited

Loading

mareklibra May 16, 2019

atiratree May 15, 2019

mareklibra May 15, 2019

mareklibra May 15, 2019

atiratree May 15, 2019

atiratree May 15, 2019

mareklibra May 15, 2019

mareklibra May 15, 2019

atiratree May 15, 2019

mareklibra May 15, 2019

andybraren commented May 15, 2019

atiratree commented May 15, 2019

andybraren commented May 15, 2019

atiratree commented May 15, 2019

mareklibra commented May 16, 2019

Fix rendered disk size <1Gi #452

Fix rendered disk size <1Gi #452

Conversation

mareklibra commented May 15, 2019 • edited Loading

mareklibra commented May 15, 2019

coveralls commented May 15, 2019 • edited Loading

Pull Request Test Coverage Report for Build 1858

💛 - Coveralls

mareklibra commented May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andybraren commented May 15, 2019

atiratree commented May 15, 2019

andybraren commented May 15, 2019

atiratree commented May 15, 2019

mareklibra commented May 16, 2019

mareklibra commented May 15, 2019 •

edited

Loading

coveralls commented May 15, 2019 •

edited

Loading

mareklibra commented May 15, 2019 •

edited

Loading

atiratree May 15, 2019 •

edited

Loading