You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/05/31 06:04:10 UTC
[GitHub] [ozone] errose28 opened a new pull request, #3465: HDDS-6760. [SCM HA finalization] Support for happy path finalization
errose28 opened a new pull request, #3465:
URL: https://github.com/apache/ozone/pull/3465
## What changes were proposed in this pull request?
**Please read the design document attached to HDDS-5141 for context when reviewing**
Add the foundation for finalizing an SCM HA cluster. Non-HA SCM finalization will also use this code from now on. Other remaining items under the parent Jira HDDS-5141 which are out of scope for this task are:
- HDDS-6762
- Async finalization from client perspective (return to the client after the finalizing mark has been written to the DB).
- Ability for the client to query the status of ongoing SCM finalization.
- HDDS-6761
- Resume finalization on the new SCM leader on leader change, and cease the finalization thread on the old leader.
- Allow followers to finalize from a snapshot.
- This is the purpose of storing the layout version in the DB as well as the version file.
Portions of the common upgrade framework were refactored under this PR as well to make it more flexible for supporting SCM HA finalization. For example, finalization in the `BasicUpgradeFinalizer` was split from one method finalizing all layout features to one method invoked to finalize each layout feature.
Some more invasive changes are marked as TODO to reduce the size of this PR:
- Refactor the old `SCMNodeManager` constructor which has been left due to its high usage in tests.
- Refactor `TestSCMNodeManager` to use junit jupiter for test specific parameterization.
## What is the link to the Apache JIRA
HDDS-6760
## How was this patch tested?
- Existing unit tests modified.
- New unit tests added.
- `TestHDDSUpgrade#testFinalizationFromInitialVersionToLatestVersion` was run with 3 SCMs and passed consistently, although this test is excluded from the regular CI workflow.
- Existing upgrade acceptance tests passed. These tests currently do not use SCM HA but are still running the code in this PR.
## Outstanding Issues
- This PR is being left as a draft until the following issues are resolved:
- Failure in `TestDatanodeUpgradeToScmHA`.
- Failure in `TestScmFinalization#testResumeFinalizationFromCheckpoint`
- See the `TODO` comment in `BasicUpgradeFinalizer#finalize` for what needs to be done to fix this test.
- `ScmUpgradeFinalizer` is not receiving injected `UpgradeFinalizationExecutor`s after refactoring.
- This is an easy fix that will be corrected soon.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3465: HDDS-6760. [SCM HA finalization] Support for happy path finalization
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3465:
URL: https://github.com/apache/ozone/pull/3465#issuecomment-1146635922
@errose28 Thanks for updating the patch. Looks like upgrade acceptance test failed at `Finalize SCM`: https://github.com/apache/ozone/runs/6734226622#step:5:712 (unlikely to be related to the latest commit, but probably related to the patch itself).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3465: HDDS-6760. [SCM HA finalization] Support for happy path finalization
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3465:
URL: https://github.com/apache/ozone/pull/3465#issuecomment-1148216496
Hi @adoroszlai I have updated the PR. The issue with containers stuck in the CLOSING state should be fixed, see the explanation added to the docker-config file comments. I have also changed the PR so that the finalization checkpoint is written to the `SCMContext` by the `FinalizationStateManager`, where it can be read by other managers. This removes a circular dependency, fixes a deadlock bug, and makes the code easier to work with IMO.
I will be doing repeated runs of the upgrade acceptance test overnight here to test stability, but PTAL in the meantime.
cc @nandakumar131
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] nandakumar131 merged pull request #3465: HDDS-6760. [SCM HA finalization] Support for happy path finalization
Posted by GitBox <gi...@apache.org>.
nandakumar131 merged PR #3465:
URL: https://github.com/apache/ozone/pull/3465
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] nandakumar131 commented on pull request #3465: HDDS-6760. [SCM HA finalization] Support for happy path finalization
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on PR #3465:
URL: https://github.com/apache/ozone/pull/3465#issuecomment-1149646097
@errose28, thanks for working on this.
+1 on the changes, I will merge the PR shortly.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #3465: HDDS-6760. [SCM HA finalization] Support for happy path finalization
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #3465:
URL: https://github.com/apache/ozone/pull/3465#discussion_r888946535
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/IntegerCodec.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.hdds.scm.ha.io;
+
+import com.google.common.primitives.Ints;
+import com.google.common.primitives.Longs;
+import com.google.protobuf.ByteString;
+import com.google.protobuf.InvalidProtocolBufferException;
+
+/**
+ * Encodes/decodes an integer to a byte string.
+ */
+public class IntegerCodec implements Codec {
+ @Override
+ public ByteString serialize(Object object)
+ throws InvalidProtocolBufferException {
+ return ByteString.copyFrom(Ints.toByteArray((Integer) object));
+ }
+
+ @Override
+ public Object deserialize(Class<?> type, ByteString value)
+ throws InvalidProtocolBufferException {
+ return Longs.fromByteArray(value.toByteArray());
Review Comment:
Typo?
```suggestion
return Ints.fromByteArray(value.toByteArray());
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -667,6 +670,23 @@ private void initializeSystemManagers(OzoneConfiguration conf,
containerManager.getContainers(), containerManager,
pipelineManager, eventQueue, serviceManager, scmContext);
}
+
+ UpgradeFinalizationExecutor<SCMUpgradeFinalizationContext>
+ finalizationExecutor = new DefaultUpgradeFinalizationExecutor<>();
+ if (configurator.getUpgradeFinalizationExecutor() != null) {
+ finalizationExecutor = configurator.getUpgradeFinalizationExecutor();
+ }
Review Comment:
Nit: avoid creating `DefaultUpgradeFinalizationExecutor` unnecessarily.
```suggestion
UpgradeFinalizationExecutor<SCMUpgradeFinalizationContext>
finalizationExecutor;
if (configurator.getUpgradeFinalizationExecutor() != null) {
finalizationExecutor = configurator.getUpgradeFinalizationExecutor();
} else {
finalizationExecutor = new DefaultUpgradeFinalizationExecutor<>();
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] nandakumar131 commented on pull request #3465: HDDS-6760. [SCM HA finalization] Support for happy path finalization
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on PR #3465:
URL: https://github.com/apache/ozone/pull/3465#issuecomment-1148848187
@errose28, thanks for working on this PR.
The branch has conflicts, can you please look into it?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3465: HDDS-6760. [SCM HA finalization] Support for happy path finalization
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3465:
URL: https://github.com/apache/ozone/pull/3465#issuecomment-1146449718
Thanks for taking the time to review this large feature @adoroszlai! I have addressed both comments in the most recent commit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3465: HDDS-6760. [SCM HA finalization] Support for happy path finalization
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3465:
URL: https://github.com/apache/ozone/pull/3465#issuecomment-1149305172
Although the tests were passing pretty consistently with the last set of changes and a bit longer test timeout, I have made some minor changes that I think will improve the test execution and finalization in a live cluster overall. The case we are working with is when finalization is run soon after SCM exits safemode and the datanodes have not yet finished leader elections for their pipelines. In this case, the close commands sent by SCM may result in all 3 container replicas entering the CLOSING state but no datanode actually submitting the close command through Ratis. The datanodes will not finalize when containers are in the CLOSING state.
In the original SCM finalization implementation, the `HealthyReadonlyNodeHandler` implementation (which is invoked when datanode heartbeats to SCM with a lower MLV) contained this block:
```java
Set<PipelineID> pipelineIDs = nodeManager.getPipelines(datanodeDetails);
for (PipelineID id: pipelineIDs) {
LOG.info("Closing pipeline {} which uses HEALTHY READONLY datanode {} ",
id, datanodeDetails);
try {
pipelineManager.closePipeline(pipelineManager.getPipeline(id), false);
} catch (IOException ex) {
LOG.error("Failed to close pipeline {} which uses HEALTHY READONLY " +
"datanode {}: ", id, datanodeDetails, ex);
}
}
```
Since pipelines were closed normally (not force closed) when finalization started, this would force close the already closed pipelines, moving the CLOSING containers to QUASI_CLOSED and allowing finalization to pass.
In earlier versions of this patch, I changed this block:
```java
Set<PipelineID> pipelineIDs = nodeManager.getPipelines(datanodeDetails);
for (PipelineID id: pipelineIDs) {
try {
Pipeline pipeline = pipelineManager.getPipeline(id);
if (pipeline.getPipelineState() != Pipeline.PipelineState.CLOSED) {
LOG.warn("Closing pipeline {} which uses HEALTHY READONLY datanode " +
"{}.", id, datanodeDetails.getUuidString());
pipelineManager.closePipeline(pipeline, true);
}
} catch (IOException ex) {
LOG.error("Failed to close pipeline {} which uses HEALTHY READONLY " +
"datanode {}: ", id, datanodeDetails, ex);
}
}
```
Since the pipelines were already closed during finalization, the pipeline close here became a no-op, and the CLOSING containers lingered on the datanodes. I originally handled this in the test by making the ReplicationManager run soon after SCM exited safemode to move the containers from CLOSING to CLOSED, but this was still slow to finish, both in the test and possibly in a real cluster as well.
In the most recent update, I have changed the block to this:
```java
Set<PipelineID> pipelineIDs = nodeManager.getPipelines(datanodeDetails);
for (PipelineID pipelineID : pipelineIDs) {
try {
Pipeline pipeline = pipelineManager.getPipeline(pipelineID);
LOG.info("Sending close command for pipeline {} in state {} which " +
"uses {} datanode {}. This will send close commands for its " +
"containers.",
pipelineID, pipeline.getPipelineState(),
HddsProtos.NodeState.HEALTHY_READONLY,
datanodeDetails.getUuidString());
pipelineManager.closePipeline(pipeline, true);
} catch (IOException ex) {
LOG.error("Failed to close pipeline {} which uses HEALTHY READONLY " +
"datanode {}: ", pipelineID, datanodeDetails, ex);
}
}
```
Now when a healthy readonly datanode heartbeats back to SCM, the close pipeline command will queue close container commands for all containers in the pipeline. For already CLOSED containers this will be a no-op, but for CLOSING containers whose datanodes have since finished their pipeline leader election, this will move them to CLOSED without needing to wait for the ReplicationManager to start. This should allow finalization to proceed at the same pace as before, but without the occasional quasi closed containers and without test specific config changes for the replication manager.
I will again be doing repeated testing of the acceptance test on my fork, and will update here when it appears stable.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org