Add support for disk.driver hardware requirement
#2667
Conversation
skycastlelily
requested review from
psss,
lukaszachy,
happz,
thrix and
janhavlin
as code owners
|
fix #2546 |
|
@skycastlelily could you look at the comments so we can make it into this release pls? |
skycastlelily
force-pushed
the
bootdisk
branch
from
cf1a201
to
6d96d5e
Compare
Thanks for your review,updated accordingly:) |
Sure, talked with @happz in the artemis ticket and implemented accordingly. |
|
I don't think it makes much sense to have testcloud deliver it,but I may be
wrong,feel free to talk with the testcloud maintainer about that:)
…On Wed, Feb 21, 2024 at 7:18 PM Miloš Prchlík ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec/hardware/bootdisk.fmf
<#2667 (comment)>:
> @@ -0,0 +1,36 @@
+summary: |
+ Select a guest with a specific boot-disk.
"Select or create ..." - with OpenStack, bootdisk can affect the VM
creation, maybe testcloud would be able to deliver as well.
—
Reply to this email directly, view it on GitHub
<#2667 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23GHS5P3OYO2IPJRWVDYUXJYTAVCNFSM6AAAAABCY5KUNWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJSHA4DENRQGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
If I put driver: virtio_blk into my plan, I might get an OpenStack VM.
As you can see from the link I sent in the artemis ticket, root-device
property won't accept a driver param.To be specific, the params supported
by beaker's BOOTDISK and openstack's root-device are totally
different, though I also do feel they are describing a same thing.
https://docs.openstack.org/ironic/queens/install/advanced.html#specifying-the-disk-for-deployment-root-device-hints
based on the existing flavor, as well as a KVM VM from Beaker.
AFAIK, most beaker users tends to use "hypervisor = kvm" to choose a kvm
vm, and BOOTDISK is for those users who care about some specific hardware.
I do feel I have wasted our time by offering implement openstack's
root-device together with beaker's bootdisk😅
Let's ignore the implementation in various drivers for now, and focus on
clean, correct, and easy-to-understand specifications
You mean I only need to change the bootdisk.fmf, right?
I think we can handle openstack's root-device after I extend disk.fmf or
implement device.fmf,which would be commonly used,and BOOTDISK is only for
a small group of users.
…On Wed, Feb 21, 2024 at 11:24 PM Miloš Prchlík ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec/hardware/bootdisk.fmf
<#2667 (comment)>:
> + Select or create a guest with a specific boot-disk.
+
+description: |
+ .. code-block::
+ # A list of string, each describing boot disk of beaker guest or root device of openstack.
+ for beaker:
+ boot-disk:
+ - "A Well-Known disk driver/port type"
+
+ for openstack,boot-disk will accept all string params and pass them to root device property.
+ boot-disk:
+ - "device vendor"
+
+ boot-disk:
+ - "device vendor"
+ - "wwn"
Make sense to me,but it will be a little complicated to implement,
A little complicated + correct is fine. I would be worried if it would be
terribly complicated, nearly impossible ;)
boot-disk:
- "wwn: 0x4000cca77fc4dba"
- "vendor: well-known device manufacturer"
Yeah, this is exactly what I want to avoid: this value would be completely
useless if I switch from OpenStack plugin (or artemis) to beaker plugin.
The semantics changes based on who's looking at the value, and that's not
good. Plus, encoding structure into a string leads to values that are
terribly hard to maintain, change, and validate. Hence the structure I
proposed above, let's make it explicit. beaker driver can always emit a
warning when seeing wwn key and say "unsupported constraint" - that's
acceptable.
And I was thinking to add code to artemis openstack driver to transfer the
string to dict and pass them to root-device property.
As written above, I would fight string-to-dict conversions with all my
power :)
To implement "The plugin still may say "we do not support boot-disk.wwn",
and that is absolutely acceptable "
we need make wwn/vendor/etc a property of boot-disk for openstack.
boot-disk:
wwn: 0x4000cca77fc4dba
vendor: "well-known device manufacturer"
Now,I do feel maybe it's a bad idea to mix beaker's bootdisk with
openstack 's root-device, ie,maybe we should only handle beaker's bootdisk
in this merge request:(
Good point, maybe we need "root disk" requirement, but I don't think so.
It does sound like the same concept: with Beaker, we look for machines with
a given "boot disk", with OpenStack (and Artemis with OpenStack pool), we
construct a machine with a given "boot disk". We may also look for
OpenStack flavors with a given "boot disk" - machines I checked in our
OpenStack tenants would be described in Beaker as having virtio_blk
bootdisk key. If I put driver: virtio_blk into my plan, I might get an
OpenStack VM, based on the existing flavor, as well as a KVM VM from
Beaker. So from my PoV, "constructing a VM with given boot/root disk" fits
the boot-disk description too. I'd say this aspect is fine.
The issues lie in the specification, I'm afraid, with string-only items of
the boot-disk array and encoding of details into those strings. That
harms the possibility of swapping provision plugins and makes it harder to
work with the resulting plan. I believe we can do better, even if the cost
is a slightly more verbose specification and slightly more complicated
implementation.
And I don't think the implementation will be that complicated, on the
other hand, I'm pretty sure it will be cleaner than working with "vendor:
well-known device manufacturer". Let's ignore the implementation in
various drivers for now, and focus on clean, correct, and
easy-to-understand specifications. In my experience, it often contributes
heavily to getting a cleaner code eventually.
—
Reply to this email directly, view it on GitHub
<#2667 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23F3635V3LPR5MAVGOTYUYGRLAVCNFSM6AAAAABCY5KUNWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJTGUYTONZXGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes, I am aware of that. Yet, both "boot disk" and "root disk" describe very similar concept, I don't see much difference between them. Not enough to have one key for "boot disk" and another for "root disk" when we can have just one.
Yes, I see the overlap too. And I am aware of the difference, but you can look at it as Beaker being a subset of what OpenStack (or AWS and Azure) accept: you may ask OpenStack to give you a VM whose boot disk/root disk requires a particular driver. In this sense, what you propose based on your OpenStack experience can easily cover Beaker as well.
Yes, I would focus on making the specification right for now, without looking into actual implementations in
Ok, then the question is whether we do need # "Give me a disk of this size"
# Historically, we did not care whether it was a boot disk or a root disk. It probably was.
disk:
- size: ">= 20 GB"
# New requirements, trying to pin the `disk` to a particular type of disk as seen by the guest.
# With Artemis, this would be directed to Beaker. tmt's `virtual` might be able to create this
# `mrack` would translate it to Beaker XML, everyone's happy.
disk:
- boot-disk: true
driver: pcieport
size: ">= 20 GB"
# This you can get from Beaker or OpenStack, or any cloud delivering KVM-backed VMs. Again, everyone's happy.
disk:
- boot-disk: true
driver: virtio_blk
size: ">= 20 GB"
# Artemis would send this to OpenStack, based on `wwn` key, `mrack` would tell you `wwn` is unsupported & it would try to at least get you Seagate disk, and `virtual` might try to emulate the Seagate.
disk:
- root-disk: true
wwn: 0x4000cca77fc4dba
vendor: Seagate |
|
Yes, I am aware of that. Yet, both "boot disk" and "root disk" describe
very similar concept, I don't see much difference between them. Not enough
to have one key for "boot disk" and another for "root disk" when we can
have just one
Yes,they are very similar concepts, but in beaker,BOOTDISK is for those
users who want to test a specific driver or port type,as there is no
disk.driver property.
Typically,users can do some translate work,like find all the disk models
which provide,say driver, megaraid_sas,and write plan like below,if we
offer code to handle model-name as an or-list,but I guess we should NOT
handle it like that.
disk:
model-name: model-name-a|model-name-b|....
I was NOT thinking of handling root-disk property by adding
root-disk:true,I was planning to handle it like this: if there is only one
disk specified, then we pass that one to root-device,if there are more than
one disk is specified,then we pass the first one.
But beaker'BOOTDISK doesn't fit disk well,as BOOTDISK'properties( or,more
exactly, property) are supposed to be string,while disk's properties are
supposed to be dict,so we need BOOTDISK merge requests to avoid
complexity: )
I think you professional has already spent way more than enough time on
this tiny merge request,I bet you have lots of more important work to
handle,
at least from my side,we have guest-log(my mrack log merge request has
already been merged), boot.method,and virtualization,etc,how about merge
this ,and let it go?: )
…On Fri, Feb 23, 2024 at 6:01 PM Miloš Prchlík ***@***.***> wrote:
If I put driver: virtio_blk into my plan, I might get an OpenStack VM.
As you can see from the link I sent in the artemis ticket, root-device
property won't accept a driver param.
Yes, I am aware of that. Yet, both "boot disk" and "root disk" describe
very similar concept, I don't see much difference between them. Not enough
to have one key for "boot disk" and another for "root disk" when we can
have just one.
To be specific, the params supported by beaker's BOOTDISK and openstack's
root-device are totally different, though I also do feel they are
describing a same thing.
https://docs.openstack.org/ironic/queens/install/advanced.html#specifying-the-disk-for-deployment-root-device-hints
Yes, I see the overlap too. And I am aware of the difference, but you can
look at it as Beaker being a subset of what OpenStack (or AWS and Azure)
accept: you may ask OpenStack to give you a VM whose boot disk/root disk
requires a particular driver. In this sense, what you propose based on your
OpenStack experience can easily cover Beaker as well.
Let's ignore the implementation in various drivers for now, and focus on
clean, correct, and easy-to-understand specifications
You mean I only need to change the bootdisk.fmf, right?
Yes, I would focus on making the specification right for now, without
looking into actual implementations in mrack.py or Artemis. Let's define
the boot-disk key first.
I think we can handle openstack's root-device after I extend disk.fmf or
implement device.fmf,which would be commonly used,and BOOTDISK is only for
a small group of users.
Ok, then the question is whether we do need boot-disk after all. :) If
you can see a way to define "root disk" requirements under disk, then
maybe we can do the same with "boot disk". For example:
# "Give me a disk of this size"# Historically, we did not care whether it was a boot disk or a root disk. It probably was.disk:
- size: ">= 20 GB"
# New requirements, trying to pin the `disk` to a particular type of disk as seen by the guest.# With Artemis, this would be directed to Beaker. tmt's `virtual` might be able to create this# `mrack` would translate it to Beaker XML, everyone's happy.disk:
- boot-disk: true
driver: pcieport
size: ">= 20 GB"
# This you can get from Beaker or OpenStack, or any cloud delivering KVM-backed VMs. Again, everyone's happy.disk:
- boot-disk: true
driver: virtio_blk
size: ">= 20 GB"
# Artemis would send this to OpenStack, based on `wwn` key, `mrack` would tell you `wwn` is unsupported & it would try to at least get you Seagate disk, and `virtual` might try to emulate the Seagate.disk:
- root-disk: true
wwn: 0x4000cca77fc4dba
vendor: Seagate
—
Reply to this email directly, view it on GitHub
<#2667 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23ES2F2NJURI5WYDRQTYVBSIRAVCNFSM6AAAAABCY5KUNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRRGAZTQOJXG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
And I think this is the crucial point: "beaker'BOOTDISK doesn't fit disk well,as BOOTDISK'properties( or,more exactly, property) are supposed to be string" - Beaker's BOOTDISK. That does not mean it has to be a simple string in our specifications... What
Ok, that's pretty much how disk:
- size: ">= 60 GB"
driver: virtio_blk
# Or:
driver: nvme
# And, of course, in the future, we can also add support for other well-established keys (not necessarily here and now):
vendor-name: Seagate
...
# Or add more keys to support less common disk properties:
wwm: 0x...This would make perfect sense to me. It would be obvious what I'm looking for - Nth disk shall be using a given driver -, we would rely on keys already defined elsewhere for the very same concepts, and it would be easy to extend plugins to materialize the Basically, this would be my bet at this moment. It seems much more fitting than a new key, especially because it's re-using what we already defined for other subsystems.
I don't see it as a waste of time. This is literally my job, to build tools for testing stuff ;) And that requires discussions, reviews, dropping ideas, and inventing new ones when necessary. This is a discussion of how to do something in the best way possible, and that's useful. Especially for me, I never used root disk with OpenStack or
Well, I for one would not be very happy about merging it as is & "let it go". There are loose ends and open topics that should be resolved, merging the patch without doing so for the sake of cleaning one's table would not be right. Those topics won't disappear, and we will be committed to implementing and supporting something that's not done properly. This is not a hot-fix patch, we do have time to discuss things. I'd like there to be a mutual understanding of where this is heading and how things should be designed to deliver good UX and a well-designed tool. |
|
That does not mean it has to be a simple string in *our* specifications...
Yeah,I know,but as you know, openstack root-device also won't accept a
driver param, if we use disk.driver to implement BOOTDISK, it won't
benefit anyone,and will make it a little inconvenient to BOOTDISK users,
though we don't need add new key, we do need add a new subkey,so I didn't
propose that in my comments,but I believe you professional have better
viewpoint than me.
So, am I supposed to update this merge request accordingly after the disk
model-name merge request is merged,or should I send another commit in the
disk-model mr?:)
…On Sat, Feb 24, 2024 at 4:28 AM Miloš Prchlík ***@***.***> wrote:
Yes, I am aware of that. Yet, both "boot disk" and "root disk" describe
very similar concept, I don't see much difference between them. Not enough
to have one key for "boot disk" and another for "root disk" when we can
have just one
Yes,they are very similar concepts, but in beaker,BOOTDISK is for those
users who want to test a specific driver or port type,as there is no
disk.driver property. Typically,users can do some translate work,like find
all the disk models which provide,say driver, megaraid_sas,and write plan
like below,if we offer code to handle model-name as an or-list,but I guess
we should NOT handle it like that. disk: model-name:
model-name-a|model-name-b|.... I was NOT thinking of handling root-disk
property by adding root-disk:true,I was planning to handle it like this: if
there is only one disk specified, then we pass that one to root-device,if
there are more than one disk is specified,then we pass the first one. But
beaker'BOOTDISK doesn't fit disk well,as BOOTDISK'properties( or,more
exactly, property) are supposed to be string,while disk's properties are
supposed to be dict,so we need BOOTDISK merge requests to avoid complexity:
)
And I think this is the crucial point: "beaker'BOOTDISK doesn't fit disk
well,as BOOTDISK'properties( or,more exactly, property) are supposed to be
string" - *Beaker's* BOOTDISK. That does not mean it has to be a simple
string in *our* specifications...
What BOOTDISK really means is the following: "I want a machine with boot
disk based on *this driver*" - and we already have a way to express "I
want a machine with X based on this driver" in our toolbox, we use driver
key to specify such a request. So to me, this sounds like our specification
should follow the established road and use driver: ... when expressing
what Beaker calls BOOTDISK. The value would be a simple string, but
instead of a new, dedicated key, we should add driver key to disk. The
Beaker plugin would then translate it into Beaker's language & emit one
simple element.
I was NOT thinking of handling root-disk property by adding
root-disk:true,I was planning to handle it like this: if there is only one
disk specified, then we pass that one to root-device,if there are more than
one disk is specified,then we pass the first one.
Ok, that's pretty much how disk is treated today, the first item speaks
about the first disk and so on. No problem there. With the driver
addition, we'd get back to the following proposal, just without the
root-disk/boot-disk keys we don't need:
disk:
- size: ">= 60 GB"
driver: virtio_blk
# Or:
driver: nvme
# And, of course, in the future, we can also add support for other well-established keys (not necessarily here and now):
vendor-name: Seagate
...
# Or add more keys to support less common disk properties:
wwm: 0x...
This would make perfect sense to me. It would be obvious what I'm looking
for - Nth disk shall be using a given driver -, we would rely on keys
already defined elsewhere for the very same concepts, and it would be easy
to extend plugins to materialize the disk[].driver requirement.
Basically, this would be my bet at this moment. It seems much more fitting
than a new key, especially because it's re-using what we already defined
for other subsystems.
I think you professional has already spent way more than enough time on
this tiny merge request,
I don't see it as a waste of time. This is *literally* my job, to build
tools for testing stuff ;) And that requires discussions, reviews, dropping
ideas, and inventing new ones when necessary. This is a discussion of how
to do something in the best way possible, and that's useful. Especially for
me, I never used root disk with OpenStack or BOOTDISK with Beaker, and
this helped me gain more insight.
I bet you have lots of more important work to handle, at least from my
side,we have guest-log(my mrack log merge request has already been merged),
boot.method,and virtualization,etc,how about merge this ,and let it go?: )
Well, I for one would not be very happy about merging it as is & "let it
go". There are loose ends and open topics that should be resolved, merging
the patch without doing so for the sake of cleaning one's table would not
be right. Those topics won't disappear, and we will be committed to
implementing and supporting something that's not done properly. This is not
a hot-fix patch, we do have time to discuss things. I'd like there to be a
mutual understanding of where this is heading and how things should be
designed to deliver good UX and a well-designed tool.
—
Reply to this email directly, view it on GitHub
<#2667 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23HSEOR3YA652ZLC673YVD3YJAVCNFSM6AAAAABCY5KUNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRRHE2DQOJUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I disagree :) It would benefit everyone in the sense that an existing concept, "I wish the guest to have a device with a given driver", would be extended to a new device class,
Maybe, yes, they will have to write the following: disk:
- driver: ...To me, the consistency wins over a bit of verbosity.
Yes, that would be my recommendation, to update this PR to add the |
|
Yes, that would be my recommendation, to update this PR to add the driver key
into disk specification, instead of a dedicated boot_disk key.
Sure,will update this merge request after the disk-model one is merged,to
avoid conflicts: )
…On Tue, Feb 27, 2024 at 5:37 AM Miloš Prchlík ***@***.***> wrote:
That does not mean it has to be a simple string in *our* specifications...
Yeah,I know,but as you know, openstack root-device also won't accept a
driver param, if we use disk.driver to implement BOOTDISK, it won't benefit
anyone
I disagree :) It would benefit everyone in the sense that an existing
concept, "I wish the guest to have a device with a given driver", would be
extended to a new device class, disk, using the already established
language, driver. The specification would remain consistent, one thing
would be expressed with the same wording for network, device, gpu, or disk
.
and will make it a little inconvenient to BOOTDISK users
Maybe, yes, they will have to write the following:
disk:
- driver: ...
To me, the consistency wins over a bit of verbosity.
though we don't need add new key, we do need add a new subkey ,so I didn't
propose that in my comments,but I believe you professional have better
viewpoint than me. So, am I supposed to update this merge request
accordingly after the disk model-name merge request is merged,or should I
send another commit in the disk-model mr?:)
Yes, that would be my recommendation, to update this PR to add the driver
key into disk specification, instead of a dedicated boot_disk key.
Please, don't add this as a new commit to the disk.model-name PR, let's
have PRs that focus one one particular change.
—
Reply to this email directly, view it on GitHub
<#2667 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23HOQ6HTPXRDMOHBG4TYVT6BDAVCNFSM6AAAAABCY5KUNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRVGMZTMNJVGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@happz, thanks for the detailed analysis, I agree with your approach. It makes sense to keep design of the specification as consistent as possible. From my point of view, extending the existing |
skycastlelily
force-pushed
the
bootdisk
branch
from
34e27d3
to
df56df5
Compare
skycastlelily
force-pushed
the
bootdisk
branch
from
df56df5
to
bc50f4e
Compare
|
Rebased and updated:) |
psss
added
specification
full test
psss
changed the title
disk.driver hardware requirement
|
Thanks:)
…On Tue, Mar 5, 2024 at 4:36 PM Petr Šplíchal ***@***.***> wrote:
***@***.**** approved this pull request.
Thanks for adding this. I've just slightly adjusted the release note and
spec in 6406f7c
<6406f7c>
.
—
Reply to this email directly, view it on GitHub
<#2667 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23FCD75KZGDFOVOZVS3YWV7X7AVCNFSM6AAAAABCY5KUNWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJWGIZTOOBRGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
/packit test |
Pull Request Checklist