dombizita commented on code in PR #10777:
URL: https://github.com/apache/ozone/pull/10777#discussion_r3674388473
##########
hadoop-ozone/interface-client/src/main/proto/OMAdminProtocol.proto:
##########
@@ -113,4 +123,8 @@ service OzoneManagerAdminService {
// RPC request from admin to trigger snapshot defragmentation
rpc triggerSnapshotDefrag(TriggerSnapshotDefragRequest)
returns(TriggerSnapshotDefragResponse);
+
+ // RPC request from the OM leader to a peer OM to read its local upgrade
status.
+ rpc getPeerUpgradeStatus(GetPeerUpgradeStatusRequest)
Review Comment:
As `getPeerUpgradeStatus` only returns the peer OM software version,
shouldn't it go with a name like `getPeerSoftwareVersion`? Or does it make
sense to leave it like this, so it can be extended with other upgrade status
information, if needed?
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -554,12 +554,13 @@ enum Status {
DIRECTORY_NOT_EMPTY = 68;
- PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED = 69;
- REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED = 70;
- UPDATE_LAYOUT_VERSION_FAILED = 71;
- LAYOUT_FEATURE_FINALIZATION_FAILED = 72;
- PREPARE_FAILED = 73; // Deprecated
- NOT_SUPPORTED_OPERATION_WHEN_PREPARED = 74; // Deprecated
+ PERSIST_UPGRADE_TO_LAYOUT_VERSION_FAILED = 69; // [deprecated = true]
+ REMOVE_UPGRADE_TO_LAYOUT_VERSION_FAILED = 70; // [deprecated = true]
+ UPDATE_LAYOUT_VERSION_FAILED = 71; // [deprecated = true]
+ LAYOUT_FEATURE_FINALIZATION_FAILED = 72; // [deprecated = true]
+ PREPARE_FAILED = 73; // [deprecated = true]
+ NOT_SUPPORTED_OPERATION_WHEN_PREPARED = 74; // [deprecated = true]
Review Comment:
Are the `[deprecated = true]` intentionally commented out, if so why?
##########
hadoop-ozone/interface-client/src/main/proto/OMAdminProtocol.proto:
##########
@@ -93,6 +93,16 @@ message TriggerSnapshotDefragResponse {
optional bool result = 3;
}
+// Request from an OM leader to a peer OM to read its local upgrade status.
+// Intentionally OM-local: no SCM round-trip is performed by the server.
+message GetPeerUpgradeStatusRequest {
+}
+
+message GetPeerUpgradeStatusResponse {
+ // Serialized OzoneManagerVersion.SOFTWARE_VERSION of the responding OM
binary.
+ optional int32 omSoftwareVersion = 2;
Review Comment:
```suggestion
optional int32 omSoftwareVersion = 1;
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/upgrade/TestOMStartFinalizeUpgradeRequest.java:
##########
@@ -103,6 +183,128 @@ public void testAccessDeniedWhenUserIsNotAdmin() throws
IOException {
verify(scmContainerLocationProtocol, never()).finalizeUpgrade();
}
+ @Test
+ public void testPeerVersionCheckPassesWhenNoPeers() throws IOException {
+ // @BeforeEach already stubs getPeerNodes() to return an empty list.
+ // preExecute must complete normally and call SCM finalize.
+ doNothing().when(scmContainerLocationProtocol).finalizeUpgrade();
+
+ OzoneManagerProtocolProtos.OMRequest original = buildRequest();
+ new OMStartFinalizeUpgradeRequest(original).preExecute(ozoneManager);
+
+ verify(scmContainerLocationProtocol).finalizeUpgrade();
+ }
+
+ @Test
+ public void testPeerVersionCheckPassesWhenAllPeersMatch() throws IOException
{
+ doNothing().when(scmContainerLocationProtocol).finalizeUpgrade();
+
when(ozoneManager.getPeerNodes()).thenReturn(Arrays.asList(buildPeer("om2"),
buildPeer("om3")));
+ OMAdminProtocolClientSideImpl matchingClient =
peerClientWithVersion(OzoneManagerVersion.SOFTWARE_VERSION);
+
+ try (MockedStatic<OMAdminProtocolClientSideImpl> factory =
+ mockStatic(OMAdminProtocolClientSideImpl.class)) {
+ factory.when(() ->
OMAdminProtocolClientSideImpl.createProxyForSingleOM(any(), any(), any()))
+ .thenReturn(matchingClient);
+
+ new
OMStartFinalizeUpgradeRequest(buildRequest()).preExecute(ozoneManager);
+ }
+
+ verify(scmContainerLocationProtocol).finalizeUpgrade();
+ }
+
+ @Test
+ public void testPeerVersionCheckRejectsOneOlderPeer() throws IOException {
+
when(ozoneManager.getPeerNodes()).thenReturn(Arrays.asList(buildPeer("om2"),
buildPeer("om3")));
+ OMAdminProtocolClientSideImpl matchingClient =
peerClientWithVersion(OzoneManagerVersion.SOFTWARE_VERSION);
+ OMAdminProtocolClientSideImpl olderClient =
peerClientWithVersion(OzoneManagerVersion.HBASE_SUPPORT);
+
+ try (MockedStatic<OMAdminProtocolClientSideImpl> factory =
+ mockStatic(OMAdminProtocolClientSideImpl.class)) {
+ factory.when(() ->
OMAdminProtocolClientSideImpl.createProxyForSingleOM(any(), any(), any()))
+ .thenReturn(matchingClient, olderClient);
+
+ OMException ex = assertThrows(OMException.class,
+ () -> new
OMStartFinalizeUpgradeRequest(buildRequest()).preExecute(ozoneManager));
+ assertEquals(OMException.ResultCodes.NOT_SUPPORTED_OPERATION,
ex.getResult());
+ }
+
+ verify(scmContainerLocationProtocol, never()).finalizeUpgrade();
+ }
+
+ @Test
+ public void testPeerVersionCheckRejectsOneUnknownFuturePeer() throws
IOException {
+
when(ozoneManager.getPeerNodes()).thenReturn(Arrays.asList(buildPeer("om2"),
buildPeer("om3")));
+ OMAdminProtocolClientSideImpl matchingClient =
peerClientWithVersion(OzoneManagerVersion.SOFTWARE_VERSION);
+ OMAdminProtocolClientSideImpl unknownClient =
peerClientWithVersion(OzoneManagerVersion.UNKNOWN_VERSION);
+
+ try (MockedStatic<OMAdminProtocolClientSideImpl> factory =
+ mockStatic(OMAdminProtocolClientSideImpl.class)) {
+ factory.when(() ->
OMAdminProtocolClientSideImpl.createProxyForSingleOM(any(), any(), any()))
+ .thenReturn(matchingClient, unknownClient);
+
+ OMException ex = assertThrows(OMException.class,
+ () -> new
OMStartFinalizeUpgradeRequest(buildRequest()).preExecute(ozoneManager));
+ assertEquals(OMException.ResultCodes.NOT_SUPPORTED_OPERATION,
ex.getResult());
+ }
+
+ verify(scmContainerLocationProtocol, never()).finalizeUpgrade();
+ }
+
+ @Test
+ public void testPeerVersionCheckRejectsUnreachablePeer() throws IOException {
+
when(ozoneManager.getPeerNodes()).thenReturn(Collections.singletonList(buildPeer("om2")));
+ OMAdminProtocolClientSideImpl unreachableClient =
mock(OMAdminProtocolClientSideImpl.class);
+ when(unreachableClient.getPeerUpgradeStatus()).thenThrow(new
IOException("connection refused"));
+
+ try (MockedStatic<OMAdminProtocolClientSideImpl> factory =
+ mockStatic(OMAdminProtocolClientSideImpl.class)) {
+ factory.when(() ->
OMAdminProtocolClientSideImpl.createProxyForSingleOM(any(), any(), any()))
+ .thenReturn(unreachableClient);
+
+ OMException ex = assertThrows(OMException.class,
+ () -> new
OMStartFinalizeUpgradeRequest(buildRequest()).preExecute(ozoneManager));
+ assertEquals(OMException.ResultCodes.NOT_SUPPORTED_OPERATION,
ex.getResult());
+ }
+
+ verify(scmContainerLocationProtocol, never()).finalizeUpgrade();
+ }
+
+ @Test
+ public void testForceSkipsPeerVersionCheckForUnreachablePeer() throws
IOException {
Review Comment:
Maybe worth to add a similar test with force upgrade when a peer is
reachable, but there is a version mismatch.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMStartFinalizeUpgradeRequest.java:
##########
@@ -94,8 +118,42 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager, Execut
response = new
OMStartFinalizeUpgradeResponse(createErrorOMResponse(responseBuilder, e));
}
- markForAudit(auditLogger, buildAuditMessage(OMAction.UPGRADE_FINALIZE, new
HashMap<>(), exception, userInfo));
+ Map<String, String> auditMap = new HashMap<>();
+ auditMap.put("force",
String.valueOf(getOmRequest().getStartFinalizeUpgradeRequest().getForce()));
+ markForAudit(auditLogger, buildAuditMessage(OMAction.UPGRADE_FINALIZE,
auditMap, exception, userInfo));
return response;
}
+ private static void validatePeerOmVersionsBeforeFinalize(List<OMNodeDetails>
peerNodes,
+ OzoneConfiguration configuration) throws OMException {
+ if (peerNodes.isEmpty()) {
+ return;
+ }
+ OzoneManagerVersion leaderVersion = OzoneManagerVersion.SOFTWARE_VERSION;
+ List<String> failedPeers = new ArrayList<>();
+ for (OMNodeDetails peerDetails : peerNodes) {
+ String peerId = peerDetails.getNodeId();
+ OMAdminProtocolClientSideImpl client = null;
+ try {
+ client =
OMAdminProtocolClientSideImpl.createProxyForSingleOM(configuration,
getRemoteUser(), peerDetails);
+ OzoneManagerVersion peerVersion = client.getPeerUpgradeStatus();
+ if (!peerVersion.equals(leaderVersion)) {
+ LOG.warn("OM peer {} is running software version {} but leader is
running version {}. "
+ + "Rejecting finalize command.", peerId, peerVersion,
leaderVersion);
+ failedPeers.add(peerId + " (version: " + peerVersion + ")");
+ }
+ } catch (IOException e) {
+ LOG.warn("Failed to contact OM peer {} to check software version
before finalize.", peerId, e);
+ failedPeers.add(peerId + " (unreachable: " + e.getMessage() + ")");
+ } finally {
+ IOUtils.cleanupWithLogger(LOG, client);
+ }
+ }
+ if (!failedPeers.isEmpty()) {
+ throw new OMException("Finalize rejected: the following OM peers did not
confirm matching software version "
+ + "(expected version=" + leaderVersion + "): " + String.join(", ",
failedPeers),
+ OMException.ResultCodes.NOT_SUPPORTED_OPERATION);
Review Comment:
Both SCM `UNSUPPORTED_OPERATION` and peer version issues go with
`OMException.ResultCodes.NOT_SUPPORTED_OPERATION` in the end, maybe we could
use something else here.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3650,8 +3650,7 @@ public StatusAndMessages finalizeUpgrade(String
unusedUpgradeClientId)
return FINALIZED_MSG;
}
versionManager.finalizeUpgrade();
- // OM clients currently require STARTING_MSG to be returned when this
method succeeds.
- // TODO This will be removed when OM learns to finalize from SCM.
Review Comment:
The `STARTING_MSG` won't be removed?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/protocolPB/TestOMAdminProtocolServerSideImpl.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.protocolPB;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+
+import com.google.protobuf.ServiceException;
+import org.apache.hadoop.ozone.OzoneManagerVersion;
+import org.apache.hadoop.ozone.om.OzoneManager;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerAdminProtocolProtos.GetPeerUpgradeStatusRequest;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerAdminProtocolProtos.GetPeerUpgradeStatusResponse;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link OMAdminProtocolServerSideImpl#getPeerUpgradeStatus}.
+ */
+public class TestOMAdminProtocolServerSideImpl {
+
+ @Test
+ public void testGetPeerUpgradeStatusReturnsSoftwareVersion() throws
ServiceException {
+ OzoneManager om = mock(OzoneManager.class);
+
+ OMAdminProtocolServerSideImpl handler = new
OMAdminProtocolServerSideImpl(om);
+ GetPeerUpgradeStatusResponse response =
+ handler.getPeerUpgradeStatus(null,
GetPeerUpgradeStatusRequest.newBuilder().build());
+
+ assertEquals(OzoneManagerVersion.SOFTWARE_VERSION.serialize(),
response.getOmSoftwareVersion());
Review Comment:
Add an assertion that the RPC is OM local, as intended, Claude suggested
this.
```suggestion
assertEquals(OzoneManagerVersion.SOFTWARE_VERSION.serialize(),
response.getOmSoftwareVersion());
verifyNoInteractions(om);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
