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
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions examples/convert_disk.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
#!/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

#
# Copyright (c) 2022 Red Hat, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

"""
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

"""

import time

from contextlib import closing

import ovirtsdk4.types as types

from helpers import common


def parse_args():
parser = common.ArgumentParser(description="Download disk snapshot")

parser.add_argument("disk_id", help="The disk's id")

parser.add_argument(
"-f", "--format",
choices=("raw", "qcow2"),
help=("The format the disk will converted to (qcow2, raw"))

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.

help=("If true, the disk will be converted to a sparse disk, "
"Otherwise to a preallocated disk")
)

return parser.parse_args()


args = parse_args()
common.configure_logging(args)

connection = common.create_connection(args)
with closing(connection):
# Get the reference to the disks service:
disks_service = connection.system_service().disks_service()

# Find the disk we want to convert by id
disk_service = disks_service.disk_service(args.disk_id)

# 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_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)

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.

)

disk_service = disks_service.disk_service(args.disk_id)

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

break