Merge pull request #152 from MetricsHub/fix/real-bmc-robustness

Real-BMC robustness: iLO 5 session revocation, iDRAC 8 retry pauses, one-way tags, OEM states (#145 #146 #147 #150)

Author: bertysentry

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
 

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<List<Sensor>> {
 
-	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<Sensor> 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<ReadingType> 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 <code>null</code> 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;
 	}
 }

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 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) {
 			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;
 		}

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
 	 */

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;
 		}
 	}

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;
 		}
 	}

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;
 		}
 	}

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;
 		}
 	}

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;
 		}
 	}

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;
 		}
 	}