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

metal.go: remove optional rom pxe-rtl8139 #2964

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

saqibali-2k
Copy link
Member

@saqibali-2k saqibali-2k commented Jul 6, 2022

The -option-rom .../pxe-rtl8139.rom used to be required but is no
longer needed because according to [1] "If no rom bar is specified,
the qemu default will be used (older versions of qemu used a default
of "off", while newer qemus have a default of "on")".

The attribute file which is used to pass an [1]
alternative boot ROM for a network device is optional and thus
assumed to have a default in QEMU.

So now qemu will default to loading the option rom for the given NIC
device model.

[1] https://gitlab.com/libvirt/libvirt/-/blob/3547875f3a8cf286a03b62f5e7bdb8e4b0eeee29/docs/formatdomain.rst#interface-rom-bios-configuration

fixes: #2918

cgwalters
cgwalters previously approved these changes Jul 6, 2022
miabbott
miabbott previously approved these changes Jul 6, 2022
@saqibali-2k saqibali-2k dismissed stale reviews from miabbott and cgwalters via f1cef3f July 6, 2022 15:44
@saqibali-2k saqibali-2k enabled auto-merge (rebase) July 6, 2022 15:45
@saqibali-2k
Copy link
Member Author

Last push fixes a typo in the commit message.

@dustymabe
Copy link
Member

At some point pxe-rtl8139.rom symlink was moved. Lets change
the path to the new one.

To me this fact begs a few questions:

  1. Is this critical to our tests?
  2. If it is critical to our tests then how did our tests continue to pass?

@cgwalters
Copy link
Member

Is this critical to our tests?

The answer seems to be "no"...that said I think the answer is this bit of code above:
pxe.networkdevice = "e1000"

And I suspect it's something like the e1000 drivers being built into ipxe, so this secondary NIC driver was never needed.

@cgwalters
Copy link
Member

/retest

@dustymabe
Copy link
Member

We did some more digging on this the other day. @saqibali-2k is going to post an update

The `-option-rom .../pxe-rtl8139.rom` used to be required but is no
longer needed because according to [1] "If no rom bar is specified,
the qemu default will be used (older versions of qemu used a default
of "off", while newer qemus have a default of "on")".

The attribute `file` which is used to pass  an [1] `alternative
boot ROM for a network device` is optional and thus assumed to have
a default in QEMU.

So now qemu will default to loading the option rom for the given NIC
device model.

[1] https://gitlab.com/libvirt/libvirt/-/blob/3547875f3a8cf286a03b62f5e7bdb8e4b0eeee29/docs/formatdomain.rst#interface-rom-bios-configuration

fixes: coreos#2918
@cgwalters
Copy link
Member

/override ci/prow/rhcos

@openshift-ci
Copy link

openshift-ci bot commented Jul 12, 2022

@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/rhcos

In response to this:

/override ci/prow/rhcos

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.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Excellent investigation and commit message!

@saqibali-2k
Copy link
Member Author

Excellent investigation and commit message!

Credit goes to Dusty :)

@saqibali-2k
Copy link
Member Author

/retest

@saqibali-2k saqibali-2k changed the title metal.go: change pxe rom path metal.go: remove optional rom pxe-rtl8139 Jul 12, 2022
@travier
Copy link
Member

travier commented Jul 12, 2022

RHCOS is broken until openshift/os#890 is merged

@openshift-ci
Copy link

openshift-ci bot commented Jul 12, 2022

@saqibali-2k: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/rhcos 3be34d3 link true /test rhcos

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@dustymabe
Copy link
Member

RHCOS is broken until openshift/os#890 is merged

which I think means we can ignore this CI failure.

@dustymabe dustymabe disabled auto-merge July 12, 2022 16:07
@dustymabe dustymabe merged commit 575ac55 into coreos:main Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kola testiso complaining about missing pxe-rtl8139.rom
5 participants