-
Notifications
You must be signed in to change notification settings - Fork 10
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
Iscsi offload changes #136
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 11739629419Details
💛 - Coveralls |
teclator
force-pushed
the
iscsi_offload_changes
branch
from
October 31, 2024 10:58
b8867c6
to
90bd033
Compare
teclator
force-pushed
the
iscsi_offload_changes
branch
from
October 31, 2024 11:27
c4747fc
to
7e21b88
Compare
teclator
force-pushed
the
iscsi_offload_changes
branch
from
October 31, 2024 11:41
31262d5
to
5888ca5
Compare
ancorgs
reviewed
Nov 5, 2024
ancorgs
reviewed
Nov 5, 2024
dgdavid
reviewed
Nov 6, 2024
ancorgs
approved these changes
Nov 8, 2024
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.
This is certainly far from being a trivial pull request. Generally it LGTM. I hope we are not overlooking something (neither in the code or at iSCSI behavior).
✔️ Internal Jenkins job #8 successfully finished |
This was referenced Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Bugzilla: bsc#1231385
In the "iSCSI initiator" dialog, the offload card was not displayed correctly. Instead of bnx2i, the dialog showed "default (TCP)". I'm not sure what would have happened if I hadn't changed this to bnx2i.
In the "iBFT" dialog, the bnx2i transport was displayed correctly.
Solution
The selection of the offload card is relevant just in case we wanted to do some discover using that specific interface.
When a different offload card is selected it calls the iscsi_offload.sh script and select the related iscsi_iface to be used by discovery commands.
But the point is that the configuration is already done during in the initialization, so, doing it on selection doesn’t change anything at all.
What is even more important, the login and configuration most of the time is done automatically by firmware and the iscsi_offload script just creates redundant iscsi iface files… therefore, after some discussion it has been decided we could get rid of the script relying on the
iscsiadm -m fw -l
andiscsiadm -m discovery -t
fw calls.Out of the scope (to improve in the future)
During the review and testing we found that there are some behavior / bugs that could be addressed but not as part of this bug:
Testing
Screenshots