-
Notifications
You must be signed in to change notification settings - Fork 83
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
wrapanapi/systems/rhevm.py
Outdated
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe name="" ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
wrapanapi/systems/rhevm.py
Outdated
storage_domains=[types.StorageDomain(name=storage_domain)]), | ||
interface=getattr(types.DiskInterface, interface), | ||
storage_domains=[types.StorageDomain(name=storage_domain)], | ||
sparse=bool(sparse)), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one comment 🎉
wrapanapi/systems/rhevm.py
Outdated
@@ -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()), |
There was a problem hiding this comment.
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.
0bd1bcf
to
30bd585
Compare
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.