Add virtual columns PersistentVolume#storage_capacity and PersistentVolumeClaim#storage_capacity

[yrudman]

There is PersistentVolume.capacity column, defined as serialize :capacity, Hash and one of entry in that hash could be :storage =>....

This PR: Virtual column storage added to PersistentVolume model for the purpose of showing on report

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

Example of report based on PersistentVolume model with new column Storage:

@miq-bot add-label reporting

\cc @gtanzillo

[yrudman]

@miq-bot assign @gtanzillo

[yrudman]

@zakiva @nimrodshn could you review
Do you know what other possible entries could be in capacity Hash ?

[nimrodshn]

@yrudman LGTM.

[nimrodshn]

@yrudman From what I understand the PersistentVolume capacity hash has a single key/value entry - under storage key:

Generally, a PV will have a specific storage capacity. This is set using the PV’s capacity attribute. (...)
Currently, storage size is the only resource that can be set or requested. Future attributes may include IOPS, throughput, etc.

see: here for more details in the Kubernetes docs and here for the kube client code.

[yrudman]

@nimrodshn thank you

[Fryguy]

I'm ok with exposing the serialized column as a virtual column but I'm not for the storage naming as it's confusing with respect to the storages table. That is someone could read this as a method that would get a record from the storage table as opposed to the storage_capacity. I'm thinking storage_size or storage_capacity as the method name instead. @agrare Thoughts?

[Fryguy]

From what I understand the PersistentVolume capacity hash has a single key/value entry - under storage key:

If that's the case why are we storing a hash? Let's just make a column with the value and then you don't need a virtual column. @cben do you have any more insight here?

[agrare]

@Fryguy yeah I thought we agreed we were going to add a new column to store just the integer size and use the parser to normalize the value from the hash instead of just stuffing the whole hash in the column.

[cben]

I suppose the original intent was future-proofing due to k8s doc saying (emphasis mine):

Currently, storage size is the only resource that can be set or requested. Future attributes may include IOPS, throughput, etc.

There is also conceptual symmetry to PersistentVolumeClaim claims and requests attributes — also serialed hashes, also currently contain only :storage.

In principle, keeping these as hashes is more elegant. Imagine storage_request, storage_limit, storage_capacity, then future iops_request, iops_limit, iops_capacity etc. explosion...
In practice, no new capacity fields have appeared in a long time, and we'd need the names explosion anyway, even if just for virtual column names. I won't object to schema change if you want it.

• In any case, I agree about naming, it should be storage_capacity not just storage.
• BZ only talks of PersistentVolume but consider also covering PersistentVolumeClaim.

[jrafanie]

In any case, I agree about naming, it should be storage_capacity not just storage

1. I agree with @cben and @Fryguy, it's confusing to be called storage but show storage capacity in the report.

2. I agree with @agrare that we should probably collapse the hash into separate columns as @cben enumerated above.

We can do 2) after 1) is fixed and merged.

[yrudman]

@jrafanie done the same to PersistentVolumeClaim as @cben suggested

[jrafanie]

@yrudman @cben a few questions:

1. Is capacity a serialized hash? If so, it should be never be nil (no need for if capacity)
2. Do we want this method to return nils? Should it be zero instead?

[jrafanie]

I'm wondering if capacity.fetch(:storage, 0) is enough or if we want nils

[yrudman]

I think nil should mean not set, and 0 - was set to 0

[cben]

Semantically there is difference between 0 and unset capacity.
PVs probably nearly always (?) have a capacity, though there are too many types to be sure...
PVCs don't have capacity when not bound to a PV. That's not the same as being bound to a 0-size PV (which would be a pretty silly volume).

[miq-bot]

Checked commits yrudman/manageiq@3083fcf~...d922fe1 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

[jrafanie]

I think nil should mean not set, and 0 - was set to 0

@yrudman yeah, I guess I'm not sure if all callers are expecting non-nil values.

[yrudman]

@jrafanie looks as empty place on report (the only possible caller at this time):