Engine does not need to send initial size in this case. It should send the disk size.
Vdsm should allocate 128m lv for this volume, but I'm not sure we tested allocation
of such tiny disks.

Avihai tells me that we the smallest disk tests is 1g. I think this should be easy
to fix in vdsm so you can move the bug to vdsm.

Right, maybe logic in that calculate_volume_alloc_size() is enough.

Moving to VDSM, thank you.

We don't really understand why to use such small size disks but in case they are needed it would be better to use file storage or, if all disks are that small, to change the chunk size

Changing the chunk size to tiny size (e.g. 128 MiB) is very bad and
will cause VMs to pause quickly even with recent improvements in the
mailbox.

This should be a trivial fix and a good first issue.

Nir, would you mind explaning the solution that will be done here? Are you going to allocate 128M for anything thats below 128MB?

Asking so I can move on with BZ2094577 to stop flagging about these small volumes.

Thanks!

(In reply to Nir Soffer from comment #5)
> Changing the chunk size to tiny size (e.g. 128 MiB) is very bad and
> will cause VMs to pause quickly even with recent improvements in the
> mailbox.

Right but note the "if all disks are that small" part that came before that - IIRC the installer of OCP on RHV was changed to create preallocated disks for the VMs so if all the thin provisioned disks are those small PVs..

Anyway, we had a similar issue with the metadata volumes (for hibernated vms or snapshots with memory) and I don't remember all the details of what was discussed back then but eventually we ended up setting them with raw+preallocated, wouldn't it make sense to do the same for those disks?

(In reply to Arik from comment #7)
> (In reply to Nir Soffer from comment #5)
> > Changing the chunk size to tiny size (e.g. 128 MiB) is very bad and
> > will cause VMs to pause quickly even with recent improvements in the
> > mailbox.
> 
> Right but note the "if all disks are that small" part that came before that
> - IIRC the installer of OCP on RHV was changed to create preallocated disks
> for the VMs so if all the thin provisioned disks are those small PVs..
> 
> Anyway, we had a similar issue with the metadata volumes (for hibernated vms
> or snapshots with memory) and I don't remember all the details of what was
> discussed back then but eventually we ended up setting them with
> raw+preallocated, wouldn't it make sense to do the same for those disks?

For metadata disks we wrongly used qcow2 format and then corrupted the disk
by writing over the qcow2 header.

In this case the user asked to create qcow2 disk with small virtual size
(10m).

disk = disks_service.add(
    types.Disk(
        name='mydisk',
        description='My disk',
        format=types.DiskFormat.COW,
        provisioned_size=10 * 2**20,
        storage_domains=[
            types.StorageDomain(
                name='mydata'
            )
        ]
    )
)

Vdsm ignored the requested virtual size and allocated one chunk
(2.5g) instead of one extent (128m). This is a trivial error in
the code computing the logical volume size for a new disk.

(In reply to Germano Veit Michel from comment #6)
> Nir, would you mind explaning the solution that will be done here? Are you
> going to allocate 128M for anything thats below 128MB?

Yes, with block storage we cannot allocate less than 128 MiB. If you create
10 MiB disk you will get a 128 MiB logical volume with 10 MiB qcow2 image.

(In reply to Nir Soffer from comment #8)
> In this case the user asked to create qcow2 disk with small virtual size
> (10m).
> 
> disk = disks_service.add(
>     types.Disk(
>         name='mydisk',
>         description='My disk',
>         format=types.DiskFormat.COW,
>         provisioned_size=10 * 2**20,
>         storage_domains=[
>             types.StorageDomain(
>                 name='mydata'
>             )
>         ]
>     )
> )
> 
> Vdsm ignored the requested virtual size and allocated one chunk
> (2.5g) instead of one extent (128m). This is a trivial error in
> the code computing the logical volume size for a new disk.

Yeah, but in this case it's not really the user that asks that, it's the OCP client that determines the format I suppose
Any reason to have them thin-provisioned? from my point of view, converting such small volumes to RAW+preallocated is also ok if it's just for OCP on RHV and they cannot change it at the moment..

(In reply to Arik from comment #10)
> (In reply to Nir Soffer from comment #8)
> > In this case the user asked to create qcow2 disk with small virtual size
> > (10m).
> > 
> > disk = disks_service.add(
> >     types.Disk(
> >         name='mydisk',
> >         description='My disk',
> >         format=types.DiskFormat.COW,
> >         provisioned_size=10 * 2**20,
> >         storage_domains=[
> >             types.StorageDomain(
> >                 name='mydata'
> >             )
> >         ]
> >     )
> > )
> > 
> > Vdsm ignored the requested virtual size and allocated one chunk
> > (2.5g) instead of one extent (128m). This is a trivial error in
> > the code computing the logical volume size for a new disk.
> 
> Yeah, but in this case it's not really the user that asks that, it's the OCP
> client that determines the format I suppose
> Any reason to have them thin-provisioned? from my point of view, converting
> such small volumes to RAW+preallocated is also ok if it's just for OCP on
> RHV and they cannot change it at the moment..

I don't have any idea why they create small qcow2 disks, but it is clear that
vdsm behavior is wrong.

Germanno, do you know why these tiny disks are created? I don't see real use
case for 10 MiB disk except testing.

(In reply to Nir Soffer from comment #11)
> Germanno, do you know why these tiny disks are created? I don't see real use
> case for 10 MiB disk except testing.

This would come from "apps" running on OCP. I'm not aware of a PVC required by the
default cluster components that would need such a small storage.

It depends on what is the app running and what are its needs, its not in our hands
as one can decide to run anything in OCP. With microservices and friends, it may be
expected?

Customer has just one of these on his entire OCP cluster, everything else is 1G+ or 2.5G+.

(In reply to Germano Veit Michel from comment #12)
> Customer has just one of these on his entire OCP cluster, everything else is
> 1G+ or 2.5G+.

OK, that makes more sense
So I'm not sure about the cost-effectiveness of dealing with that, I see the point of fixing incorrect behavior of VDSM but this change would also require a change on the engine side and that scenario should really not be that common. Nir mentioned possible corruption with metadata volumes in comment 8, which I may have not been involved in its discussions, but even without the corruption we chose not to mess with such small volumes when we got reports of allocating 1G for them on block storage. I don't think this bug really justifies to reconsider that at this point..

Arik,

I'm not sure if I'm missing something, are you saying this should be WONTFIX? 
What on engine side needs to be fixed here? Isn't it just stop using volume_utilization_chunk_mb as initial size in calculate_volume_alloc_size?

But indeed, this is not a severe bug, and I can make the discrepancy tool ignore wasted storage space under 2.5G to not be triggered by these. It's just not very correct...

(In reply to Nir Soffer from comment #2)
> Engine does not need to send initial size in this case. It should send the
> disk size.

OK, I misinterpreted this part if "disk size" refers to the virtual size of the disk, I believe that is sent already and then we don't need to change the engine side
but even then, what's the point of defining thin-provisioned (sparse) disks in this size? we don't want the mailbox mechanism to come into play and monitor those volumes, right? I'm not suggest to close it as WONTFIX, I suggest to apply the same approach that was applied for metadata volumes (regardless of the reason - whether it was to prevent corrupted as Nir wrote or not to allocate the previous chunk size of 1G) which is to create them as preallocated volumes

(In reply to Arik from comment #15) 
> but even then, what's the point of defining thin-provisioned (sparse) disks
> in this size? we don't want the mailbox mechanism to come into play and
> monitor those volumes, right?
True, a tiny sparse disk saves nothing. But it could be extended later (who knows),
a client can do whatever it wants - and RHV should do whatever its being told via
API.

We need to test what the latest version of the oVirt CSI driver is doing, there
might be another bug there.

> I'm not suggest to close it as WONTFIX, I
> suggest to apply the same approach that was applied for metadata volumes
> (regardless of the reason - whether it was to prevent corrupted as Nir wrote
> or not to allocate the previous chunk size of 1G) which is to create them as
> preallocated volumes

You mean to automatically switch thin to prealloc if the size is smaller than X,
regardless of what the client requests? What if its extended later? These are
not system disks and the user/client can in theory do anything with it.

And blocking it may break existing things...

Right, unlike the metadata volumes these volumes can be extended but that's ok, preallocated volumes can be extended. If you mean that they can be extended up to some size for which it would make sense to use thin provisioning - I'm not sure

what's the plan?
it doesn't seem to be worth addressing if it's a single PV in the whole setup, i.e. not frequent.

(In reply to Michal Skrivanek from comment #18)
> it doesn't seem to be worth addressing if it's a single PV in the whole
> setup, i.e. not frequent.

yeah, and Germano changed rhv-image-discrepancies (bz 2094577) so that would be less visible, but it really sounds like fairly easy fix and if with that we can improve the experience of ocp on rhv also in cases of having many workloads that ask for such small volumes then I think it is worth giving a try to what Nir suggested above

Due to QE capacity, we are not going to cover this issue in our automation

This bug has low overall severity and is not going to be further verified by QE. If you believe special care is required, feel free to properly align relevant severity, flags and keywords to raise PM_Score or use one of the Bumps ('PrioBumpField', 'PrioBumpGSS', 'PrioBumpPM', 'PrioBumpQA') in Keywords to raise it's PM_Score above verification threashold (1000).

This bug has low overall severity and passed an automated regression suite, and is not going to be further verified by QE. If you believe special care is required, feel free to re-open to ON_QA status.