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

Revert a commit in ipxe that breaks Mellanox NICs #116

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

nshalman
Copy link
Member

Description

This would update iPXE to the latest master plus a revert of a commit identified to break Mellanox NICs.
This is a temporary measure that might need to be repeated on future updates of iPXE until ipxe/ipxe#1091 has been fixed correctly.

Why is this needed

Relates to: #115, ipxe/ipxe#1091

How Has This Been Tested?

I did repeated testing of iPXE binaries from my branches
mainline-ipxe
test-revert
to confirm that the issue exists and is addressed by the revert.

How are existing users impacted? What migration steps/scripts do we need?

This would unbreak users of certain Mellanox NICs (Definitely Mellanox CX4, possibly others too)

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@nshalman nshalman force-pushed the mellanox-workaround branch from 2be4124 to 93ab6b2 Compare December 11, 2023 17:49
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (33e7193) 97.28% compared to head (04747d7) 97.28%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #116   +/-   ##
=======================================
  Coverage   97.28%   97.28%           
=======================================
  Files           5        5           
  Lines         442      442           
=======================================
  Hits          430      430           
  Misses          8        8           
  Partials        4        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jacobweinstock
Copy link
Member

Hey @nshalman , im not seeing commit fbc3b4a104698658202c2a83217ca8722453bf49 in ipxe/ipxe. you have a link by chance to this commit?

@nshalman
Copy link
Member Author

https://github.com/nshalman/ipxe/tree/fix-mellanox / nshalman/ipxe@fbc3b4a

Because of how github works, ipxe/ipxe@fbc3b4a also resolves.

@jacobweinstock
Copy link
Member

hmmm...im thoroughly confused here. That link ipxe/ipxe@fbc3b4a shows me this message:

image

@jacobweinstock
Copy link
Member

jacobweinstock commented Dec 12, 2023

oh i think i see, maybe. This is just GitHub doing redirects for us. Ultimately, this is a commit from your fork. correct?

@nshalman
Copy link
Member Author

oh i think i see, maybe. This is just GitHub doing redirects for us. Ultimately, this is a commit from your fork. correct?

Correct.

Relates to: tinkerbell#115, ipxe/ipxe#1091
This would update iPXE to the latest master plus a revert
of a commit identified to break Mellanox NICs.

This is a temporary measure that might need to be repeated on
future updates of iPXE until ipxe/ipxe#1091 has been fixed correctly.

Signed-off-by: Nahum Shalman <[email protected]>
@nshalman nshalman force-pushed the mellanox-workaround branch from 93ab6b2 to 04747d7 Compare December 14, 2023 20:46
@nshalman
Copy link
Member Author

I could make it explicit that we're pulling from my fork temporarily, but the nice thing about doing it this way is that as soon as ipxe is bumped the workaround is no longer in place. We fail securely back to upstream ipxe rather than risking staying stuck pointing at a fork. The worst possible outcome of future updates is that issue #115 (which will still be open) reappears in ipxedust.

@jacobweinstock jacobweinstock added the ready-to-merge Mergify: Ready for Merging label Dec 15, 2023
@mergify mergify bot merged commit 8f29898 into tinkerbell:main Dec 15, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Mergify: Ready for Merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants