From 283cdf35f0ef0fd35b54ea38de52c0c4ba1691cf Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 20:02:40 +0200 Subject: [PATCH 1/3] Make real BMCs collect reliably: keep-alive, retries, one-way tags, OEM states Fixes four problems found by the lab survey of 2026-10-09 and the Codex review of #148. #146 - HP iLO 5 revokes a session (every command then fails with D4h) 120 s after it opens, and about 60 s after it when a Get Channel Authentication Capabilities or a Set Session Privilege Level is sent during the session. The keep-alive sent Get Channel Authentication Capabilities every 30 s, so the FRU walk (about 60 s on that iLO) failed half the time. The keep-alive is now a Get Device ID, as ipmitool sends, which leaves the session its 120 s. Resending the refused request in the same session never succeeds (61 tries over 22 s), so no same-session retry is added. A Get Sensor Reading refused with any completion code now costs only that sensor (WARN naming the host), not the whole walk. #147 - Dell iDRAC 8 answers C3h for its absent FRUs within 15-200 ms and loses about 1.5% of the replies, which succeed when sent again at once. The connector waited a random pause of up to idleTime (4 s) before every resend. It now resends at once after a lost reply or a C3h; the pause only follows the codes that say the BMC is busy. Every try takes a fresh tag: resending under the tag of a request that was just answered raced with the receiver releasing that tag, and could wait forever. The retry WARN is one line, without a stack trace. #150 - A one-way IPMI message is queued like any request (QueueElement carries a one-way flag), so its tag stays reserved until its reply or its timeout; its reply is dropped and its timeout not reported. The SOL ACK-only packets, which the BMC never acknowledges, keep an unqueued tag (SolMessageHandler.takeTag). Connection.KeepAlive, added in this release cycle, becomes private. #145 - Values the record decoders do not model (OEM or chassis-specific entity IDs, reserved device or sensor types, undefined states) are logged at DEBUG instead of ERROR or WARN. The event/reading types 70h-7Fh are OEM (ReadingType.isOem): their sensors report the raw state bytes, and an OEM sensor with no state asserted reports no state. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- .../ipmi/client/runner/GetSensorsRunner.java | 64 ++++---- .../ipmi/core/api/sync/IpmiConnector.java | 33 ++-- .../core/coding/commands/CommandCodes.java | 5 + .../commands/fru/record/ChassisType.java | 2 +- .../fru/record/FruMultiRecordType.java | 2 +- .../record/ManagementAccessRecordType.java | 2 +- .../commands/sdr/record/DeviceType.java | 2 +- .../coding/commands/sdr/record/EntityId.java | 2 +- .../commands/sdr/record/ReadingType.java | 22 ++- .../commands/sdr/record/SensorType.java | 2 +- .../commands/sdr/record/SensorUnit.java | 2 +- .../ipmi/core/connection/Connection.java | 53 +++++-- .../core/connection/IpmiMessageHandler.java | 20 +-- .../ipmi/core/connection/MessageHandler.java | 26 ++- .../core/connection/SolMessageHandler.java | 9 ++ .../core/connection/queue/MessageQueue.java | 43 ++++- .../core/connection/queue/QueueElement.java | 18 +++ src/main/resources/vxipmi.properties | 2 +- src/site/markdown/configuration.md | 6 +- src/site/markdown/installation.md | 6 +- src/site/markdown/low-level-api.md | 11 +- src/site/markdown/sensors.md | 8 +- src/site/markdown/supported-commands.md | 2 +- src/site/markdown/timeouts-and-errors.md | 31 ++-- src/site/markdown/troubleshooting.md | 7 + src/site/markdown/upgrading.md | 33 +++- .../client/runner/GetSensorsRunnerTest.java | 76 +++++++++ .../ipmi/core/api/sync/IpmiConnectorTest.java | 148 ++++++++++++++++++ .../commands/sdr/record/ReadingTypeTest.java | 30 ++++ .../ipmi/core/connection/ConnectionTest.java | 10 +- .../connection/queue/MessageQueueTest.java | 31 +++- 32 files changed, 580 insertions(+), 130 deletions(-) create mode 100644 src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ReadingTypeTest.java diff --git a/README.md b/README.md index b99164d..519ab8c 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ The BMC must have IPMI over LAN enabled and an account with the User privilege: ## Upgrading -Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/upgrading.html#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged. The Full, Compact and Event-Only sensor records now share the `AbstractSensorRecord` superclass, and commands can check responses with `IpmiCommandCoder.validateResponse()`; both are described on the same page. The user name and password are now encoded in UTF-8 whatever the platform charset, and the BMC key is used as raw bytes; as a result, `AuthenticationAlgorithm.getKeyExchangeAuthenticationCode()` and `checkKeyExchangeAuthenticationCode()` take the key and password as `byte[]` instead of `String`. `UdpMessenger.getSentPackets()` is removed, and the `CONST1`/`CONST2` constants of `IntegrityAlgorithm` and `ConfidentialityAesCbc128` are now `private`. `IpmiConnector.closeConnection()` releases the connection, whose handle then throws `IllegalStateException`; the keep-alive is actually sent with the default configuration, as a `Connection.KeepAlive` request whose reply and timeout are not reported to the listeners; and `Constants.TIMEOUT`, which nothing reads, is deprecated. +Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/upgrading.html#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged. The Full, Compact and Event-Only sensor records now share the `AbstractSensorRecord` superclass, and commands can check responses with `IpmiCommandCoder.validateResponse()`; both are described on the same page. The user name and password are now encoded in UTF-8 whatever the platform charset, and the BMC key is used as raw bytes; as a result, `AuthenticationAlgorithm.getKeyExchangeAuthenticationCode()` and `checkKeyExchangeAuthenticationCode()` take the key and password as `byte[]` instead of `String`. `UdpMessenger.getSentPackets()` is removed, and the `CONST1`/`CONST2` constants of `IntegrityAlgorithm` and `ConfidentialityAesCbc128` are now `private`. `IpmiConnector.closeConnection()` releases the connection, whose handle then throws `IllegalStateException`; the keep-alive is actually sent with the default configuration, as a one-way Get Device ID whose reply and timeout are not reported to the listeners; a one-way IPMI message keeps its tag reserved until its reply or its timeout; and `Constants.TIMEOUT`, which nothing reads, is deprecated. ## Build instructions diff --git a/src/main/java/org/metricshub/ipmi/client/runner/GetSensorsRunner.java b/src/main/java/org/metricshub/ipmi/client/runner/GetSensorsRunner.java index b919ba2..aee8889 100644 --- a/src/main/java/org/metricshub/ipmi/client/runner/GetSensorsRunner.java +++ b/src/main/java/org/metricshub/ipmi/client/runner/GetSensorsRunner.java @@ -45,13 +45,15 @@ import org.metricshub.ipmi.core.coding.payload.lan.IPMIException; import org.metricshub.ipmi.core.coding.protocol.AuthenticationType; import org.metricshub.ipmi.core.common.TypeConverter; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Get Full And Compact Sensor records */ public class GetSensorsRunner extends AbstractIpmiRunner> { - private static final int OEM_EVENT_READING_TYPE = 127; + private static final Logger LOGGER = LoggerFactory.getLogger(GetSensorsRunner.class); public GetSensorsRunner(IpmiClientConfiguration ipmiConfiguration) { super(ipmiConfiguration); @@ -88,12 +90,9 @@ public List call() throws Exception { // Only Full and Compact sensor records have a reading associated // with them (see IPMI specification for details) if (sensorRecord instanceof FullSensorRecord || sensorRecord instanceof CompactSensorRecord) { - int recordReadingId = TypeConverter - .byteToInt(((AbstractSensorRecord) sensorRecord).getSensorNumber()); - // If our record has got a reading associated, we get request // for it - GetSensorReadingResponseData data = getSensorRecordReading(recordReadingId); + GetSensorReadingResponseData data = getSensorRecordReading((AbstractSensorRecord) sensorRecord); // Build the states e.g. deviceName=OK|deviceName=Device Present String states = buildStates(data, sensorRecord); @@ -146,12 +145,13 @@ static String buildStates(final GetSensorReadingResponseData data, final SensorR final AbstractSensorRecord record = (AbstractSensorRecord) sensorRecord; final String deviceName = record.getName(); - if (record.getEventReadingType() == OEM_EVENT_READING_TYPE) { - return buildOemState(data.getRaw(), deviceName); - } - final List events = data.getStatesAsserted(record.getSensorType(), record.getEventReadingType()); + // Like any discrete sensor, an OEM sensor with no state asserted reports no state + if (ReadingType.isOem(record.getEventReadingType())) { + return events.isEmpty() ? Utils.EMPTY : buildOemState(data.getRaw(), deviceName); + } + return appendReadingTypes(events, deviceName); } catch (Exception e) { @@ -160,7 +160,7 @@ static String buildStates(final GetSensorReadingResponseData data, final SensorR } /** - * Build the state for oem event reading type (0x7f) + * Build the state of an OEM event/reading type (70h-7Fh) * * @param raw a byte array of the raw IPMI command data * @param deviceName the name of the device @@ -210,32 +210,36 @@ private static String createStateEntry(final String deviceName, final ReadingTyp } /** - * Using the given reading id run the GetSensorReading request to get reading data + * Run the GetSensorReading request of a sensor record. A reading the BMC does not provide (DataNotPresent) or + * refuses with another completion code costs that sensor its reading, not the whole walk. * - * @param recordReadingId the reading identifier of the sensor record - * @return {@link GetSensorReadingResponseData} instance - * @throws Exception at sendMessage or if the error completion code is not DataNotPresent + * @param sensorRecord the Full or Compact sensor record + * @return {@link GetSensorReadingResponseData} instance, or null when the BMC returned no reading + * @throws Exception at sendMessage when the BMC does not answer */ - private GetSensorReadingResponseData getSensorRecordReading(final int recordReadingId) throws Exception { + GetSensorReadingResponseData getSensorRecordReading(final AbstractSensorRecord sensorRecord) + throws Exception { + int sensorNumber = TypeConverter.byteToInt(sensorRecord.getSensorNumber()); try { - // If we have a reading id means the reading data (e.g. temperature) is potentially available so let's perform the - // re - if (recordReadingId >= 0) { - return (GetSensorReadingResponseData) getConnector() - .sendMessage( - getHandle(), - new GetSensorReading( - IpmiVersion.V20, - getHandle().getCipherSuite(), - AuthenticationType.RMCPPlus, - recordReadingId)); - - } + return (GetSensorReadingResponseData) getConnector() + .sendMessage( + getHandle(), + new GetSensorReading( + IpmiVersion.V20, + getHandle().getCipherSuite(), + AuthenticationType.RMCPPlus, + sensorNumber)); } catch (IPMIException e) { if (e.getCompletionCode() != CompletionCode.DataNotPresent) { - throw e; + LOGGER + .warn( + "Failed to read sensor {} ({}) on {}: {}", + sensorNumber, + sensorRecord.getName(), + getIpmiConfiguration().getHostname(), + e.getMessage()); } + return null; } - return null; } } diff --git a/src/main/java/org/metricshub/ipmi/core/api/sync/IpmiConnector.java b/src/main/java/org/metricshub/ipmi/core/api/sync/IpmiConnector.java index 135f0e6..814f144 100644 --- a/src/main/java/org/metricshub/ipmi/core/api/sync/IpmiConnector.java +++ b/src/main/java/org/metricshub/ipmi/core/api/sync/IpmiConnector.java @@ -29,6 +29,7 @@ import org.metricshub.ipmi.core.coding.commands.PrivilegeLevel; import org.metricshub.ipmi.core.coding.commands.ResponseData; import org.metricshub.ipmi.core.coding.commands.session.GetChannelAuthenticationCapabilitiesResponseData; +import org.metricshub.ipmi.core.coding.payload.CompletionCode; import org.metricshub.ipmi.core.coding.payload.lan.IPMIException; import org.metricshub.ipmi.core.coding.protocol.PayloadType; import org.metricshub.ipmi.core.coding.security.CipherSuite; @@ -402,20 +403,15 @@ private ResponseData sendThroughAsyncConnector( ResponseData responseData = null; int tries = 0; - int tag = -1; boolean messageSent = false; while (!messageSent) { try { ++tries; - if (tag >= 0) { - tag = asyncConnector.retry(connectionHandle, tag, request.getSupportedPayloadType()); - } - - if (tag < 0) { - tag = asyncConnector.sendMessage(connectionHandle, request, !waitForResponse); - } + // A fresh tag for every try: the request that timed out or was answered left the queue, or is about + // to, and a reply to the same tag would then be dropped as an orphan + int tag = asyncConnector.sendMessage(connectionHandle, request, !waitForResponse); logger.debug("Sending message with tag {}, try {}", tag, tries); @@ -429,27 +425,36 @@ private ResponseData sendThroughAsyncConnector( } catch (IPMIException e) { handleErrorResponse(tries, e); } catch (Exception e) { - handleRetriesWhenException(tries, e); + // No reply in time: the BMC already had the whole message timeout, the request is sent again at once + handleRetriesWhenException(tries, e, false); } } return responseData; } - private void handleRetriesWhenException(int tries, Exception e) throws Exception { + /** + * Throws the exception when the request was tried {@link #retries} times already, otherwise lets the caller send + * it again, after a random pause of up to {@link #idleTime} ms if asked. + */ + private void handleRetriesWhenException(int tries, Exception e, boolean pause) throws Exception { if (tries > retries) { throw e; - } else { + } + if (pause) { long sleepTime = (random.nextLong() % (idleTime / 2)) + (idleTime / 2); - Thread.sleep(sleepTime); - logger.warn("Receiving message failed, retrying", e); } + logger.warn("Receiving message failed, retrying: {}: {}", e.getClass().getSimpleName(), e.getMessage()); } + /** + * Retries a request the BMC answered with a transient completion code: after a pause when the BMC says it is busy, + * at once after a timeout on its side (C3h), where it already waited for the device it could not reach. + */ private void handleErrorResponse(int tries, IPMIException e) throws Exception { if (e.getCompletionCode().isTransient()) { - handleRetriesWhenException(tries, e); + handleRetriesWhenException(tries, e, e.getCompletionCode() != CompletionCode.Timeout); } else { throw e; } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/CommandCodes.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/CommandCodes.java index 44e4d15..a7c02c6 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/CommandCodes.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/CommandCodes.java @@ -39,6 +39,11 @@ public final class CommandCodes { */ public static final byte GET_CHASSIS_STATUS = 0x01; + /** + * An IPMI code for Get Device ID command (Application network function) + */ + public static final byte GET_DEVICE_ID = 0x01; + /** * An IPMI code for Chassis Control command */ diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ChassisType.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ChassisType.java index 98b5cad..067b790 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ChassisType.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ChassisType.java @@ -168,7 +168,7 @@ public static ChassisType parseInt(int value) { case LAPTOP: return LapTop; default: - logger.error("Invalid value: " + value); + logger.debug("Invalid value: {}", value); return Other; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/FruMultiRecordType.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/FruMultiRecordType.java index 28d048d..e6a9164 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/FruMultiRecordType.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/FruMultiRecordType.java @@ -88,7 +88,7 @@ public static FruMultiRecordType parseInt(int value) { case EXTENDEDCOMPATIBILITYRECORD: return ExtendedCompatibilityRecord; default: - logger.error("Invalid value: " + value); + logger.debug("Invalid value: {}", value); return Unspecified; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ManagementAccessRecordType.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ManagementAccessRecordType.java index ae276bb..291823f 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ManagementAccessRecordType.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ManagementAccessRecordType.java @@ -67,7 +67,7 @@ public static ManagementAccessRecordType parseInt(int value) { case COMPONENTMANAGEMENTURL: return ComponentManagementURL; default: - logger.error("Invalid value: " + value); + logger.debug("Invalid value: {}", value); return Unspecified; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/DeviceType.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/DeviceType.java index af6e9e4..6ab1aae 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/DeviceType.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/DeviceType.java @@ -172,7 +172,7 @@ public static DeviceType parseInt(int value) { case EEPROM24C02: return Eeprom24C02; default: - logger.error("Invalid value: " + value); + logger.debug("Invalid value: {}", value); return Other; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/EntityId.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/EntityId.java index d1a19ee..6ea8e69 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/EntityId.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/EntityId.java @@ -351,7 +351,7 @@ public static EntityId parseInt(int value) { case BASEBOARD: return Baseboard; default: - logger.error("Invalid value: " + value); + logger.debug("Invalid value: {}", value); return Other; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ReadingType.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ReadingType.java index a65fb2c..ee66fe8 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ReadingType.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ReadingType.java @@ -1119,7 +1119,19 @@ public int getCode() { } /** - * Determines type of discrete sensor reading. + * Tells whether an event/reading type is OEM: the IPMI 2.0 specification (Table 42-1) reserves {@code 70h} to + * {@code 7Fh} for OEM use, and defines none of their states. + * + * @param eventReadingType the event/reading type code of a sensor record + * @return true for an OEM event/reading type + */ + public static boolean isOem(int eventReadingType) { + return eventReadingType >= 0x70 && eventReadingType <= 0x7f; + } + + /** + * Determines type of discrete sensor reading. The states of an OEM event/reading type or of an OEM sensor type are + * {@link #UnknownOEMEvent}. * * @param sensorType * - {@link SensorType} of the sensor @@ -1132,7 +1144,7 @@ public int getCode() { */ public static ReadingType parseInt(SensorType sensorType, int eventReadingType, int offset) { - if (sensorType == SensorType.Oem) { + if (sensorType == SensorType.Oem || isOem(eventReadingType)) { return UnknownOEMEvent; } @@ -1670,10 +1682,8 @@ public static ReadingType parseInt(SensorType sensorType, int eventReadingType, case MONITORASICIC: return MonitorAsicIc; default: - logger - .warn( - "Invalid value: " + value + " (" + Integer.toHexString(value) - + ") for sensor " + sensorType); + // BMCs assert states the reading type does not define (HP iLO sets bits 6 and 7 of generic types) + logger.debug("Invalid value: {} ({}) for sensor {}", value, Integer.toHexString(value), sensorType); return Unknown; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorType.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorType.java index 18eb0ce..4a8f21d 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorType.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorType.java @@ -231,7 +231,7 @@ public static SensorType parseInt(int value) { if (value >= OEM) { return Oem; } - logger.error("Invalid value: " + value); + logger.debug("Invalid value: {}", value); return Oem; } } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorUnit.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorUnit.java index 5ed5c76..6c7ae23 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorUnit.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/sdr/record/SensorUnit.java @@ -532,7 +532,7 @@ public static SensorUnit parseInt(int value) { case CORRECTABLEERROR: return CorrectableError; default: - logger.error("Invalid value: " + value); + logger.debug("Invalid value: {}", value); return Other; } } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java index 2491bf2..97d1fd6 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java @@ -23,16 +23,23 @@ */ import org.metricshub.ipmi.core.coding.PayloadCoder; +import org.metricshub.ipmi.core.coding.commands.CommandCodes; +import org.metricshub.ipmi.core.coding.commands.IpmiCommandCoder; import org.metricshub.ipmi.core.coding.commands.IpmiVersion; import org.metricshub.ipmi.core.coding.commands.PrivilegeLevel; import org.metricshub.ipmi.core.coding.commands.ResponseData; -import org.metricshub.ipmi.core.coding.commands.session.GetChannelAuthenticationCapabilities; import org.metricshub.ipmi.core.coding.commands.session.GetChannelAuthenticationCapabilitiesResponseData; import org.metricshub.ipmi.core.coding.commands.session.GetChannelCipherSuitesResponseData; import org.metricshub.ipmi.core.coding.commands.session.OpenSessionResponseData; import org.metricshub.ipmi.core.coding.commands.session.Rakp1ResponseData; import org.metricshub.ipmi.core.coding.commands.session.Rakp3ResponseData; import org.metricshub.ipmi.core.coding.payload.IpmiPayload; +import org.metricshub.ipmi.core.coding.payload.lan.IPMIException; +import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanMessage; +import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanRequest; +import org.metricshub.ipmi.core.coding.payload.lan.NetworkFunction; +import org.metricshub.ipmi.core.coding.protocol.AuthenticationType; +import org.metricshub.ipmi.core.coding.protocol.IpmiMessage; import org.metricshub.ipmi.core.coding.protocol.Ipmiv20Message; import org.metricshub.ipmi.core.coding.protocol.PayloadType; import org.metricshub.ipmi.core.coding.security.AuthenticationRakpHmacSha1; @@ -680,24 +687,44 @@ public void notify(StateMachineAction action) { } /** - * The keep-alive request: queued like any request, so that its tag stays reserved until its reply arrives or it - * times out, but owned by nobody: its reply is discarded and its timeout is not reported to the listeners. + * The keep-alive request: a Get Device ID, as ipmitool sends. HP iLO 5 revokes the privileges of a session 60 s + * after a Get Channel Authentication Capabilities or a Set Session Privilege Level sent in it, instead of 120 s. */ - public static final class KeepAlive extends GetChannelAuthenticationCapabilities { + private static final class KeepAlive extends IpmiCommandCoder { + + private KeepAlive(CipherSuite cipherSuite) { + super(IpmiVersion.V20, cipherSuite, AuthenticationType.RMCPPlus); + } + + @Override + public byte getCommandCode() { + return CommandCodes.GET_DEVICE_ID; + } + + @Override + public NetworkFunction getNetworkFunction() { + return NetworkFunction.ApplicationRequest; + } + + @Override + protected IpmiLanMessage preparePayload(int sequenceNumber) { + return new IpmiLanRequest(getNetworkFunction(), getCommandCode(), null, TypeConverter.intToByte(sequenceNumber)); + } + /** - * Creates the keep-alive request of a session. - * - * @param cipherSuite the {@link CipherSuite} of the session + * Never called: the keep-alive is sent one-way, its reply is discarded unread. */ - public KeepAlive(CipherSuite cipherSuite) { - super(IpmiVersion.V20, IpmiVersion.V20, cipherSuite, PrivilegeLevel.Callback, TypeConverter.intToByte(0xe)); + @Override + public ResponseData getResponseData(IpmiMessage message) throws IPMIException { + validateResponse(message); + return null; } } /** - * {@link TimerTask} runner - periodically sends a no-op message to keep the session up. The message is a - * {@link KeepAlive}: its reply is discarded, while a reply to the same command sent by the application is - * delivered to the application. When the message queue is full, the keep-alive of this period is skipped. + * {@link TimerTask} runner - periodically sends a no-op message to keep the session up: a Get Device ID sent + * one-way, whose reply and timeout are not reported to the listeners. When the message queue is full, the + * keep-alive of this period is skipped. */ @Override public void run() { @@ -706,7 +733,7 @@ public void run() { return; } try { - sendMessage(new KeepAlive(((SessionValid) current).getCipherSuite()), false); + sendMessage(new KeepAlive(((SessionValid) current).getCipherSuite()), true); } catch (Exception e) { LOGGER.error("Keep-alive failed: " + e.getMessage(), e); } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java index 0383606..6fdaee8 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/IpmiMessageHandler.java @@ -27,6 +27,7 @@ import org.metricshub.ipmi.core.coding.commands.ResponseData; import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanMessage; import org.metricshub.ipmi.core.coding.protocol.Ipmiv20Message; +import org.metricshub.ipmi.core.connection.queue.QueueElement; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -73,8 +74,9 @@ protected void handleIncomingMessageInternal(Ipmiv20Message message) { if (message.getPayload() instanceof IpmiLanMessage) { IpmiLanMessage lanMessagePayload = (IpmiLanMessage) message.getPayload(); - PayloadCoder coder = getMessageQueue().getMessageFromQueue(lanMessagePayload.getSequenceNumber()); int tag = lanMessagePayload.getSequenceNumber(); + QueueElement element = getMessageQueue().getElement(tag); + PayloadCoder coder = element == null ? null : element.getRequest(); LOGGER.debug("Received message with tag " + tag); @@ -86,14 +88,8 @@ protected void handleIncomingMessageInternal(Ipmiv20Message message) { return; } - if (coder instanceof Connection.KeepAlive) { - // Nobody waits for the reply of the keep-alive: it only frees the tag - getMessageQueue().remove(tag); - return; - } - - // A late reply to a one-way message, whose tag is not reserved, may carry the tag of a newer queued - // request: the reply of a request answers the command of that request, and the request stays queued + // A late reply to a request that timed out may carry the tag of a newer queued request: the reply of a + // request answers the command of that request, and the request stays queued if (coder instanceof IpmiCommandCoder && !answers((IpmiCommandCoder) coder, lanMessagePayload)) { LOGGER .debug( @@ -102,6 +98,12 @@ protected void handleIncomingMessageInternal(Ipmiv20Message message) { return; } + if (element.isOneWay()) { + // Nobody waits for the reply of a one-way message (the keep-alive): it only frees the tag + getMessageQueue().remove(tag); + return; + } + try { ResponseData responseData = coder.getResponseData(message); getConnection().notifyResponseListeners(getConnection().getHandle(), tag, responseData, null); diff --git a/src/main/java/org/metricshub/ipmi/core/connection/MessageHandler.java b/src/main/java/org/metricshub/ipmi/core/connection/MessageHandler.java index c4e0517..c1c576d 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/MessageHandler.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/MessageHandler.java @@ -95,15 +95,17 @@ public MessageHandler(Connection connection, int timeout, int minSequenceNumber, * ID of the current session. * @param isOneWay * flag indicating, if message is one way and we shouldn't await response, - * or it isn't and needs response from remote system - * @return sequence number of the sent message + * or it isn't and needs response from remote system. A one-way IPMI message is queued all the same, so that + * its tag stays reserved until its reply arrives or it times out, but its reply and its timeout are not + * reported (see {@link #takeTag(PayloadCoder, boolean)}). + * @return sequence number of the sent message, or -1 when the queue is full * @throws ConnectionException when could not send message due to some problems with connection */ public int sendMessage(PayloadCoder payloadCoder, StateMachine stateMachine, int sessionId, boolean isOneWay) throws ConnectionException { validateSessionState(stateMachine); - int seq = isOneWay ? messageQueue.getSequenceNumber() : messageQueue.add(payloadCoder); + int seq = takeTag(payloadCoder, isOneWay); if (seq > 0) { stateMachine .doTransition(new Sendv20Message(payloadCoder, sessionId, seq, connection.getNextSessionSequenceNumber())); @@ -112,6 +114,18 @@ public int sendMessage(PayloadCoder payloadCoder, StateMachine stateMachine, int return seq; } + /** + * Takes the tag of a message to send by queuing it: the tag stays reserved until its reply arrives or it times + * out. + * + * @param payloadCoder the message to send + * @param isOneWay true when nobody waits for the reply + * @return the tag of the message, or -1 when the queue is full + */ + protected int takeTag(PayloadCoder payloadCoder, boolean isOneWay) { + return messageQueue.add(payloadCoder, isOneWay); + } + /** * Attempts to retry sending message with given tag, assuming that this message exists in message queue. * @@ -178,6 +192,12 @@ public void tearDown() { messageQueue.tearDown(); } + /** + * Returns a sequence number for a message sent outside the queue (the Close Session request, the SOL ACK-only + * packets). + * + * @return a sequence number that no queued request holds + */ public int getSequenceNumber() { return messageQueue.getSequenceNumber(); } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/SolMessageHandler.java b/src/main/java/org/metricshub/ipmi/core/connection/SolMessageHandler.java index def4655..5d550f9 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/SolMessageHandler.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/SolMessageHandler.java @@ -45,6 +45,15 @@ public SolMessageHandler(Connection connection, int timeout) throws IOException super(connection, timeout, SolMessage.MIN_SEQUENCE_NUMBER, SolMessage.MAX_SEQUENCE_NUMBER); } + /** + * The one-way SOL messages are the ACK-only packets, which go out with packet sequence number 0 and which the BMC + * never acknowledges (IPMI 2.0 section 15.9): they are not queued, so that they hold no slot of the window. + */ + @Override + protected int takeTag(PayloadCoder payloadCoder, boolean isOneWay) { + return isOneWay ? getSequenceNumber() : super.takeTag(payloadCoder, isOneWay); + } + /** * Assuming that given message is SOL message, reads both data and acknowledge information from it, * notifying registered listeners about incoming data. diff --git a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java index 1d69e3c..c477df3 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java @@ -136,6 +136,20 @@ private synchronized void releaseTag(int tag) { * that value. */ public int add(PayloadCoder request) { + return add(request, false); + } + + /** + * Adds request to the queue and generates the tag. A one-way request holds its tag like any other until its reply + * arrives or it times out, so that a late reply cannot be taken for the reply of a newer request; its reply and + * its timeout are not reported to the listeners. + * + * @param request the request to queue + * @param oneWay true when nobody waits for the reply + * @return Sequence number of the message if it was added to the queue, -1 otherwise. The tag used to identify + * message is equal to that value. + */ + public int add(PayloadCoder request, boolean oneWay) { run(); boolean first = true; synchronized (queue) { @@ -166,7 +180,7 @@ public int add(PayloadCoder request) { lastSequenceNumber = sequenceNumber; - QueueElement element = new QueueElement(sequenceNumber, request); + QueueElement element = new QueueElement(sequenceNumber, request, oneWay); queue.add(element); return sequenceNumber; @@ -240,8 +254,10 @@ public boolean containsId(int sequenceNumber) { } /** - * Returns the sequence number for a message that awaits no reply: it skips the tags of the queued requests, so - * a reply to the one-way message can never be taken for the reply of a queued request. + * Returns a sequence number for a message sent outside the queue (the Close Session request, the SOL ACK-only + * packets): it skips the tags of the queued requests. + * + * @return a sequence number that no queued request holds */ public int getSequenceNumber() { synchronized (lastSequenceNumberLock) { @@ -254,6 +270,23 @@ public int getSequenceNumber() { } } + /** + * Returns the queued request with the given tag, with its one-way flag. + * + * @param tag the tag of the request + * @return the {@link QueueElement} of the request, or null if no request with the given tag awaits a reply + */ + public QueueElement getElement(int tag) { + synchronized (queue) { + for (QueueElement element : queue) { + if (element.getId() == tag && element.getRequest() != null) { + return element; + } + } + } + return null; + } + /** * Returns message with the given sequence number from the queue or null if * no message with the given tag is currently in the queue. @@ -353,7 +386,7 @@ private boolean messageJustTimedOut(QueueElement oldestQueueElement) { /** * Removes the oldest message from the queue; when it timed out (rather than being answered), the response - * listeners are told so, which lets the sender retry it with a fresh tag. Nobody waits for a keep-alive: its + * listeners are told so, which lets the sender retry it with a fresh tag. Nobody waits for a one-way message: its * timeout is not reported. */ private void processObsoleteMessage(QueueElement message, boolean done) { @@ -362,7 +395,7 @@ private void processObsoleteMessage(QueueElement message, boolean done) { queue.remove(0); releaseTag(tag); - if (!done && !(message.getRequest() instanceof Connection.KeepAlive)) { + if (!done && !message.isOneWay()) { logger.debug("Message timed out, tag: {}", tag); try { connection diff --git a/src/main/java/org/metricshub/ipmi/core/connection/queue/QueueElement.java b/src/main/java/org/metricshub/ipmi/core/connection/queue/QueueElement.java index d3f0a9d..3c835b0 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/queue/QueueElement.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/queue/QueueElement.java @@ -38,14 +38,32 @@ public class QueueElement { private PayloadCoder request; private ResponseData response; private Date timestamp; + private final boolean oneWay; public QueueElement(int id, PayloadCoder request) { + this(id, request, false); + } + + /** + * @param id the tag of the request + * @param request the request awaiting its reply + * @param oneWay true when nobody waits for the reply: the reply and the timeout of the request are not reported + */ + public QueueElement(int id, PayloadCoder request, boolean oneWay) { this.id = id; this.request = request; + this.oneWay = oneWay; timestamp = new Date(); retries = 0; } + /** + * @return true when nobody waits for the reply: the element only reserves the tag until the reply or the timeout + */ + public boolean isOneWay() { + return oneWay; + } + public int getId() { return id; } diff --git a/src/main/resources/vxipmi.properties b/src/main/resources/vxipmi.properties index 7bda221..dac06de 100644 --- a/src/main/resources/vxipmi.properties +++ b/src/main/resources/vxipmi.properties @@ -1,4 +1,4 @@ #Indicates how many times the message will be retried on a failure retries=3 -#Idle time in ms before resending a message that didn't receive answer. +#Upper bound in ms of the random pause before resending a request the BMC answered as busy (node busy, out of resources, initialization in progress). idleTime=4000 diff --git a/src/site/markdown/configuration.md b/src/site/markdown/configuration.md index 54f46b4..2e00f2d 100644 --- a/src/site/markdown/configuration.md +++ b/src/site/markdown/configuration.md @@ -93,9 +93,9 @@ this deadline; see [Timeouts and Errors](timeouts-and-errors.html). ### Keep-alive -While a session is open, the client sends a no-op message (Get Channel Authentication -Capabilities) every `pingPeriod` **milliseconds**, so that the BMC does not close the session for -inactivity during a long collection. +While a session is open, the client sends a no-op message (Get Device ID, as `ipmitool` does) every +`pingPeriod` **milliseconds**, so that the BMC does not close the session for inactivity during a +long collection. | `pingPeriod` | Behavior | | --- | --- | diff --git a/src/site/markdown/installation.md b/src/site/markdown/installation.md index d12c905..036a29e 100644 --- a/src/site/markdown/installation.md +++ b/src/site/markdown/installation.md @@ -57,9 +57,9 @@ What is logged, and at which level: | Level | Messages | | --- | --- | -| `ERROR` | A value the decoders do not know (`Invalid value: ...` for an entity ID, sensor type or unit), a failed session handshake, exceptions in the receiving and keep-alive threads. | -| `WARN` | An SDR record that cannot be decoded and is skipped, a FRU that cannot be read or decoded (the FRU is then truncated or missing), a message that failed and is resent, a packet whose integrity check failed, a response listener that threw while a timeout was reported. | -| `DEBUG` | Each message sent, with its tag and attempt number; each message that timed out; a session that could not be closed cleanly; every lookup of a [`connection.properties`](timeouts-and-errors.html#library-wide-defaults) value. | +| `ERROR` | A failed session handshake, exceptions in the receiving and keep-alive threads. | +| `WARN` | An SDR record that cannot be decoded and is skipped, a FRU that cannot be read or decoded (the FRU is then truncated or missing), a sensor whose reading the BMC refused, a message that failed and is resent, a packet whose integrity check failed, a response listener that threw while a timeout was reported. | +| `DEBUG` | Each message sent, with its tag and attempt number; each message that timed out; a value the decoders do not model (`Invalid value: ...` for an entity ID, sensor type, unit or state); a session that could not be closed cleanly; every lookup of a [`connection.properties`](timeouts-and-errors.html#library-wide-defaults) value. | > [!TIP] > Set the `org.metricshub.ipmi` logger to `WARN` in production, and to `DEBUG` when diagnosing a diff --git a/src/site/markdown/low-level-api.md b/src/site/markdown/low-level-api.md index 277e9b6..8e999a2 100644 --- a/src/site/markdown/low-level-api.md +++ b/src/site/markdown/low-level-api.md @@ -97,11 +97,12 @@ port (or always pass `0`), and call `tearDown()` when you are done with it. | `closeConnection(handle)` | Close and release the connection; the handle is no longer usable. | | `tearDown()` | Close every connection and release the local port. | -The keep-alive is a `Connection.KeepAlive` request, a Get Channel Authentication Capabilities that -the connection queues every `pingPeriod` ms while a session is open. Nobody owns it: its reply is -discarded instead of being delivered to the listeners, and its timeout is not reported. A request -of that class sent by the application gets the same treatment; send a plain -`GetChannelAuthenticationCapabilities` to get the reply. +The keep-alive is a Get Device ID that the connection sends one-way every `pingPeriod` ms while a +session is open. A one-way IPMI message (the keep-alive, or a request sent with +`sendOneWayMessage()`) is queued like any request: its tag stays reserved until its reply arrives +or it times out, so that a late reply cannot be taken for the reply of a later request, and it +takes one of the 8 slots of the message window until then. Its reply is discarded instead of +being delivered to the listeners, and its timeout is not reported. ### Choosing the cipher suite diff --git a/src/site/markdown/sensors.md b/src/site/markdown/sensors.md index 257a4a7..183bb39 100644 --- a/src/site/markdown/sensors.md +++ b/src/site/markdown/sensors.md @@ -44,7 +44,7 @@ Each [`Sensor`](apidocs/org/metricshub/ipmi/client/model/Sensor.html) holds: | `getEntityId()`, `getDeviceId()` | The entity the sensor belongs to: its type (`EntityId.Processor`, `EntityId.PowerSupply`, ...) and instance number | | `isFull()`, `isCompact()` | Whether the record is a Full Sensor record (an analog sensor with a conversion formula and thresholds) or a Compact one (usually a discrete sensor) | | `getRecord()` | The decoded record: a [`FullSensorRecord`](apidocs/org/metricshub/ipmi/core/coding/commands/sdr/record/FullSensorRecord.html) or a [`CompactSensorRecord`](apidocs/org/metricshub/ipmi/core/coding/commands/sdr/record/CompactSensorRecord.html), both [`AbstractSensorRecord`](apidocs/org/metricshub/ipmi/core/coding/commands/sdr/record/AbstractSensorRecord.html) | -| `getData()` | The [`GetSensorReadingResponseData`](apidocs/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.html), or `null` when the BMC has no reading for the sensor (completion code `DataNotPresent`) | +| `getData()` | The [`GetSensorReadingResponseData`](apidocs/org/metricshub/ipmi/core/coding/commands/sdr/GetSensorReadingResponseData.html), or `null` when the BMC returns no reading for the sensor: completion code `DataNotPresent`, or another error completion code, logged at `WARN` ([Errors that do not fail the call](timeouts-and-errors.html#errors-that-do-not-fail-the-call)) | | `getStates()` | The asserted states, as `sensorName=state|sensorName=state...`, or an empty string | ### Readings @@ -99,8 +99,10 @@ CPU0_Status=Presence detected The raw states are available as `getData().getStatesAsserted(record.getSensorType(), record.getEventReadingType())`, a list of [`ReadingType`](apidocs/org/metricshub/ipmi/core/coding/commands/sdr/record/ReadingType.html). -For OEM sensors (event/reading type `0x7F`), whose states the specification does not define, -the state is the raw reading: `sensorName=0xHHLL`. +For OEM sensors (event/reading types `0x70` to `0x7F`), whose states the specification does not +define, the state is the raw value of the state bytes: `sensorName=0xHHLL` (`0xLL` when the BMC +returns a single state byte). An OEM sensor with no state asserted reports no state, like any +discrete sensor. ## Text output format diff --git a/src/site/markdown/supported-commands.md b/src/site/markdown/supported-commands.md index 5418f5b..d1305fb 100644 --- a/src/site/markdown/supported-commands.md +++ b/src/site/markdown/supported-commands.md @@ -17,7 +17,7 @@ ones `IpmiClient` uses. A command not listed here can be added by | Command | Class | NetFn / Cmd | Used by `IpmiClient` | | --- | --- | --- | --- | | **Session** | | | | -| Get Channel Authentication Capabilities | `GetChannelAuthenticationCapabilities` | App / `38h` | every call, and the keep-alive | +| Get Channel Authentication Capabilities | `GetChannelAuthenticationCapabilities` | App / `38h` | every call | | Get Channel Cipher Suites | `GetChannelCipherSuites` | App / `54h` | every call (unless `skipAuth`) | | RMCP+ Open Session | `OpenSession` | (payload) | every call | | RAKP Message 1 / 3 | `Rakp1`, `Rakp3` | (payload) | every call | diff --git a/src/site/markdown/timeouts-and-errors.md b/src/site/markdown/timeouts-and-errors.md index d59b143..e230984 100644 --- a/src/site/markdown/timeouts-and-errors.md +++ b/src/site/markdown/timeouts-and-errors.md @@ -32,18 +32,20 @@ never keep the JVM alive. Below the overall timeout, each message has its own timeout and is retried: 1. The request is sent, and the client waits for the reply up to the **per-message timeout**. -2. Without a reply, the request is sent again, after a random pause of up to `idleTime` - (4 000 ms), up to `retries` (3) times. +2. Without a reply, the request is sent again at once (the BMC already had the whole timeout), up + to `retries` (3) times. 3. When every try failed, the call fails with a `ConnectionException`: `Command timed out` during the session handshake, `Message timed out` in the session. -BMC replies with a *transient* completion code — node busy, out of resources, initialization in -progress, timeout — are retried the same way. Any other error completion code fails at once. +BMC replies with a *transient* completion code are retried too. In the session, the request is +sent again after a random pause of up to `idleTime` (4 000 ms) when the BMC says it is busy (node +busy, out of resources, initialization in progress), and at once after a timeout on the BMC side +(`C3h`), where the BMC already waited for the device it could not reach; the steps of the session +handshake are sent again at once. Any other error completion code fails at once. The per-message timeout is **5 s** by default, and `IpmiClient` caps it by the overall timeout. -With the defaults, a lost reply costs the per-message timeout plus the pause, then the request -is sent again; a BMC that never answers fails after 4 tries, about 20 s (plus the pauses) into -the call. +With the defaults, a lost reply costs the per-message timeout, then the request is sent again; a +BMC that never answers fails after 4 tries, about 20 s into the call. `IpmiClientConfiguration` does not expose the per-message timeout ([#101](https://github.com/metricshub/ipmi-java/issues/101)). To change it: @@ -62,7 +64,7 @@ The defaults come from two properties files packaged in the jar, read through th | --- | --- | --- | --- | | `timeout` | `5000` | Per-message timeout, in ms | When each connection is created | | `retries` | `3` | How many times a failed message is sent again | When each `IpmiConnector` is created | -| `idleTime` | `4000` | Upper bound of the random pause before a retry, in ms | When each `IpmiConnector` is created | +| `idleTime` | `4000` | Upper bound of the random pause before resending a request the BMC answered as busy, in ms | When each `IpmiConnector` is created | | `pingPeriod` | `30000` | Keep-alive period, in ms, when the configuration's `pingPeriod` is `-1` | When each `IpmiConnector` is created | Override them at application startup, from a single thread, before the first IPMI call: the @@ -97,7 +99,7 @@ Common causes wrapped in the `ExecutionException`: | `IllegalArgumentException: Authentication check failed` | The RAKP handshake failed: the BMC's proof does not match the password or the [BMC key](configuration.html#bmc-key). The credentials are sent once. | | `IPMIException: Unauthorized name.`, `Invalid role.`, ... | The BMC refused the session: unknown user, user not allowed over the LAN channel or at the User level, cipher suite refused ([Troubleshooting](troubleshooting.html#the-login-fails)). | | `ConnectionException: Command timed out` / `Message timed out` | No reply after all the [tries](#per-message-timeout-and-retries) of a message: `Command timed out` during the session handshake, the usual symptom of a wrong host, a closed UDP port or IPMI over LAN disabled; `Message timed out` in the session. | -| `IPMIException` | The BMC answered with an error completion code. `getCompletionCode()` returns it, for example `InsufficientPrivilege` (`0xD4`). | +| `IPMIException` | The BMC answered with an error completion code. `getCompletionCode()` returns it, for example `InsufficentPrivilege` (sic, `0xD4`). | | `IllegalArgumentException: ... is not yet implemented.` | The chosen cipher suite uses an algorithm the client does not implement (xRC4, MD5-128). See [cipher suites](preparing-the-bmc.html#cipher-suites). | | `Exception: Cannot get the available cipher suites.` | The BMC returned an empty cipher suite list. | | `UnknownHostException` | The host name cannot be resolved. | @@ -113,9 +115,10 @@ Some problems are logged at the `WARN` level and the call goes on with what it c ([Supported Commands](supported-commands.html#oem-and-unknown-records)); * a **FRU** that cannot be read (for example a FRU device that is not present) is reported truncated or not at all ([FRU Inventory](fru-inventory.html#how-the-frus-are-read)); -* a **sensor** whose reading is not available (completion code `DataNotPresent`) is returned - without reading data. +* a **sensor** whose reading is not available (completion code `DataNotPresent`, not logged) or + refused with another completion code is returned without reading data. -Unknown values in a record (an entity ID, a sensor type or a unit the library does not know) are -logged at the `ERROR` level as `Invalid value: ...` and replaced with a default (`Other` for an -entity ID); the record is still decoded. +Values a record may carry but the library does not model (an OEM or chassis-specific entity ID, a +reserved device or sensor type, a state the reading type does not define) are logged at the +`DEBUG` level as `Invalid value: ...` and replaced with a default (`Other` for an entity ID); the +record is still decoded. diff --git a/src/site/markdown/troubleshooting.md b/src/site/markdown/troubleshooting.md index 4045cab..d51fdc4 100644 --- a/src/site/markdown/troubleshooting.md +++ b/src/site/markdown/troubleshooting.md @@ -64,6 +64,13 @@ one, check: With the [low-level API](low-level-api.html#privilege-level), open the session with the level the command needs (Operator for Chassis Control, Administrator for configuration commands). +When commands that worked earlier in the same session start failing with `0xD4`, the BMC revoked +the session. HP iLO 5 does so 120 s after the session opened, whatever its privilege level, and +can do so after 60 s when a Get Channel Authentication Capabilities or a Set Session Privilege +Level command is sent during the session (the library's keep-alive is a Get Device ID for that +reason). Each `IpmiClient` call opens its own session; with the low-level API, open a new session +for work that lasts longer. + ## `... is not yet implemented.` `IllegalArgumentException: Confidentiality algorithm XRC4-128 is not yet implemented.` (or diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index b47d005..6eb55d2 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -24,6 +24,10 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world is 5 s by default (it was 5 minutes) and capped by the overall timeout, a retried message waits for the reply of the resent request, and the handshake steps wait for the elapsed time rather than a number of sleeps ([Timeouts and Errors](timeouts-and-errors.html)); +* a request is sent again at once after a lost reply or a timeout on the BMC side (`C3h`), where + 1.2.02 paused for a random time of up to `idleTime` (4 s) before each resend: a collection on a + Dell iDRAC 8, which answers `C3h` for its absent FRUs and loses a few replies, took over two + minutes; the pause now only follows the completion codes that say the BMC is busy; * a BMC that never answers now fails the session handshake with `ExecutionException` wrapping `ConnectionException: Command timed out`, about 20 s into the call, where 1.2.02 threw `TimeoutException` at the overall timeout; @@ -39,9 +43,19 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world reply is sent again; * the [keep-alive](configuration.html#keep-alive) is actually sent: with the default `pingPeriod` (`-1`) 1.2.02 sent no keep-alive at all, so a session could expire during a long collection; the - keep-alive is now one message every 30 s by default, whose reply is discarded, and a - Get Channel Authentication Capabilities command sent by the application in a session gets its - reply (1.2.02 dropped it, as it did the keep-alive replies); + keep-alive is now a Get Device ID every 30 s by default, whose reply is discarded (not the Get + Channel Authentication Capabilities of 1.2.02, after which HP iLO 5 revokes the session within + 60 s), and a Get Channel Authentication Capabilities command sent by the application in a + session gets its reply (1.2.02 dropped it, as it did the keep-alive replies); +* a one-way IPMI message (`IpmiConnector.sendOneWayMessage()`, `IpmiAsyncConnector.sendMessage()` + with `isOneWay`) is queued like any request: its tag stays reserved, and it takes a slot of the + 8-message window, until its reply arrives or it times out, where 1.2.02 could reuse the tag at + once and take a late reply for the reply of a later request; its reply and its timeout are + still not reported (the Serial over LAN acknowledgements, which the BMC never answers, are not + queued); +* a sensor whose Get Sensor Reading fails with an error completion code is returned without + reading, and a `WARN` names it, where 1.2.02 failed the whole call (it tolerated + `DataNotPresent` only); * `IpmiConnector.closeConnection()` releases the connection: its handle then throws `IllegalStateException` instead of addressing a disconnected connection, and a session that fails to be established by `SerialOverLan` closes its own connection instead of tearing down the @@ -86,7 +100,18 @@ The decoders follow the IPMI 2.0 and FRU specifications more closely; the visibl Info fails, returns FRU 0 once, and stops reading a FRU at its first unreadable chunk instead of shifting the following chunks into the gap; * reserved values of the rate unit, modifier unit usage and power restore policy decode to - `None` or `Unknown` instead of throwing. + `None` or `Unknown` instead of throwing; +* the sensors of the OEM event/reading types `70h` to `7Eh` (Cisco IMC, Dell iDRAC and Fujitsu iRMC + use them for presence and module sensors) report the raw value of their state bytes + (`sensorName=0xHHLL`) like those of `7Fh`, where 1.2.02 reported `sensorName=Unknown`; an OEM + sensor (`70h` to `7Fh`) with no state asserted reports no state, where 1.2.02 reported the + state bytes of a `7Fh` one; `ReadingType.parseInt()`, and so `getStatesAsserted()` and the event + of a SEL record, returns `UnknownOEMEvent` for the states of `70h` to `7Fh`, where 1.2.02 + returned `Unknown` unless the sensor type was OEM; the new `ReadingType.isOem()` tells these + types apart; +* a value the decoders do not model (an OEM or chassis-specific entity ID, a reserved device or + sensor type, a state the reading type does not define) is logged at `DEBUG` instead of `ERROR` + or `WARN`. Code that **uses the low-level API** to read FRUs: the `offset` and `countToRead` arguments of the `ReadFruData` constructors are now **in bytes** whatever the access unit of the device, and diff --git a/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java b/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java index c360586..59afc7e 100644 --- a/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java +++ b/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java @@ -1,14 +1,26 @@ package org.metricshub.ipmi.client.runner; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import java.net.InetAddress; +import java.util.concurrent.atomic.AtomicReference; + import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.client.IpmiClientConfiguration; import org.metricshub.ipmi.client.Utils; +import org.metricshub.ipmi.core.api.async.ConnectionHandle; +import org.metricshub.ipmi.core.api.sync.IpmiConnector; +import org.metricshub.ipmi.core.coding.PayloadCoder; +import org.metricshub.ipmi.core.coding.commands.ResponseData; import org.metricshub.ipmi.core.coding.commands.sdr.GetSensorReadingResponseData; import org.metricshub.ipmi.core.coding.commands.sdr.record.CompactSensorRecord; import org.metricshub.ipmi.core.coding.commands.sdr.record.FullSensorRecord; import org.metricshub.ipmi.core.coding.commands.sdr.record.SensorType; +import org.metricshub.ipmi.core.coding.payload.CompletionCode; +import org.metricshub.ipmi.core.coding.payload.lan.IPMIException; +import org.metricshub.ipmi.core.connection.ConnectionException; class GetSensorsRunnerTest { @@ -35,6 +47,7 @@ void testBuildStates() { final byte[] raw = { 1, 2, 3, 127 }; final GetSensorReadingResponseData data = validReading(); data.setRaw(raw); + data.setStatesAsserted(new boolean[] { true, true }); final CompactSensorRecord record = new CompactSensorRecord(); record.setName(DEVICE_NAME); record.setEventReadingType(OEM_EVENT_READING_TYPE); @@ -61,6 +74,7 @@ void testBuildStates() { final byte[] raw = { 1, 2, 3, 127 }; final GetSensorReadingResponseData data = validReading(); data.setRaw(raw); + data.setStatesAsserted(new boolean[] { true, true }); final FullSensorRecord record = new FullSensorRecord(); record.setName(DEVICE_NAME); record.setEventReadingType(OEM_EVENT_READING_TYPE); @@ -83,6 +97,68 @@ void testBuildStates() { } } + @Test + void everyOemEventReadingTypeReportsTheRawStates() { + // Cisco IMC and Dell iDRAC use 70h for their Entity Presence and Module/Board sensors + for (int eventReadingType = 0x70; eventReadingType <= 0x7f; eventReadingType++) { + final GetSensorReadingResponseData data = validReading(); + data.setRaw(new byte[] { 1, 2, 3, 0 }); + data.setStatesAsserted(new boolean[] { true, true }); + final CompactSensorRecord record = new CompactSensorRecord(); + record.setName(DEVICE_NAME); + record.setSensorType(SensorType.EntityPresence); + record.setEventReadingType(eventReadingType); + + assertEquals("name=0x0003", GetSensorsRunner.buildStates(data, record), Integer.toHexString(eventReadingType)); + + // No state asserted (bit 7 of byte 4 is reserved and set): no state, as for any discrete sensor + final GetSensorReadingResponseData idle = validReading(); + idle.setRaw(new byte[] { 1, 2, 0, (byte) 0x80 }); + idle.setStatesAsserted(new boolean[15]); + assertEquals(Utils.EMPTY, GetSensorsRunner.buildStates(idle, record), Integer.toHexString(eventReadingType)); + } + } + + @Test + void aRefusedReadingCostsOnlyThatSensor() throws Exception { + final AtomicReference failure = new AtomicReference<>(); + final IpmiConnector connector = new IpmiConnector(0, 0) { + @Override + public ResponseData sendMessage(ConnectionHandle handle, PayloadCoder request) throws Exception { + throw failure.get(); + } + }; + try { + final GetSensorsRunner runner = new GetSensorsRunner( + new IpmiClientConfiguration("bmc", "user", new char[0], null, false, 1)) { + @Override + protected IpmiConnector getConnector() { + return connector; + } + + @Override + protected ConnectionHandle getHandle() { + return new ConnectionHandle(0, InetAddress.getLoopbackAddress(), 623); + } + }; + final CompactSensorRecord record = new CompactSensorRecord(); + record.setName(DEVICE_NAME); + + // HP iLO answers D4h once it revoked the session + failure.set(new IPMIException(CompletionCode.InsufficentPrivilege)); + assertNull(runner.getSensorRecordReading(record)); + + failure.set(new IPMIException(CompletionCode.DataNotPresent)); + assertNull(runner.getSensorRecordReading(record)); + + // A BMC that does not answer fails the walk + failure.set(new ConnectionException("Message timed out")); + assertThrows(ConnectionException.class, () -> runner.getSensorRecordReading(record)); + } finally { + connector.tearDown(); + } + } + @Test void statesOfAnUnavailableOrUnscannedSensorAreNotReported() { final boolean[] statesAsserted = { true, false }; diff --git a/src/test/java/org/metricshub/ipmi/core/api/sync/IpmiConnectorTest.java b/src/test/java/org/metricshub/ipmi/core/api/sync/IpmiConnectorTest.java index e5cd670..6eb957f 100644 --- a/src/test/java/org/metricshub/ipmi/core/api/sync/IpmiConnectorTest.java +++ b/src/test/java/org/metricshub/ipmi/core/api/sync/IpmiConnectorTest.java @@ -2,11 +2,31 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.lang.reflect.Field; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.TimeUnit; +import java.util.function.Function; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; import org.metricshub.ipmi.core.api.async.ConnectionHandle; +import org.metricshub.ipmi.core.api.async.IpmiAsyncConnector; +import org.metricshub.ipmi.core.coding.commands.IpmiVersion; +import org.metricshub.ipmi.core.coding.commands.PrivilegeLevel; +import org.metricshub.ipmi.core.coding.commands.chassis.GetChassisStatus; +import org.metricshub.ipmi.core.coding.protocol.AuthenticationType; +import org.metricshub.ipmi.core.coding.security.CipherSuite; +import org.metricshub.ipmi.core.common.PropertiesManager; +import org.metricshub.ipmi.core.connection.Connection; import org.metricshub.ipmi.core.connection.ConnectionException; +import org.metricshub.ipmi.core.connection.ConnectionManager; +import org.metricshub.ipmi.core.sm.StateMachine; +import org.metricshub.ipmi.core.sm.states.SessionValid; import org.metricshub.ipmi.core.transport.FakeBmc; class IpmiConnectorTest { @@ -35,6 +55,21 @@ class IpmiConnectorTest { private static final int TIMEOUT_MS = 500; + /** The session ID of the console in the sessions the tests fake. */ + private static final int SESSION_ID = 1; + + /** Offset of the IPMI payload in an RMCP+ datagram of cipher suite 0. */ + private static final int PAYLOAD_OFFSET = 16; + + /** Get Chassis Status response data: power on, no last power event, no chassis state flag. */ + private static final byte[] CHASSIS_STATUS = { 0x01, 0x00, 0x00 }; + + /** + * Long enough for a pause before a resend to show: up to 20 s, where a resend without pause comes within the + * message timeout plus one tick of the queue timer. + */ + private static final String LONG_IDLE_TIME = "20000"; + @Test void aBadReplyFailsTheStepAtOnceAndLeavesItRetriable() throws Exception { try (FakeBmc bmc = new FakeBmc(request -> TRUNCATED_REPLY)) { @@ -56,4 +91,117 @@ void aBadReplyFailsTheStepAtOnceAndLeavesItRetriable() throws Exception { } } } + + @Test + @Timeout(30) + void aLostReplyIsSentAgainWithoutPause() throws Exception { + // The BMC drops the first request and answers the second one + long gapMs = gapBetweenTwoTries(tries -> tries == 1 ? null : 0x00); + assertTrue(gapMs < TIMEOUT_MS + 1500, "the request was sent again after " + gapMs + " ms"); + } + + @Test + @Timeout(30) + void aTimeoutOnTheBmcSideIsSentAgainWithoutPause() throws Exception { + // C3h, "Timeout while processing command": the BMC already waited for the device it could not reach + long gapMs = gapBetweenTwoTries(tries -> tries == 1 ? 0xc3 : 0x00); + assertTrue(gapMs < 1000, "the request was sent again after " + gapMs + " ms"); + } + + /** + * Sends a Get Chassis Status in a faked session to a BMC that answers each try with the given completion code (or + * not at all, for null), and returns the time between the first two tries. + */ + private static long gapBetweenTwoTries(Function completionCodeOfTry) throws Exception { + PropertiesManager properties = PropertiesManager.getInstance(); + String idleTime = properties.getProperty("idleTime"); + properties.setProperty("idleTime", LONG_IDLE_TIME); + List arrivals = new CopyOnWriteArrayList<>(); + try (FakeBmc bmc = new FakeBmc(request -> { + arrivals.add(System.nanoTime()); + Integer completionCode = completionCodeOfTry.apply(arrivals.size()); + return completionCode == null ? null : sessionReply(request, completionCode, CHASSIS_STATUS); + })) { + // No keep-alive: the BMC sees the requests of the test only + IpmiConnector connector = new IpmiConnector(0, 0); + try { + ConnectionHandle handle = connector + .createConnection(bmc.getAddress(), bmc.getPort(), CipherSuite.getEmpty(), PrivilegeLevel.User); + connector.setTimeout(handle, TIMEOUT_MS); + openSession(connector, handle); + + assertNotNull( + connector + .sendMessage( + handle, + new GetChassisStatus(IpmiVersion.V20, CipherSuite.getEmpty(), AuthenticationType.RMCPPlus))); + assertEquals(2, arrivals.size(), "the request must be sent twice"); + return TimeUnit.NANOSECONDS.toMillis(arrivals.get(1) - arrivals.get(0)); + } finally { + connector.tearDown(); + } + } finally { + properties.setProperty("idleTime", idleTime); + } + } + + /** Puts the connection in a session of cipher suite 0, as if the handshake had succeeded. */ + private static void openSession(IpmiConnector connector, ConnectionHandle handle) throws Exception { + IpmiAsyncConnector asyncConnector = (IpmiAsyncConnector) field(IpmiConnector.class, "asyncConnector") + .get(connector); + ConnectionManager manager = (ConnectionManager) field(IpmiAsyncConnector.class, "connectionManager") + .get(asyncConnector); + Connection connection = manager.getConnection(handle.getHandle()); + ((StateMachine) field(Connection.class, "stateMachine").get(connection)) + .setCurrent(new SessionValid(CipherSuite.getEmpty(), SESSION_ID)); + } + + private static Field field(Class type, String name) throws NoSuchFieldException { + Field field = type.getDeclaredField(name); + field.setAccessible(true); + return field; + } + + /** + * The reply of a BMC to an IPMI request sent in a session of cipher suite 0 (neither authenticated nor + * encrypted): same tag and command, response network function, given completion code and data. + */ + private static byte[] sessionReply(byte[] request, int completionCode, byte[] data) { + byte[] payload = new byte[8 + data.length]; + payload[0] = (byte) 0x81; + payload[1] = (byte) ((((request[PAYLOAD_OFFSET + 1] & 0xff) >> 2) + 1) << 2); + payload[2] = (byte) -(payload[0] + payload[1]); + payload[3] = 0x20; + payload[4] = request[PAYLOAD_OFFSET + 4]; + payload[5] = request[PAYLOAD_OFFSET + 5]; + payload[6] = (byte) completionCode; + System.arraycopy(data, 0, payload, 7, data.length); + byte checksum = 0; + for (int i = 3; i < payload.length - 1; i++) { + checksum += payload[i]; + } + payload[payload.length - 1] = (byte) -checksum; + + byte[] reply = new byte[PAYLOAD_OFFSET + payload.length]; + byte[] header = { + 0x06, + 0x00, + (byte) 0xff, + 0x07, + 0x06, + 0x00, + SESSION_ID, + 0x00, + 0x00, + 0x00, + 0x01, + 0x00, + 0x00, + 0x00, + (byte) payload.length, + 0x00 }; + System.arraycopy(header, 0, reply, 0, PAYLOAD_OFFSET); + System.arraycopy(payload, 0, reply, PAYLOAD_OFFSET, payload.length); + return reply; + } } diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ReadingTypeTest.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ReadingTypeTest.java new file mode 100644 index 0000000..65d9347 --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/sdr/record/ReadingTypeTest.java @@ -0,0 +1,30 @@ +package org.metricshub.ipmi.core.coding.commands.sdr.record; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class ReadingTypeTest { + + @Test + void eventReadingTypes70hTo7FhAreOem() { + assertFalse(ReadingType.isOem(0x6f), "sensor-specific"); + assertTrue(ReadingType.isOem(0x70)); + assertTrue(ReadingType.isOem(0x7f)); + assertFalse(ReadingType.isOem(0x80), "reserved"); + } + + @Test + void statesOfAnOemEventReadingTypeAreUnknownOemEvents() { + assertEquals(ReadingType.UnknownOEMEvent, ReadingType.parseInt(SensorType.EntityPresence, 0x70, 3)); + assertEquals(ReadingType.UnknownOEMEvent, ReadingType.parseInt(SensorType.ModuleBoard, 0x75, 0)); + } + + @Test + void anUndefinedStateIsUnknown() { + // HP iLO asserts state 6 of the Device Present reading type (08h), which defines states 0 and 1 + assertEquals(ReadingType.Unknown, ReadingType.parseInt(SensorType.Fan, 0x08, 6)); + } +} diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java index ef05066..1a1ff1a 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java @@ -205,7 +205,7 @@ public void processRequest(IpmiPayload payload) { } @Test - void theKeepAliveReplyIsDiscardedAndFreesItsTag() throws Exception { + void theKeepAliveIsAOneWayGetDeviceIdWhoseReplyIsDiscarded() throws Exception { Connection connection = connect(TIMEOUT_MS); try { openSession(connection); @@ -223,11 +223,11 @@ public void processRequest(IpmiPayload payload) { }); connection.run(); int keepAliveTag = 1; // the first tag of a new connection - // The keep-alive reply (command 38h) is not delivered, and its tag is free again - connection.notify(new MessageAction(reply(keepAliveTag, (byte) 0x38))); + // The keep-alive reply (Get Device ID, command 01h) is not delivered + connection.notify(new MessageAction(reply(keepAliveTag, (byte) 0x01))); assertEquals(-1, notifiedTag.get(), "the keep-alive reply must not reach the listeners"); - // The same command sent by the application gets its reply + // A request sent by the application gets its reply GetChannelAuthenticationCapabilities request = new GetChannelAuthenticationCapabilities( IpmiVersion.V20, IpmiVersion.V20, @@ -284,7 +284,7 @@ public void processRequest(IpmiPayload payload) { (byte) 0xe); int tag = connection.sendMessage(request, false); - // A late reply to a one-way Get Device ID (command 01h) whose tag was reused by the request + // A late reply to a Get Device ID (command 01h) that timed out and whose tag was reused by the request connection.notify(new MessageAction(reply(tag, (byte) 0x01))); assertEquals(-1, notifiedTag.get(), "a reply to another command must not answer the queued request"); diff --git a/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java b/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java index 2dd2ee8..24cb0f9 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Arrays; @@ -137,17 +138,41 @@ void oneWaySequenceNumbersSkipTheQueuedTags() { } @Test - void aTimedOutKeepAliveIsNotReported() throws Exception { + void aTimedOutOneWayMessageIsNotReported() throws Exception { MessageQueue queue = newQueue(); try { connection.registerListener(recorder()); - int keepAlive = queue.add(new Connection.KeepAlive(CipherSuite.getEmpty())); + int oneWay = queue.add(request(), true); int request = queue.add(request()); Thread.sleep(TIMER_TICK_MS); - assertFalse(queue.containsId(keepAlive), "the keep-alive must leave the queue"); + assertFalse(queue.containsId(oneWay), "the one-way message must leave the queue"); assertEquals(Collections.singletonList(request + ":Message timed out"), reported); } finally { queue.tearDown(); } } + + @Test + void aOneWayMessageHoldsItsTagAndItsSlotUntilItsReply() { + MessageQueue queue = newQueue(); + try { + int oneWay = queue.add(request(), true); + assertTrue(queue.getElement(oneWay).isOneWay()); + // The rest of the window, with requests answered at once + for (int i = 1; i < WINDOW_SIZE; i++) { + int tag = queue.add(request()); + assertTrue(tag > 0 && tag != oneWay, "add " + i + " took tag " + tag); + assertFalse(queue.getElement(tag).isOneWay()); + queue.remove(tag); + } + assertEquals(-1, queue.add(request()), "the window slides only once the one-way message is answered"); + + // Its reply frees the tag and the window + queue.remove(oneWay); + assertNull(queue.getElement(oneWay)); + assertTrue(queue.add(request()) > 0); + } finally { + queue.tearDown(); + } + } } From 2673350e4e716ed9769eae9151d91ec49ea2b12c Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 20:13:07 +0200 Subject: [PATCH 2/3] Pin the one-way routing in tests; correct the review findings in the docs Second review pass on the fixes: - ConnectionTest checks that the keep-alive is queued as a one-way Get Device ID and released by its reply, and that one-way IPMI messages are queued while SOL ACK-only packets are not (it fails without the SolMessageHandler.takeTag override). - The one-way queue test uses a long timeout, so a slow machine cannot release the slot; the no-pause tests use a one-hour idleTime, so a reintroduced pause cannot pass by chance. - upgrading.md attributes the two-minute iDRAC 8 collections to the pauses combined with the 5 s timeout, and states that 1.2.02 did not reserve one-way tags; troubleshooting.md lists the per-sensor WARN and quotes the actual D4h message; the retry Javadoc counts the tries right. Co-Authored-By: Claude Opus 5.5 --- .../ipmi/core/api/sync/IpmiConnector.java | 4 +- src/site/markdown/troubleshooting.md | 3 +- src/site/markdown/upgrading.md | 12 ++-- .../ipmi/core/api/sync/IpmiConnectorTest.java | 7 ++- .../ipmi/core/connection/ConnectionTest.java | 55 ++++++++++++++++++- .../connection/queue/MessageQueueTest.java | 7 ++- 6 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/core/api/sync/IpmiConnector.java b/src/main/java/org/metricshub/ipmi/core/api/sync/IpmiConnector.java index 814f144..946221a 100644 --- a/src/main/java/org/metricshub/ipmi/core/api/sync/IpmiConnector.java +++ b/src/main/java/org/metricshub/ipmi/core/api/sync/IpmiConnector.java @@ -434,8 +434,8 @@ private ResponseData sendThroughAsyncConnector( } /** - * Throws the exception when the request was tried {@link #retries} times already, otherwise lets the caller send - * it again, after a random pause of up to {@link #idleTime} ms if asked. + * Throws the exception when the request was already sent again {@link #retries} times (retries + 1 tries), + * otherwise lets the caller send it again, after a random pause of up to {@link #idleTime} ms if asked. */ private void handleRetriesWhenException(int tries, Exception e, boolean pause) throws Exception { if (tries > retries) { diff --git a/src/site/markdown/troubleshooting.md b/src/site/markdown/troubleshooting.md index d51fdc4..6e19fe0 100644 --- a/src/site/markdown/troubleshooting.md +++ b/src/site/markdown/troubleshooting.md @@ -51,7 +51,7 @@ sent once; the `ExecutionException` wraps the reason: Check the account with `ipmitool -I lanplus ... -L USER chassis status`: `ipmitool` reports `RAKP 2 HMAC is invalid` for a wrong password and `unauthorized name` for an unknown user. -## `IPMIException: Insufficient privilege level` (`0xD4`) +## `IPMIException: Cannot execute command due to insufficient privilege level ...` (`0xD4`) The BMC refused a command at the session's privilege level. `IpmiClient` always uses the **User** level, which the specification allows for every command it sends; if a BMC refuses @@ -83,6 +83,7 @@ to make it use suite 3 or 17. | Symptom | Cause | | --- | --- | | `WARN Skipping SDR record ...` | A record that cannot be decoded is skipped; the other sensors are still returned. [SDR records](supported-commands.html#sdr-records) lists what is decoded. | +| `WARN Failed to read sensor () on : ...` | The BMC refused the Get Sensor Reading of that sensor with an error completion code: the sensor is returned without reading or states, and the other sensors are still returned. With the `0xD4` message, the BMC may have revoked the session (see the `0xD4` section above). | | `WARN Failed to read FRU at offset , the FRU data is truncated there: Requested Sensor, data, or record not present` | The FRU is declared in the SDR repository but not present, for example an empty power supply bay. Usually harmless: the reading stops there and the areas read so far are decoded. | | `WARN Failed to read FRU ` | The BMC did not answer Get FRU Inventory Area Info for that FRU, or its data is not in the IPMI FRU format (for example the SPD data of a memory module, [#107](https://github.com/metricshub/ipmi-java/issues/107)). The other FRUs are still returned. | | `WARN The info area at offset is truncated: skipped` | The read stopped before the end of that area (see above): the complete areas of the FRU are still returned. | diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index 6eb55d2..690810b 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -25,9 +25,10 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world for the reply of the resent request, and the handshake steps wait for the elapsed time rather than a number of sleeps ([Timeouts and Errors](timeouts-and-errors.html)); * a request is sent again at once after a lost reply or a timeout on the BMC side (`C3h`), where - 1.2.02 paused for a random time of up to `idleTime` (4 s) before each resend: a collection on a - Dell iDRAC 8, which answers `C3h` for its absent FRUs and loses a few replies, took over two - minutes; the pause now only follows the completion codes that say the BMC is busy; + 1.2.02 paused for a random time of up to `idleTime` (4 s) before each resend; with the 5 s + message timeout, these pauses made a collection on a Dell iDRAC 8, which answers `C3h` for its + absent FRUs and loses a few replies, take over two minutes; the pause now only follows the + completion codes that say the BMC is busy; * a BMC that never answers now fails the session handshake with `ExecutionException` wrapping `ConnectionException: Command timed out`, about 20 s into the call, where 1.2.02 threw `TimeoutException` at the overall timeout; @@ -49,8 +50,9 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world session gets its reply (1.2.02 dropped it, as it did the keep-alive replies); * a one-way IPMI message (`IpmiConnector.sendOneWayMessage()`, `IpmiAsyncConnector.sendMessage()` with `isOneWay`) is queued like any request: its tag stays reserved, and it takes a slot of the - 8-message window, until its reply arrives or it times out, where 1.2.02 could reuse the tag at - once and take a late reply for the reply of a later request; its reply and its timeout are + 8-message window, until its reply arrives or it times out, where 1.2.02 did not reserve the tag, + so it could be handed out again while the reply was still on its way and a late reply taken for + the reply of a later request; its reply and its timeout are still not reported (the Serial over LAN acknowledgements, which the BMC never answers, are not queued); * a sensor whose Get Sensor Reading fails with an error completion code is returned without diff --git a/src/test/java/org/metricshub/ipmi/core/api/sync/IpmiConnectorTest.java b/src/test/java/org/metricshub/ipmi/core/api/sync/IpmiConnectorTest.java index 6eb957f..93166fe 100644 --- a/src/test/java/org/metricshub/ipmi/core/api/sync/IpmiConnectorTest.java +++ b/src/test/java/org/metricshub/ipmi/core/api/sync/IpmiConnectorTest.java @@ -65,10 +65,11 @@ class IpmiConnectorTest { private static final byte[] CHASSIS_STATUS = { 0x01, 0x00, 0x00 }; /** - * Long enough for a pause before a resend to show: up to 20 s, where a resend without pause comes within the - * message timeout plus one tick of the queue timer. + * Long enough for any pause before a resend to show: a random pause of up to one hour either shows in the gap or + * trips the timeout of the test, where a resend without pause comes within the message timeout plus one tick of + * the queue timer. */ - private static final String LONG_IDLE_TIME = "20000"; + private static final String LONG_IDLE_TIME = "3600000"; @Test void aBadReplyFailsTheStepAtOnceAndLeavesItRetriable() throws Exception { diff --git a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java index 1a1ff1a..a7f2b61 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -33,7 +34,14 @@ import org.metricshub.ipmi.core.sm.states.SessionValid; import org.metricshub.ipmi.core.transport.UdpMessage; import java.util.List; +import java.util.Map; import java.util.concurrent.CountDownLatch; +import org.metricshub.ipmi.core.coding.commands.IpmiCommandCoder; +import org.metricshub.ipmi.core.coding.payload.lan.NetworkFunction; +import org.metricshub.ipmi.core.coding.payload.sol.SolAckState; +import org.metricshub.ipmi.core.coding.sol.SolCoder; +import org.metricshub.ipmi.core.connection.queue.MessageQueue; +import org.metricshub.ipmi.core.connection.queue.QueueElement; import org.metricshub.ipmi.core.coding.commands.session.GetChannelCipherSuitesResponseData; import org.metricshub.ipmi.core.sm.actions.ResponseAction; @@ -223,9 +231,18 @@ public void processRequest(IpmiPayload payload) { }); connection.run(); int keepAliveTag = 1; // the first tag of a new connection - // The keep-alive reply (Get Device ID, command 01h) is not delivered + MessageQueue queue = handler(connection, PayloadType.Ipmi).getMessageQueue(); + QueueElement keepAlive = queue.getElement(keepAliveTag); + assertNotNull(keepAlive, "the keep-alive must hold its tag in the queue"); + assertTrue(keepAlive.isOneWay()); + IpmiCommandCoder command = (IpmiCommandCoder) keepAlive.getRequest(); + assertEquals(NetworkFunction.ApplicationRequest, command.getNetworkFunction()); + assertEquals(0x01, command.getCommandCode(), "Get Device ID"); + + // The keep-alive reply is not delivered, and frees the tag connection.notify(new MessageAction(reply(keepAliveTag, (byte) 0x01))); assertEquals(-1, notifiedTag.get(), "the keep-alive reply must not reach the listeners"); + assertNull(queue.getElement(keepAliveTag)); // A request sent by the application gets its reply GetChannelAuthenticationCapabilities request = new GetChannelAuthenticationCapabilities( @@ -243,6 +260,42 @@ public void processRequest(IpmiPayload payload) { } } + @Test + void oneWayIpmiMessagesAreQueuedButNotTheSolAcknowledgements() throws Exception { + Connection connection = connect(60000); + try { + MessageHandler ipmi = handler(connection, PayloadType.Ipmi); + int tag = ipmi + .takeTag( + new GetChannelAuthenticationCapabilities( + IpmiVersion.V20, + IpmiVersion.V20, + CipherSuite.getEmpty(), + PrivilegeLevel.Callback, + (byte) 0xe), + true); + assertTrue(ipmi.getMessageQueue().getElement(tag).isOneWay()); + + // The BMC never acknowledges an ACK-only SOL packet: a stream of them must not fill the 8-slot window + MessageHandler sol = handler(connection, PayloadType.Sol); + for (int i = 0; i < 32; i++) { + int solTag = sol + .takeTag(new SolCoder((byte) (i % 15 + 1), (byte) 1, SolAckState.ACK, CipherSuite.getEmpty()), true); + assertTrue(solTag > 0, "ACK " + i + " got no tag"); + assertNull(sol.getMessageQueue().getElement(solTag), "ACK " + i + " was queued"); + } + } finally { + connection.disconnect(); + } + } + + @SuppressWarnings("unchecked") + private static MessageHandler handler(Connection connection, PayloadType payloadType) throws Exception { + Field field = Connection.class.getDeclaredField("messageHandlers"); + field.setAccessible(true); + return ((Map) field.get(connection)).get(payloadType); + } + /** A minimal Application (07h) IPMI LAN response with the given tag (rqSeq, bits 7:2 of byte 4) and command. */ private static Ipmiv20Message reply(int tag, byte command) { return reply(tag, (byte) 0x07, command); diff --git a/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java b/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java index 24cb0f9..ce4d4cd 100644 --- a/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java +++ b/src/test/java/org/metricshub/ipmi/core/connection/queue/MessageQueueTest.java @@ -154,7 +154,12 @@ void aTimedOutOneWayMessageIsNotReported() throws Exception { @Test void aOneWayMessageHoldsItsTagAndItsSlotUntilItsReply() { - MessageQueue queue = newQueue(); + // Only the reply may release the one-way message here, not a timeout on a slow machine + MessageQueue queue = new MessageQueue( + connection, + 60000, + IpmiLanMessage.MIN_SEQUENCE_NUMBER, + IpmiLanMessage.MAX_SEQUENCE_NUMBER); try { int oneWay = queue.add(request(), true); assertTrue(queue.getElement(oneWay).isOneWay()); From 49c62322555ab4abf623b09dcaeaaa7b3ca80f1f Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 9 Oct 2026 21:38:31 +0200 Subject: [PATCH 3/3] Drop the unverified iLO 5 session-lifetime claims Further lab runs with a dedicated iLO account showed that the D4h comes from sessions deleted by another client of the lab iLO (a management tool that deletes the IPMI sessions every two minutes): the iLO applies the deletion at a whole minute of the session's age, whatever the keep-alive command (a Get Device ID keep-alive session was revoked at 121 s too). The keep-alive stays a Get Device ID, as ipmitool sends, without the claim that Get Channel Authentication Capabilities shortens iLO sessions; troubleshooting describes the revoked-session symptom instead. Co-Authored-By: Claude Opus 5.5 --- .../metricshub/ipmi/core/connection/Connection.java | 3 +-- src/site/markdown/troubleshooting.md | 11 +++++------ src/site/markdown/upgrading.md | 5 ++--- .../ipmi/client/runner/GetSensorsRunnerTest.java | 2 +- 4 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java index 97d1fd6..fdf5791 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java @@ -687,8 +687,7 @@ public void notify(StateMachineAction action) { } /** - * The keep-alive request: a Get Device ID, as ipmitool sends. HP iLO 5 revokes the privileges of a session 60 s - * after a Get Channel Authentication Capabilities or a Set Session Privilege Level sent in it, instead of 120 s. + * The keep-alive request: a Get Device ID, as ipmitool sends. */ private static final class KeepAlive extends IpmiCommandCoder { diff --git a/src/site/markdown/troubleshooting.md b/src/site/markdown/troubleshooting.md index 6e19fe0..d68a58f 100644 --- a/src/site/markdown/troubleshooting.md +++ b/src/site/markdown/troubleshooting.md @@ -64,12 +64,11 @@ one, check: With the [low-level API](low-level-api.html#privilege-level), open the session with the level the command needs (Operator for Chassis Control, Administrator for configuration commands). -When commands that worked earlier in the same session start failing with `0xD4`, the BMC revoked -the session. HP iLO 5 does so 120 s after the session opened, whatever its privilege level, and -can do so after 60 s when a Get Channel Authentication Capabilities or a Set Session Privilege -Level command is sent during the session (the library's keep-alive is a Get Device ID for that -reason). Each `IpmiClient` call opens its own session; with the low-level API, open a new session -for work that lasts longer. +When commands that worked earlier in the same session start failing with `0xD4`, the session was +revoked, and sending the command again in the same session does not help. On HP iLO 5, this is +what happens when another client deletes the sessions, for example a management tool logged in +as an administrator; the revocation shows at a whole minute of the session's age (60 s, 120 s, +...). Each `IpmiClient` call opens its own session, so the next call works again. ## `... is not yet implemented.` diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index 690810b..3269087 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -44,9 +44,8 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world reply is sent again; * the [keep-alive](configuration.html#keep-alive) is actually sent: with the default `pingPeriod` (`-1`) 1.2.02 sent no keep-alive at all, so a session could expire during a long collection; the - keep-alive is now a Get Device ID every 30 s by default, whose reply is discarded (not the Get - Channel Authentication Capabilities of 1.2.02, after which HP iLO 5 revokes the session within - 60 s), and a Get Channel Authentication Capabilities command sent by the application in a + keep-alive is now a Get Device ID every 30 s by default, as `ipmitool` sends, whose reply is + discarded, and a Get Channel Authentication Capabilities command sent by the application in a session gets its reply (1.2.02 dropped it, as it did the keep-alive replies); * a one-way IPMI message (`IpmiConnector.sendOneWayMessage()`, `IpmiAsyncConnector.sendMessage()` with `isOneWay`) is queued like any request: its tag stays reserved, and it takes a slot of the diff --git a/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java b/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java index 59afc7e..092bd31 100644 --- a/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java +++ b/src/test/java/org/metricshub/ipmi/client/runner/GetSensorsRunnerTest.java @@ -144,7 +144,7 @@ protected ConnectionHandle getHandle() { final CompactSensorRecord record = new CompactSensorRecord(); record.setName(DEVICE_NAME); - // HP iLO answers D4h once it revoked the session + // HP iLO answers D4h once the session was revoked failure.set(new IPMIException(CompletionCode.InsufficentPrivilege)); assertNull(runner.getSensorRecordReading(record));