kola: add additionalDisks support in kola.json #1573
Conversation
|
Skipping CI for Draft Pull Request. |
|
See example usage in coreos/ignition#1010. |
mantle/platform/platform.go
Outdated
| NewMachine(userdata *conf.UserData) (Machine, error) | ||
|
|
||
| // NewMachineWithDisks creates a new CoreOS machine with additional disks. | ||
| NewMachineWithDisks(userdata *conf.UserData, addDisks []string) (Machine, error) |
There was a problem hiding this comment.
Does it make sense to have a separate method for this, rather than generalizing NewMachineWithOptions to all platforms?
There was a problem hiding this comment.
I'd be +1 on changing this to be NewMachineWithOptions and having a generalized MachineOptions type. I really like shifting the general implementations up to the interface so we can drop explicit type checking.
I am worried that the current implementation, forcing test errors rather than skipping tests that are requesting additional resources on platforms that don't implement them yet, will be a bit obtuse to work around once tests are added to kola that make use of it. I wonder if there might be some value in implementing a platform check + skip into the harness (similar to what's done for --no-net).
There was a problem hiding this comment.
I'm +1 on NewMachineWithOptions as well.
There was a problem hiding this comment.
This was a bit trickier than expected, because there are some options that are just specific to QEMU, like host port forwarding and pdeathsig handling. I split out a new QEMU-specific type that's a superset of the now generalized MachineOptions type. This does still allow us to clean up the type-checking.
There was a problem hiding this comment.
I am worried that the current implementation, forcing test errors rather than skipping tests that are requesting additional resources on platforms that don't implement them yet, will be a bit obtuse to work around once tests are added to kola that make use of it.
Hmm, can you expand on this? Support for e.g. AWS tests that want to use AdditionalDisks is contingent on first adding AdditionalDisks support for AWS, right?
There was a problem hiding this comment.
Hmm, can you expand on this? Support for e.g. AWS tests that want to use AdditionalDisks is contingent on first adding AdditionalDisks support for AWS, right?
Yes if the test wants to use AdditionalDisks then it requires that the platform support it. With as it was at the time I made the comment (haven't gone through the current code yet) if I say add an external test that requires additional disks and then run my external tests on qemu & aws in CI the tests for aws always fail because we don't support additional disks. What I was talking about was adding something to the harness that would check if the test needed additional disks and if so skip the test if the platform was one that doesn't have the support for adding them.
The key there is a space-separated list of platform names, not a JSON list directly.
Make `NewMachineWithOptions` part of the cross-platform interface instead of being specific to QEMU. This then adds support for additional disks (though it only works on QEMU right now). We add a new `NewMachineWithQemuOptions` type which embeds this type and includes more QEMU-specific features, such as host port forwarding and `PR_SET_PDEATHSIG`. This allows us to simplify tests which previously had to use dynamic type checking in order to add additional disks. This also fixes support for `kola spawn --remove=false`, which broke at some point.
Add the ability to specify additional disks in `kola.json` for use by external tests. This will be useful for testing the new rootfs reprovisioning work. For now, this only supports the qemu platform. Closes: coreos#1565
jlebon
force-pushed
the
pr/add-disks-tests
branch
from
0a5bdff to
07552f6
Compare
|
Ready for review! |
|
LGTM |
It's mostly back up now (still sorting through some issues) and green on this PR! ✔️ |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arithx, jlebon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add the ability to specify additional disks in
kola.jsonfor use byexternal tests. This will be useful for testing the new rootfs
reprovisioning work.
For now, this only supports the qemu platform.
Closes: #1565