Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ jobs:
if [[ -s "target/${{ inputs.script }}/summary.md" ]]; then
cat target/${{ inputs.script }}/summary.md >> $GITHUB_STEP_SUMMARY
fi
hadoop-ozone/dev-support/checks/_summary.sh target/${{ inputs.script }}/summary.txt
hadoop-ozone/dev-support/checks/_summary.sh target/${{ inputs.script }}/summary.txt || true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
hadoop-ozone/dev-support/checks/_summary.sh target/${{ inputs.script }}/summary.txt || true
hadoop-ozone/dev-support/checks/_summary.sh target/${{ inputs.script }}/summary.txt


- name: Archive build results
if: ${{ !cancelled() }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/intermittent-test-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ jobs:
env:
DEVELOCITY_ACCESS_KEY: ${{ secrets.DEVELOCITY_ACCESS_KEY }}
- name: Summary of failures
run: hadoop-ozone/dev-support/checks/_summary.sh target/unit/summary.txt
run: hadoop-ozone/dev-support/checks/_summary.sh target/unit/summary.txt || true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
run: hadoop-ozone/dev-support/checks/_summary.sh target/unit/summary.txt || true
run: hadoop-ozone/dev-support/checks/_summary.sh target/unit/summary.txt

if: ${{ !cancelled() }}
- name: Archive build results
uses: actions/upload-artifact@v4
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/repeat-acceptance.yml
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ jobs:
KEEP_IMAGE: false
continue-on-error: true
- name: Summary of failures
run: hadoop-ozone/dev-support/checks/_summary.sh target/${{ github.job }}/summary.txt
run: hadoop-ozone/dev-support/checks/_summary.sh target/${{ github.job }}/summary.txt || true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
run: hadoop-ozone/dev-support/checks/_summary.sh target/${{ github.job }}/summary.txt || true
run: hadoop-ozone/dev-support/checks/_summary.sh target/${{ github.job }}/summary.txt

if: ${{ !cancelled() }}
- name: Archive build results
uses: actions/upload-artifact@v4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,10 @@ public final class ScmConfigKeys {
"ozone.scm.container.size";
public static final String OZONE_SCM_CONTAINER_SIZE_DEFAULT = "5GB";

public static final String OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER =
Copy link
Member

Choose a reason for hiding this comment

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

I’m thinking this patch only needs to add a config to set the required data size (dataSizeRequired), which we can then read from ReplicationManagerUtil here: https://github.com/peterxcli/ozone/blob/7868a862ff616154e4bd9ab52dada929e15042ec/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManagerUtil.java#L93-L94.
@siddhantsangwan does this approach look correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @peterxcli ! I've tried to simplify the approach removed the new config entirely and directly use HddsServerUtil.requiredReplicationSpace() which already provides the 2x multiplier.

"ozone.scm.container.space.requirement.multiplier";
public static final double OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER_DEFAULT = 2.0;

public static final String OZONE_SCM_CONTAINER_LOCK_STRIPE_SIZE =
"ozone.scm.container.lock.stripes";
public static final int OZONE_SCM_CONTAINER_LOCK_STRIPE_SIZE_DEFAULT = 512;
Expand Down
16 changes: 16 additions & 0 deletions hadoop-hdds/common/src/main/resources/ozone-default.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,22 @@
balances the amount of metadata.
</description>
</property>
<property>
<name>ozone.scm.container.space.requirement.multiplier</name>
<value>2.0</value>

Choose a reason for hiding this comment

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

it would solve the problem with 2 concurrent allocations, which would both pass. But with increased concurrency we could expect the same failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we even need to add one more config here, instead this value can be 5 or 10x of container size , concurrency prediction is unknown here. Even if concurrency is very high on same DN at the same time, client always has a mechanism to retry on another pipeline.

<tag>OZONE, SCM, MANAGEMENT</tag>
<description>
Multiplier for container space requirement when checking if a datanode
has enough space for container allocation. The required space is calculated
as container size multiplied by this value. This prevents concurrent clients
from all passing the space check when there's only enough space for one
container. For example, with default container size of 5GB and multiplier
of 2.0, the system will require 10GB of available space before allocating
a new container. This ensures that if only 6GB is remaining, the check will
fail, preventing multiple clients from attempting to create containers
concurrently when there's only space for one.
</description>
</property>
<property>
<name>ozone.scm.container.lock.stripes</name>
<value>512</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public class ContainerManagerImpl implements ContainerManager {
private final Random random = new Random();

private final long maxContainerSize;
private final long containerSpaceRequirement;

/**
*
Expand Down Expand Up @@ -115,6 +116,11 @@ public ContainerManagerImpl(
maxContainerSize = (long) conf.getStorageSize(ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES);

double multiplier = conf.getDouble(
ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER,
ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER_DEFAULT);
this.containerSpaceRequirement = (long) (maxContainerSize * multiplier);

this.scmContainerManagerMetrics = SCMContainerManagerMetrics.create();
}

Expand Down Expand Up @@ -352,23 +358,23 @@ public ContainerInfo getMatchingContainer(final long size, final String owner,
synchronized (pipeline.getId()) {
containerIDs = getContainersForOwner(pipeline, owner);
if (containerIDs.size() < getOpenContainerCountPerPipeline(pipeline)) {
if (pipelineManager.hasEnoughSpace(pipeline, maxContainerSize)) {
if (pipelineManager.hasEnoughSpace(pipeline, containerSpaceRequirement)) {
allocateContainer(pipeline, owner);
containerIDs = getContainersForOwner(pipeline, owner);
} else {
LOG.debug("Cannot allocate a new container because pipeline {} does not have the required space {}.",
pipeline, maxContainerSize);
pipeline, containerSpaceRequirement);
}
}
containerIDs.removeAll(excludedContainerIDs);
containerInfo = containerStateManager.getMatchingContainer(
size, owner, pipeline.getId(), containerIDs);
if (containerInfo == null) {
if (pipelineManager.hasEnoughSpace(pipeline, maxContainerSize)) {
if (pipelineManager.hasEnoughSpace(pipeline, containerSpaceRequirement)) {
containerInfo = allocateContainer(pipeline, owner);
} else {
LOG.debug("Cannot allocate a new container because pipeline {} does not have the required space {}.",
pipeline, maxContainerSize);
pipeline, containerSpaceRequirement);
}
}
return containerInfo;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public class ECPipelineProvider extends PipelineProvider<ECReplicationConfig> {
private final ConfigurationSource conf;
private final PlacementPolicy placementPolicy;
private final long containerSizeBytes;
private final long containerSpaceRequirement;

public ECPipelineProvider(NodeManager nodeManager,
PipelineStateManager stateManager,
Expand All @@ -73,6 +74,10 @@ public ECPipelineProvider(NodeManager nodeManager,
this.containerSizeBytes = (long) this.conf
.getStorageSize(ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES);
double multiplier = conf.getDouble(
ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER,
ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER_DEFAULT);
this.containerSpaceRequirement = (long) (containerSizeBytes * multiplier);
}

@Override
Expand All @@ -88,7 +93,7 @@ protected Pipeline create(ECReplicationConfig replicationConfig,
throws IOException {
List<DatanodeDetails> dns = placementPolicy
.chooseDatanodes(excludedNodes, favoredNodes,
replicationConfig.getRequiredNodes(), 0, this.containerSizeBytes);
replicationConfig.getRequiredNodes(), 0, this.containerSpaceRequirement);
return create(replicationConfig, dns);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ public class RatisPipelineProvider
private final LeaderChoosePolicy leaderChoosePolicy;
private final SCMContext scmContext;
private final long containerSizeBytes;
private final long containerSpaceRequirement;
private final long minRatisVolumeSizeBytes;

@VisibleForTesting
Expand All @@ -89,6 +90,10 @@ public RatisPipelineProvider(NodeManager nodeManager,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT,
StorageUnit.BYTES);
double multiplier = conf.getDouble(
ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER,
ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER_DEFAULT);
this.containerSpaceRequirement = (long) (containerSizeBytes * multiplier);
this.minRatisVolumeSizeBytes = (long) this.conf.getStorageSize(
ScmConfigKeys.OZONE_DATANODE_RATIS_VOLUME_FREE_SPACE_MIN,
ScmConfigKeys.OZONE_DATANODE_RATIS_VOLUME_FREE_SPACE_MIN_DEFAULT,
Expand Down Expand Up @@ -165,7 +170,7 @@ public synchronized Pipeline create(RatisReplicationConfig replicationConfig,
switch (factor) {
case ONE:
dns = pickNodesNotUsed(replicationConfig, minRatisVolumeSizeBytes,
containerSizeBytes, conf);
containerSpaceRequirement, conf);
break;
case THREE:
List<DatanodeDetails> excludeDueToEngagement = filterPipelineEngagement();
Expand All @@ -178,7 +183,7 @@ public synchronized Pipeline create(RatisReplicationConfig replicationConfig,
}
dns = placementPolicy.chooseDatanodes(excludedNodes,
favoredNodes, factor.getNumber(), minRatisVolumeSizeBytes,
containerSizeBytes);
containerSpaceRequirement);
break;
default:
throw new IllegalStateException("Unknown factor: " + factor.name());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,11 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.atLeast;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand All @@ -47,6 +50,7 @@
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor;
import org.apache.hadoop.hdds.scm.ScmConfigKeys;
import org.apache.hadoop.hdds.scm.container.replication.ContainerReplicaPendingOps;
import org.apache.hadoop.hdds.scm.container.states.ContainerStateMap;
import org.apache.hadoop.hdds.scm.ha.SCMHAManager;
Expand Down Expand Up @@ -179,6 +183,42 @@ public void testGetMatchingContainerReturnsContainerWhenEnoughSpaceInDatanodes()
assertNotNull(container);
}

@Test
public void testContainerSpaceRequirementMultiplier() throws IOException {
long sizeRequired = 256 * 1024 * 1024;
double multiplier = 2.0;
long containerSize = 5L * 1024 * 1024 * 1024;
long expectedSpaceRequirement = (long) (containerSize * multiplier);

PipelineManager spyPipelineManager = spy(pipelineManager);
File tempDir = new File(testDir, "tempDir");
OzoneConfiguration conf = SCMTestUtils.getConf(tempDir);
conf.setDouble(ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER, multiplier);

ContainerManager manager = new ContainerManagerImpl(conf,
scmhaManager, sequenceIdGen, spyPipelineManager,
SCMDBDefinition.CONTAINERS.getTable(dbStore), pendingOpsMock);

Pipeline pipeline = spyPipelineManager.getPipelines().iterator().next();

doReturn(false).when(spyPipelineManager)
.hasEnoughSpace(any(Pipeline.class), anyLong());
ContainerInfo container = manager
.getMatchingContainer(sizeRequired, "test", pipeline, Collections.emptySet());
assertNull(container);
verify(spyPipelineManager, atLeast(1))
.hasEnoughSpace(eq(pipeline), eq(expectedSpaceRequirement));

reset(spyPipelineManager);
doReturn(true).when(spyPipelineManager)
.hasEnoughSpace(any(Pipeline.class), anyLong());
container = manager
.getMatchingContainer(sizeRequired, "test", pipeline, Collections.emptySet());
assertNotNull(container);
verify(spyPipelineManager, atLeast(1))
.hasEnoughSpace(eq(pipeline), eq(expectedSpaceRequirement));
}

@Test
void testUpdateContainerState() throws Exception {
final ContainerInfo container = containerManager.allocateContainer(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,21 @@ public class TestECPipelineProvider {
private PipelineStateManager stateManager =
mock(PipelineStateManager.class);
private PlacementPolicy placementPolicy = mock(PlacementPolicy.class);
private long containerSizeBytes;
private long containerSpaceRequirement;

@BeforeEach
public void setup() throws IOException, NodeNotFoundException {
OzoneConfiguration conf = new OzoneConfiguration();
provider = new ECPipelineProvider(
nodeManager, stateManager, conf, placementPolicy);
this.containerSizeBytes = (long) conf.getStorageSize(
long containerSizeBytes = (long) conf.getStorageSize(
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT,
StorageUnit.BYTES);
double multiplier = conf.getDouble(
ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER,
ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER_DEFAULT);
this.containerSpaceRequirement = (long) (containerSizeBytes * multiplier);
// Placement policy will always return EC number of random nodes.
when(placementPolicy.chooseDatanodes(anyList(),
anyList(), anyInt(), anyLong(),
Expand Down Expand Up @@ -200,7 +204,7 @@ public void testExcludedAndFavoredNodesPassedToPlacementPolicy()
assertEquals(ecConf.getData() + ecConf.getParity(), pipeline.getNodes().size());

verify(placementPolicy).chooseDatanodes(excludedNodes, favoredNodes,
ecConf.getRequiredNodes(), 0, containerSizeBytes);
ecConf.getRequiredNodes(), 0, containerSpaceRequirement);
}

private Set<ContainerReplica> createContainerReplicas(int number) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ data:
OZONE-SITE.XML_hdds.scm.safemode.min.datanode: "3"
OZONE-SITE.XML_ozone.datanode.pipeline.limit: "1"
OZONE-SITE.XML_hdds.datanode.volume.min.free.space: "1GB"
OZONE-SITE.XML_ozone.scm.container.space.requirement.multiplier: "1.0"
OZONE-SITE.XML_ozone.metadata.dirs: "/data/metadata"
OZONE-SITE.XML_ozone.om.address: "om-0.om"
OZONE-SITE.XML_ozone.recon.address: "recon-0.recon"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ data:
OZONE-SITE.XML_hdds.scm.safemode.min.datanode: "3"
OZONE-SITE.XML_ozone.datanode.pipeline.limit: "1"
OZONE-SITE.XML_hdds.datanode.volume.min.free.space: "1GB"
OZONE-SITE.XML_ozone.scm.container.space.requirement.multiplier: "1.0"
OZONE-SITE.XML_ozone.metadata.dirs: /data/metadata
OZONE-SITE.XML_ozone.om.address: om-0.om
OZONE-SITE.XML_ozone.recon.address: recon-0.recon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ data:
OZONE-SITE.XML_hdds.scm.safemode.min.datanode: "3"
OZONE-SITE.XML_ozone.datanode.pipeline.limit: "1"
OZONE-SITE.XML_hdds.datanode.volume.min.free.space: "1GB"
OZONE-SITE.XML_ozone.scm.container.space.requirement.multiplier: "1.0"
OZONE-SITE.XML_ozone.metadata.dirs: /data/metadata
OZONE-SITE.XML_ozone.om.address: om-0.om
OZONE-SITE.XML_ozone.recon.address: recon-0.recon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ data:
OZONE-SITE.XML_hdds.scm.safemode.min.datanode: "3"
OZONE-SITE.XML_ozone.datanode.pipeline.limit: "1"
OZONE-SITE.XML_hdds.datanode.volume.min.free.space: "1GB"
OZONE-SITE.XML_ozone.scm.container.space.requirement.multiplier: "1.0"
OZONE-SITE.XML_ozone.metadata.dirs: /data/metadata
OZONE-SITE.XML_ozone.om.address: om-0.om
OZONE-SITE.XML_ozone.recon.address: recon-0.recon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ data:
OZONE-SITE.XML_hdds.scm.safemode.min.datanode: "3"
OZONE-SITE.XML_ozone.datanode.pipeline.limit: "1"
OZONE-SITE.XML_hdds.datanode.volume.min.free.space: "1GB"
OZONE-SITE.XML_ozone.scm.container.space.requirement.multiplier: "1.0"
OZONE-SITE.XML_ozone.metadata.dirs: /data/metadata
OZONE-SITE.XML_ozone.om.address: om-0.om
OZONE-SITE.XML_ozone.recon.address: recon-0.recon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ data:
OZONE-SITE.XML_hdds.scm.safemode.min.datanode: "3"
OZONE-SITE.XML_ozone.datanode.pipeline.limit: "1"
OZONE-SITE.XML_hdds.datanode.volume.min.free.space: "1GB"
OZONE-SITE.XML_ozone.scm.container.space.requirement.multiplier: "1.0"
OZONE-SITE.XML_ozone.metadata.dirs: /data/metadata
OZONE-SITE.XML_ozone.om.address: om-0.om
OZONE-SITE.XML_ozone.recon.address: recon-0.recon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ public void init() throws Exception {
conf = new OzoneConfiguration();
chunkSize = (int) OzoneConsts.MB;
blockSize = 4 * chunkSize;
conf.setDouble(ScmConfigKeys.OZONE_SCM_CONTAINER_SPACE_REQUIREMENT_MULTIPLIER, 1.0);
conf.setTimeDuration(OZONE_SCM_STALENODE_INTERVAL, 100, TimeUnit.SECONDS);

RatisClientConfig ratisClientConfig =
Expand Down