-
Notifications
You must be signed in to change notification settings - Fork 171
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
rflash to upgrade firmware with ipmitool-xcat #355
Conversation
chenglch
commented
Nov 3, 2015
- check ipmitool-xcat version (1.8.15 or above)
- check hpm file
- upgrade firmware with ipmitool-xcat
@whowutwut , rflash require documentation or man page, but this request will not include this as rflash depends on ipmitool-xcat 1.8.15 which should be patched and built in xcat-dep. I will commit a another pull request in xcat-dep, then commit the doc or man page. |
@chenglch is ipmitool-xcat just regular ipmitool that is built in the xcat-deps package? so we rename it to ipmitool-xcat? |
rflash <noderange> [--bpa_acdl]", | ||
rflash <noderange> [--bpa_acdl] | ||
PPC64LE (using BMC Management) specific: | ||
rflash <noderange> [-c] <hpm_file>", |
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.
Is the only option for rflash (with BMC) the -c option? What does -c stand for?
If other options are specified, do we exit with error?
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.
I see below, --check... does that just check the firmware version? Should we change [-c | --check]
in the Usage?
- check ipmitool-xcat version (1.8.15 or above) - check hpm file - upgrade firmware with ipmitool-xcat - add thread support for rflash
@pdlun92, @whowutwut Thanks for your review, I have updated these files again to cover your concerns, besides that I add the thread and lock support to execute rflash |
# step 1 power off | ||
my $cmd = $pre_cmd." chassis power off"; | ||
$output = xCAT::Utils->runcmd($cmd, -1); | ||
if ($::RUNCMD_RC != 0) { |
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.
As both $::RUNCMD_RC and $callback are global variables, will not use multi thread to implement rflash.
I think when we do Perltidy to clean up, we should have a 2nd commit, rather than merge the changes into the single commit. This makes it hard to review formatting changes vs actual code changes. The comment was actually lost in the changes , i saw it in my email notification. |
} | ||
# NOTE (chenglch) lanplus should be used for the task of hpm update | ||
# which indicate the bmc support ipmi protocol version 2.0. | ||
my $pre_cmd = "$IPMIXCAT -H $bmc_addr -I lanplus -U $bmc_userid"; |
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.
Nice change... I like how you made this a variable :-)
I was going to comment on in the first review, but was torn between having a single variable for the command or having it spelled out each time for readability on what steps we are issuing.
rflash to upgrade firmware with ipmitool-xcat
We reviewed this code and seems okay. I know @zet809 was assigned to this Pull request but I would like to get this into tonight weekly build so we can test with it ASAP. |
As the race condition exists and the thread is not suggested in perl, I add a new pull request #382. This implementation make use of Coro, and AnyEvent library which is very similar to the eventlet in python, but we have to add new dependency for Coro library. |
Did you add the threading in the 2nd commit to this Pull request? I think i saw but didn't review very carefully. sorry. Is the threading to speed up the rflash over a noderange? @daniceexi While it's convenient to ammend the commits, I don't think we should do this in our instructions... in step2.... As a reviewer, it's hard to see the additional changes if the whole file changes, if we have multiple commits, we can just look at the 2nd or 3rd, or 4th one to see the deltas... rather than review again from scratch... |
@whowutwut
*Cons Maybe a compromises could be that commit can separated if the change has multiple lines. If the change is trivial, maybe use --amend is better. |