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

add example for disk conversion #57

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bennyz
Copy link
Member

@bennyz bennyz commented Feb 24, 2022

depends on oVirt/ovirt-engine#51
and new sdk release

@@ -0,0 +1,82 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

python3

Copy link
Member Author

Choose a reason for hiding this comment

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

ack


"""
Show how to download disk snapshots using imageio client.
Requires the ovirt-imageio-client package.
Copy link
Member

Choose a reason for hiding this comment

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

Need to replace with relevant text

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

# Start the conversion to raw/preallocated
if args.format == 'raw':
disk_format = types.DiskFormat.RAW
elif args.format == 'cow':
Copy link
Member

Choose a reason for hiding this comment

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

cow -> qcow2

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

disk_service.convert(
disk=types.Disk(
format=disk_format,
sparse=args.sparse)
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if user choose:

format="raw", sparse=True

On block storage, or:

format="cow", sparse=False

On any storage?

Copy link
Member Author

@bennyz bennyz Feb 24, 2022

Choose a reason for hiding this comment

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

It fails on engine's validation:

ovirtsdk4.Error: Fault reason is "Operation Failed". Fault detail is "[Cannot convert disk Virtual Disk. Disk configuration (COW Preallocated backup-None) is incompatible with the storage domain type.]". HTTP response code is 409.

Copy link
Member

Choose a reason for hiding this comment

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

We need to make cow-prellocated possible, this is required for incremental backup.

while True:
time.sleep(1)
disk = disk_service.get()
if disk.status == types.DiskStatus.OK:
Copy link
Member

Choose a reason for hiding this comment

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

Waiting for disk status is usually broken because when change the status before the
disk is unlocked. Do we have better way to wait? use jobs?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an async operation, but the memory lock is released before before the status change so we shouldn't have that race condition

disk_format = types.DiskFormat.RAW
elif args.format == 'cow':
disk_format = types.DiskFormat.COW
disk_service.convert(
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to account for the case where format isn't passed

Copy link
Member

@nirs nirs Feb 24, 2022

Choose a reason for hiding this comment

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

You can make both format and sparse required=True, but do we want to support convert
from raw-sparse to raw-preallocated? seems resonable to me.

So the rule can be, specify either format or sparse or both.

What about validation with current disk properties? For example:

disk format: raw
disk sparse: true
format: raw
sparse: true

Or the case when the disk does not exists.

I guess this validation is on the backend, but I think good application will validate
this on the client side to avoid sending pointless requests.

So I think the flow should be:

get the disk
fail if the disk does not exist
validate the parameters with the disk
fail if we have nothing to do, or the requested parameters cannot be used.
convert the disk
wait until conversion completes

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, the parameters are optional (only one is required)


parser.add_argument(
"--sparse",
type=bool,
Copy link
Member Author

Choose a reason for hiding this comment

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

this can't actually be a bool, if it's not passed we don't want to change the allocation policy

Copy link
Member

Choose a reason for hiding this comment

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

The way to add bool arguments is:

parser.add_argument("--sparse", action="store_true")

If not set, args.sparse will be None. We can use this for
keep the current format.

Same for the format - if the user does not set the foramt,
we want to keep the current format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants