Skip to content

HDDS-8758. Container replication to use DISK volumes#4857

Closed
Cyrill wants to merge 1 commit intoapache:masterfrom
Cyrill:HDDS-8758-store-closed-on-disk
Closed

HDDS-8758. Container replication to use DISK volumes#4857
Cyrill wants to merge 1 commit intoapache:masterfrom
Cyrill:HDDS-8758-store-closed-on-disk

Conversation

@Cyrill
Copy link
Contributor

@Cyrill Cyrill commented Jun 8, 2023

What changes were proposed in this pull request?

Now there is a way to define a volume choosing policy for replicating closed containers only.
Also added a policy that selects nodes with enough space on DISK/SSD storage - to be able to replicated closed containers on nodes with HDD.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-8758

How was this patch tested?

unit tests, manual tests

HddsTestUtils.createStorageReport(UUID.randomUUID(),
"/" + UUID.randomUUID(), 7_000_000);
datanodeInfo.
updateStorageReports(new ArrayList<>(asList(data0, data1, data2)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
updateStorageReports(new ArrayList<>(asList(data0, data1, data2)));
updateStorageReports(asList(data0, data1, data2));

2_000_000, 0, 2_000_000, StorageTypeProto.SSD);
datanodeInfo.
updateMetaDataStorageReports(
new ArrayList<>(Arrays.asList(meta0)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
new ArrayList<>(Arrays.asList(meta0)));
asList(meta0));

2_000_000, 0, 2_000_000, StorageTypeProto.SSD);
datanodeInfo.
updateMetaDataStorageReports(
new ArrayList<>(Arrays.asList(meta0)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
new ArrayList<>(Arrays.asList(meta0)));
asList(meta0));

"/" + UUID.randomUUID(), 6_000_000, 0, 6_000_000,
StorageTypeProto.SSD);
datanodeInfo.
updateStorageReports(new ArrayList<>(asList(data0, data1)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
updateStorageReports(new ArrayList<>(asList(data0, data1)));
updateStorageReports(asList(data0, data1));

2_000_000, 0, 2_000_000, StorageTypeProto.SSD);
datanodeInfo.
updateMetaDataStorageReports(
new ArrayList<>(Arrays.asList(meta0)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
new ArrayList<>(Arrays.asList(meta0)));
asList(meta0));


NodeManager mockNodeManager = Mockito.mock(NodeManager.class);
when(mockNodeManager.getNodes(NodeStatus.inServiceHealthy()))
.thenReturn(new ArrayList<>(datanodes));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.thenReturn(new ArrayList<>(datanodes));
.thenReturn(asList(datanodes));

"/" + UUID.randomUUID(), 6_000_000, 0, 6_000_000,
StorageTypeProto.SSD);
datanodeInfo.
updateStorageReports(new ArrayList<>(asList(data0, data1)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
updateStorageReports(new ArrayList<>(asList(data0, data1)));
updateStorageReports(asList(data0, data1));

2_000_000, 0, 2_000_000, StorageTypeProto.DISK);
datanodeInfo.
updateMetaDataStorageReports(
new ArrayList<>(Arrays.asList(meta0, meta1)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
new ArrayList<>(Arrays.asList(meta0, meta1)));
asList(meta0, meta1));

HddsTestUtils.createStorageReport(UUID.randomUUID(),
"/" + UUID.randomUUID(), 7_000_000);
datanodeInfo.
updateStorageReports(new ArrayList<>(asList(data0, data1, data2)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
updateStorageReports(new ArrayList<>(asList(data0, data1, data2)));
updateStorageReports(asList(data0, data1, data2));

super(new RoundRobinVolumeChoosingPolicy(),
volume -> volume.getStorageType() == StorageType.SSD);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a new line required after the class definition?

@sodonnel
Copy link
Contributor

sodonnel commented Jun 9, 2023

I only quickly looked over the code, but I was wondering how this is intended to be used?

There doesn't seem to be any way to direct certain containers to SSD / RAM / HDD? It appears you set the placement policy on all or some DNs to be SSD / HDD and then only on replication the containers will goto that type of disk on the target DN. Replication only happens if something goes wrong on the cluster (eg node goes down, unhealthy container etc) or the balancer is run. If open containers are created on HDD and then the replication policy says SSD, containers are just going to be on a mixture of disk types with no real control over where they are going or when they might move some replicas to a different storage type.

I wonder if we need a wider design around storage policies, as it is a more difficult problem in Ozone than in HDFS. For example in HDFS a block is a standalone entity associated with a file. That file can have a storage policy set against it. In Ozone, a block is in a container which is shared by many different keys of the same replication type. It is not so simple to say a given key should be marked HOT or Archive it affects the entire contents of the container.

I also think there is some parallel work going on with the data temperature heat map, and using that to move containers around, but I don't think I have seen anything about how that heat map is going to deal with containers holding a mix of hot / cold / archive data.

@vtutrinov
Copy link
Contributor

@sodonnel design doc of the feature is here
The following PR to be discussed is - #4895

@guohao-rosicky
Copy link
Contributor

Can ssd be temporary storage and call disk balance to move the container on ssd to hdd when the container is shut down. I think an option can be added for configuration

@vtutrinov
Copy link
Contributor

@guohao-rosicky This is the plan we're following:

  1. Write new data to SSD
  2. Move closed containers to DISK (HDD) volumes
  3. Replicate the containers from (e.g.) broken datanodes to HDD volumes of healthy ones

@adoroszlai
Copy link
Contributor

/pending

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

/pending

@github-actions
Copy link

github-actions bot commented Nov 6, 2023

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."

@github-actions github-actions bot closed this Nov 6, 2023
@guohao-rosicky
Copy link
Contributor

@guohao-rosicky This is the plan we're following:

  1. Write new data to SSD
  2. Move closed containers to DISK (HDD) volumes
  3. Replicate the containers from (e.g.) broken datanodes to HDD volumes of healthy ones

@vtutrinov @Cyrill Do you want to continue this task? That's a great idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants