-
Notifications
You must be signed in to change notification settings - Fork 165
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 VirtualBox OVA image #2489
Add VirtualBox OVA image #2489
Conversation
Skipping CI for Draft Pull Request. |
Ready for review |
trivial LGTM |
cc @bgilbert for review. |
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.
Some initial comments. I'd also like to go through the hardware settings in VBox and see if they look reasonable. Have you done that also?
This can probably be squashed down to one commit. |
Looked through the resulting VM settings. We should enable EFI and maybe nested virtualization. Networking still seems to be NAT. Also, the storage controller is LSI Logic, which is an old favorite for virtualization but is a bit obscure. AHCI (Serial ATA) is supported, so maybe we should switch to that? |
@bgilbert Are the settings possibly being lost during import? There is a section called "MAC Address Policy" that seems to imply that its overriding with NAT settings. |
Yeah, it's possible. If we can't find a way around that, we should document the fact and how to switch to bridging. |
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.
A couple nits and one thing I missed earlier. Also, please squash down to one commit.
@bgilbert This might be ready now. Go ahead and take a look! |
I think it's okay to fix the remaining issues as a followup if it helps for FCOS freeze. For that purpose, tagging @dustymabe for review, since I can't approve my own PR. |
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.
LGTM
@dustymabe Hey sorry, just pushed a quick fix around the ACHI port count. Could I get your re-approval? |
Tested and LGTM! |
/retest-required |
/retest |
The /override ci/prow/rhcos |
@bgilbert: Overrode contexts on behalf of bgilbert: ci/prow/rhcos In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
stamp!
xref coreos/ignition#1269