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

[1LP][WIPTEST] Update rhevm template kwargs, handle disk attachments #389

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

Conversation

mshriver
Copy link
Collaborator

draft state, reader beware

add_disk kwargs for name/format/sparse updated

disk_attachments handling for template deploy. needs some consideration of the tradeoffs, and whether the kwarg should contain the attachments that should exist - or just modifications to attachments on the template that match by name.

Copy link
Contributor

@izapolsk izapolsk left a comment

Choose a reason for hiding this comment

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

Looks good to me

def add_disk(self, storage_domain=None, size=None, interface='VIRTIO', format=None,
active=True):
def add_disk(self, storage_domain=None, size=None, interface='virtio', format='cow',
active=True, sparse=True, name=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe name="" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This default is coming from ovirt's defaults for types.Disk instantiation. Leaving it as None to allow rhv to name the disk, haven't tested what will happen if we explicitly pass empty string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I split this into #390 if you want to continue discussion.

storage_domains=[types.StorageDomain(name=storage_domain)]),
interface=getattr(types.DiskInterface, interface),
storage_domains=[types.StorageDomain(name=storage_domain)],
sparse=bool(sparse)),
Copy link
Contributor

Choose a reason for hiding this comment

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

it should already be bool

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it should, but there's no type validation otherwise. I can remove though and just pass through what we get.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that type validation. It will throw error right there, before performing operations with incorrect values.

@izapolsk izapolsk changed the title [WIPTEST] Update rhevm template kwargs, handle disk attachments [1LP][WIPTEST] Update rhevm template kwargs, handle disk attachments Jun 11, 2019
disk_attachments (optional) -- list of dicts defining (name (partial), format, sparse)
name of template disks will be partial matched
format defaults to COW
sparse defaults to True (thin provisioning)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@kedark3 kedark3 left a comment

Choose a reason for hiding this comment

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

Only one comment 🎉

@@ -451,10 +453,12 @@ def add_disk(self, storage_domain=None, size=None, interface='VIRTIO', format=No
"""
disk_attachments_service = self.api.disk_attachments_service()
disk_attach = types.DiskAttachment(
disk=types.Disk(format=types.DiskFormat(format),
disk=types.Disk(name=str(name),
format=types.DiskFormat(format.lower()),
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when size or storage_domain is passed None?

draft state

add_disk kwargs for name/format/sparse updated

disk_attachments handling for template deploy. needs some consideration of the tradeoffs, and whether the kwarg should contain the attachments that should exist - or just modifications to attachments on the template that match by name.
@ogajduse ogajduse marked this pull request as draft October 11, 2023 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants