Skip to content

Improve logs on IpmitoolOutOfBandManagementDriver #4702

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.

Sign up for GitHub

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

Merged
DaanHoogland merged 1 commit into from
Apr 21, 2021
Merged

Improve logs on IpmitoolOutOfBandManagementDriver #4702

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions ...che/cloudstack/outofbandmanagement/driver/ipmitool/IpmitoolOutOfBandManagementDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import org.apache.commons.lang3.StringUtils;

@DaanHoogland DaanHoogland Feb 18, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please use our own (cloudstack) Stringutils as proxy, even when calling the apache.commons

@rafaelweingartner rafaelweingartner Feb 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is no consensus on that. I would prefer this way, using the library directly. We do not get benefits on a facade like that.

@DaanHoogland DaanHoogland Feb 18, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes we do, and we are badly hampered in most cases where we don't (gson/logging mostly)

@rafaelweingartner rafaelweingartner Feb 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So, lets add a facade to Spring as well, and to any other utils library that we use :)

I still see no benefits on creating such facades.

@DaanHoogland DaanHoogland Feb 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that is a lazy exaggeration, we have a facade for string utils and we are hampered by several 3rd party libraries for which we don't. If there are resources for making such a thing for spring, great. Let's do it.

BTW, in this case I didn't 👎 on this issue, but on other counts I might as well. duckduck "using 3rd party library facades", will give you some wisdom on this.

out


public final class IpmitoolOutOfBandManagementDriver extends AdapterBase implements OutOfBandManagementDriver, Configurable {
public static final Logger LOG = Logger.getLogger(IpmitoolOutOfBandManagementDriver.class);
Expand All @@ -59,7 +60,7 @@ public final class IpmitoolOutOfBandManagementDriver extends AdapterBase impleme
private String getIpmiUserId(ImmutableMap<OutOfBandManagement.Option, String> options, final Duration timeOut) {
final String username = options.get(OutOfBandManagement.Option.USERNAME);
if (Strings.isNullOrEmpty(username)) {
throw new CloudRuntimeException("Empty IPMI user configured, cannot proceed to find user's ID");
throw new CloudRuntimeException("Empty IPMI user configured, cannot proceed to find user's ID.");
}

final List<String> ipmiToolCommands = IPMITOOL.getIpmiToolCommandArgs(IpmiToolPath.value(),
Expand All @@ -68,12 +69,17 @@ private String getIpmiUserId(ImmutableMap<OutOfBandManagement.Option, String> op
options, "user", "list");
final OutOfBandManagementDriverResponse output = IPMITOOL.executeCommands(ipmiToolCommands, timeOut);
if (!output.isSuccess()) {
throw new CloudRuntimeException("Failed to find IPMI user to change password, error: " + output.getError());
String oneLineCommand = StringUtils.join(ipmiToolCommands, " ");
String message = String.format("Failed to find IPMI user [%s] to change password. Command [%s], error [%s].", username, oneLineCommand, output.getError());
LOG.debug(message);
throw new CloudRuntimeException(message);
}

final String userId = IPMITOOL.findIpmiUser(output.getResult(), username);
if (Strings.isNullOrEmpty(userId)) {
throw new CloudRuntimeException("No IPMI user ID found for the username: " + username);
String message = String.format("No IPMI user ID found for the username [%s].", username);
LOG.debug(message);
throw new CloudRuntimeException(message);
}
return userId;
}
Expand All @@ -82,13 +88,17 @@ public OutOfBandManagementDriverResponse execute(final OutOfBandManagementDriver
if (!isIpmiToolBinAvailable) {
initDriver();
if (!isIpmiToolBinAvailable) {
return new OutOfBandManagementDriverResponse(null, "Aborting operation due to ipmitool binary not available for execution", false);
String message = "Aborting operation due to ipmitool binary not available for execution.";
LOG.debug(message);
return new OutOfBandManagementDriverResponse(null, message, false);
}
}

OutOfBandManagementDriverResponse response = new OutOfBandManagementDriverResponse(null, "Unsupported Command", false);
if (!isDriverEnabled) {
response.setError("Driver not enabled or shutdown");
String message = "Driver not enabled or shutdown.";
LOG.debug(message);
response.setError(message);
return response;
}
if (cmd instanceof OutOfBandManagementDriverPowerCommand) {
Expand All @@ -98,6 +108,8 @@ public OutOfBandManagementDriverResponse execute(final OutOfBandManagementDriver
}

if (response != null && !response.isSuccess() && response.getError().contains("RAKP 2 HMAC is invalid")) {
String message = String.format("Setting authFailure as 'true' due to [%s].", response.getError());
LOG.debug(message);
response.setAuthFailure(true);
}
return response;
Expand All @@ -111,8 +123,16 @@ private OutOfBandManagementDriverResponse execute(final OutOfBandManagementDrive

final OutOfBandManagementDriverResponse response = IPMITOOL.executeCommands(ipmiToolCommands, cmd.getTimeout());

if (response.isSuccess() && cmd.getPowerOperation().equals(OutOfBandManagement.PowerOperation.STATUS)) {
response.setPowerState(IPMITOOL.parsePowerState(response.getResult().trim()));
String oneLineCommand = StringUtils.join(ipmiToolCommands, " ");
String result = response.getResult().trim();

if (response.isSuccess()) {
LOG.debug(String.format("The command [%s] was successful and got the result [%s].", oneLineCommand, result));
if (cmd.getPowerOperation().equals(OutOfBandManagement.PowerOperation.STATUS)) {
response.setPowerState(IPMITOOL.parsePowerState(result));
}
} else {
LOG.debug(String.format("The command [%s] failed and got the result [%s]. Error: [%s].", oneLineCommand, result, response.getError()));
}
return response;
}
Expand All @@ -131,10 +151,10 @@ private void initDriver() {
final OutOfBandManagementDriverResponse output = IPMITOOL.executeCommands(Arrays.asList(IpmiToolPath.value(), "-V"));
if (output.isSuccess() && output.getResult().startsWith("ipmitool version")) {
isIpmiToolBinAvailable = true;
LOG.debug("OutOfBandManagementDriver ipmitool initialized: " + output.getResult());
LOG.debug(String.format("OutOfBandManagementDriver ipmitool initialized [%s].", output.getResult()));
} else {
isIpmiToolBinAvailable = false;
LOG.error("OutOfBandManagementDriver ipmitool failed initialization with error: " + output.getError() + "; standard output: " + output.getResult());
LOG.error(String.format("OutOfBandManagementDriver ipmitool failed initialization with error [%s]; standard output [%s].", output.getError(), output.getResult()));
}
}

Expand Down