HDDS-6070. ContainerBalancerConfig doesn't read config from ozone-site.xml #2893

symious · 2021-12-07T08:36:27Z

What changes were proposed in this pull request?

Issue happens when we try to update the config of "hdds.container.balancer.utilization.threshold", but the value from the SCM log said it's always the "0.1", which is the default value.

After checking the code, the construction of ContainerBalancerConfiguration doesn't comply with the pattern of "ConfigGroup", this ticket is to fix the issue to update the configuration from config file.

Since the class needs to hold OzoneConfiguration for configuration check, the method of "initialize" is created for this requirement.

What is the link to the Apache JIRA

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

How was this patch tested?

unit test

…e.xml

symious · 2021-12-07T08:37:02Z

@siddhantsangwan Could you help to review this PR?

siddhantsangwan · 2021-12-08T06:06:17Z

@symious Thanks for finding this bug. Looking into it.

symious · 2021-12-10T02:18:08Z

@siddhantsangwan Thank you for the review.
@JacksonYao287 Could you also help to check the PR?

symious · 2021-12-10T06:43:57Z

.../main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java

    long size = (long) ozoneConfiguration.getStorageSize(
        ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
-        ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.GB) +
+        ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES) +


@siddhantsangwan Another issue is the StorageUnit here.
The value from Unit.GB is "5 + 1GB", need to changed to Unit.BYTES.

Yes, I was just going to raise a jira for this particular bug when I saw that you've pushed an update. Thanks!

Seems the other configs, like "move.timeout" and "balancing.interval", also have the same issue.

siddhantsangwan · 2021-12-10T06:56:09Z

I have one initial observation that could be a problem. The pattern of

this.config = ozoneConfiguration.getObject(ContainerBalancerConfiguration.class);
config.initialize(ozoneConfiguration);

results in overriding the values of size.entering.target.max, size.leaving.source.max, and balancing.iteration.interval.
For example, if we set the value of size entering target using ozone configuration, the following config.initialize invocation would overwrite that value.

symious · 2021-12-10T07:11:15Z

@siddhantsangwan Thanks for the review.

Yes, and in fact the other initialize method seems a little odd. I added the method because the original ContainerBalancerConfiguration is constructed with OzoneConfiguration, which is to override those configurations on purpose.

I think the root problem is that some values of ContainerBalancerConfiguration are dependent on other configurations, like "size.entering.target.max" is depending on "OZONE_SCM_CONTAINER_SIZE_DEFAULT", but the current ConfigGroup annotation doesn't support this requirement.

JacksonYao287

@symious thanks for the work , the changes looks good !
i left a comment , please take a look

JacksonYao287 · 2021-12-13T06:32:49Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

+    this.config = ozoneConfiguration.
+        getObject(ContainerBalancerConfiguration.class);
+    config.initialize(ozoneConfiguration);


may be we can add a static build function to ContainerBalancerConfiguration class to build and initialize a ContainerBalancerConfiguration instance, for example:

public static ContainerBalancerConfiguration buildFrom(ozoneConfiguration oc) { ..... }

siddhantsangwan · 2021-12-13T17:07:21Z

I think the root problem is that some values of ContainerBalancerConfiguration are dependent on other configurations, like "size.entering.target.max" is depending on "OZONE_SCM_CONTAINER_SIZE_DEFAULT", but the current ConfigGroup annotation doesn't support this requirement.

Yeah. I can think of the following solutions:

Add a static method that accepts OzoneConfiguration and returns a ContainerBalancerConfiguration by calling a private constructor. This constructor can initialize our special configs if we pass the OzoneConfiguration from the static method. We will also need to check if these values have already been set in ozone-site.xml to avoid overriding them.

may be we can add a static build function to ContainerBalancerConfiguration class to build and initialize a ContainerBalancerConfiguration instance

This seems similar to what @JacksonYao287 is suggesting.

Maybe we can initialize these special values in their respective getters instead? Again, we'll first need to ensure these configs haven't already been set.
We could also simplify it and just put default values in the @Config annotations. For example, 70 minutes for iteration interval, 26GB for max size entering target etc. What do you all think? cc @lokeshj1703

symious · 2021-12-15T10:28:23Z

@siddhantsangwan @JacksonYao287 Thanks for the reply.

I think Option 1 is the original implementation. If we initialize ContainerBalancerConfiguration based on OzoneConfiguration, the @ConfigGroup pattern won't work, that is the content of ozone-site.xml won't be projected to ContainerBalancerConfiguration. AFAIK, we need to use ozoneConfiguration.getObject(ContainerBalancerConfiguration.class) to load the value from ozone-site.xml.

I think Option 3 might be a good choice. We can let ContainerBalancerConfiguration only handle the configs from ozone-site.xml, the validation and check of configs can be handled when starting container balancer service..

lokeshj1703 · 2021-12-15T11:24:47Z

Thanks @symious for working on this! I would like to add one more option here. ozoneConfiguration.getObject(ContainerBalancerConfiguration.class) We can try overriding this function to allow using class constructor which takes OzoneConfiguration as input.

siddhantsangwan · 2021-12-16T10:08:19Z

I think Option 3 might be a good choice.

I agree, it's the simplest. The default values could be:

size.entering.target.max = 26GB
size.leaving.source.max = 26GB
balancing.iteration.interval = 70 minutes

Some very related changes are being made in #2892

symious · 2021-12-16T15:41:19Z

@lokeshj1703 Thanks for the review.

We can try overriding this function to allow using class constructor which takes OzoneConfiguration as input.

The current implementation is inherited from ConfigurationSource as follows

default <T> T getObject(Class<T> configurationClass) {
    T configObject;
    try {
      configObject = configurationClass.newInstance();
    } catch (InstantiationException | IllegalAccessException e) {
      throw new ConfigurationException(
          "Configuration class can't be created: " + configurationClass, e);
    }

If we are going to override this method, it may look like "first try configurationClass.getDeclaredConstructor(OzoneConfiguration.class).newInstance(ozoneConfiguration), if not exists, then try configurationClass.newInstance()". Personally I think the implementation would look a little odd.

symious · 2021-12-16T15:41:49Z

@siddhantsangwan Sure, will update the default value in the next commit.

symious · 2021-12-20T02:44:14Z

@siddhantsangwan Update the commit. Could you help to check?

Added validateConfiguration in ContainerBalancer, it can return boolean to indicate an incorrect configuration, but many changes in test cases need to be made, for now, I leave the method to be void.

siddhantsangwan

These changes look great @symious . I have a few minor comments.

siddhantsangwan · 2021-12-20T06:30:34Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

+    long size = (long) ozoneConfiguration.getStorageSize(
+        ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
+        ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES) +
+        OzoneConsts.GB;


We don't need to add another GB here. The configs just need to be greater than size.

siddhantsangwan · 2021-12-20T06:32:21Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

+        ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES) +
+        OzoneConsts.GB;
+
+    if (conf.getMaxSizeEnteringTarget() < size) {


This should be the check if another GB isn't added to size (as commented above)

Suggested change

if (conf.getMaxSizeEnteringTarget() < size) {

if (conf.getMaxSizeEnteringTarget() <= size) {

siddhantsangwan · 2021-12-20T06:32:44Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

+      LOG.info("MaxSizeEnteringTarget should be larger than " +
+          "ozone.scm.container.size");
+    }
+    if (conf.getMaxSizeLeavingSource() < size) {


Same as above

Suggested change

if (conf.getMaxSizeLeavingSource() < size) {

if (conf.getMaxSizeLeavingSource() <= size) {

siddhantsangwan · 2021-12-20T06:34:07Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

+    // balancing interval should be greater than DUFactory refresh period
+    DUFactory.Conf duConf = ozoneConfiguration.getObject(DUFactory.Conf.class);
+    long balancingInterval = duConf.getRefreshPeriod().toMillis() +
+        Duration.ofMinutes(10).toMillis();


Again, don't need to add extra 10 minutes since the config only needs to be greater than DU refresh period.

siddhantsangwan · 2021-12-20T06:34:43Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

+      LOG.info("balancing.iteration.interval should be at lease 10 minutes " +
+          "larger than hdds.datanode.du.refresh.period.");


Suggested change

LOG.info("balancing.iteration.interval should be at lease 10 minutes " +

"larger than hdds.datanode.du.refresh.period.");

LOG.info("balancing.iteration.interval should be " +

"larger than hdds.datanode.du.refresh.period.");

siddhantsangwan · 2021-12-20T06:37:22Z

.../main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java

      " by default.")
  private String excludeNodes = "";

  private DUFactory.Conf duConf;


Since this isn't getting initialised now, we can remove this and its usages in the setBalancingInterval method.

siddhantsangwan · 2021-12-20T06:41:32Z

@siddhantsangwan Update the commit. Could you help to check?

Added validateConfiguration in ContainerBalancer, it can return boolean to indicate an incorrect configuration, but many changes in test cases need to be made, for now, I leave the method to be void.

Yes, having it return a boolean makes sense. And sure, we can add this change in a follow-up PR.

symious · 2021-12-20T15:49:31Z

@siddhantsangwan Updated the patch, please have a check.

Yes, having it return a boolean makes sense. And sure, we can add this change in a follow-up PR.

Sure, will create a new ticket for this change.

siddhantsangwan

The update looks good to me.

ferhui · 2021-12-23T10:59:29Z

@lokeshj1703 Can you please take another look?
@symious This PR has conflicts...

JacksonYao287

thanks @symious for updating this patch , i have two comments , please take a look

JacksonYao287 · 2021-12-23T11:17:42Z

...r-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java

+    Assert.assertEquals(cbConf.getThreshold(), 0.01d, DELTA);
+


can we remove DELTA and use assertTrue(Doublu.compare(a,b) == 0)?

I have removed DELTA and use a similar comparison as

ozone/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/placement/TestContainerPlacement.java

Line 76 in 8b4d4a9

assertEquals(beforeCapacity.getStandardDeviation(), beforeRandom

, please have a check.

JacksonYao287 · 2021-12-23T11:23:37Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java

+    if (conf.getMaxSizeEnteringTarget() <= size) {
+      LOG.info("MaxSizeEnteringTarget should be larger than " +
+          "ozone.scm.container.size");
+    }
+    if (conf.getMaxSizeLeavingSource() <= size) {
+      LOG.info("MaxSizeLeavingSource should be larger than " +
+          "ozone.scm.container.size");
+    }
+
+    // balancing interval should be greater than DUFactory refresh period
+    DUFactory.Conf duConf = ozoneConfiguration.getObject(DUFactory.Conf.class);
+    long balancingInterval = duConf.getRefreshPeriod().toMillis();
+    if (conf.getBalancingInterval().toMillis() <= balancingInterval) {
+      LOG.info("balancing.iteration.interval should be larger than " +
+          "hdds.datanode.du.refresh.period.");
+    }


when validating , if we find it is illegal(for example, conf.getMaxSizeEnteringTarget() <= size), should we throw an Exception and just return without starting balancer?

Yes, it was intended to return "false" for these situations, but it will break many test cases.
The changes will be brought up in a new ticket.

symious · 2021-12-25T00:51:53Z

@symious This PR has conflicts...

@ferhui Thanks for the review. Resolved the conflicts.

ferhui · 2021-12-27T09:06:53Z

Will Merge tomorrow if no other comments

ferhui · 2021-12-28T01:47:49Z

@symious Thanks for your contribution. @siddhantsangwan @lokeshj1703 @JacksonYao287 Thanks for your review! Merged

HDDS-6070. ContainerBalancerConfig doesn't read config from ozone-sit…

036f07a

…e.xml

trigger new CI check

0a88420

symious added 2 commits December 10, 2021 14:31

HDDS-6070. Incorrect maxSizeEnteringTarget and maxSizeLeavingSource

4552162

HDDS-6070. Test the value after initilize

f74f473

symious commented Dec 10, 2021

View reviewed changes

symious added 2 commits December 10, 2021 15:41

HDDS-6070. Fix move.timeout

2b0a78c

HDDS-6070. Remove unused imports

17a1a1c

JacksonYao287 reviewed Dec 13, 2021

View reviewed changes

HDDS-6070. Remove OzoneConfiguration from ContainerBalancerConfiguration

19a0b94

siddhantsangwan reviewed Dec 20, 2021

View reviewed changes

HDDS-6070. Fix some comparision

009dc80

siddhantsangwan approved these changes Dec 21, 2021

View reviewed changes

siddhantsangwan mentioned this pull request Dec 23, 2021

HDDS-5927. Improve defaults in ContainerBalancerConfiguration #2892

Merged

JacksonYao287 reviewed Dec 23, 2021

View reviewed changes

Merge branch 'master' into HDDS-6070

cb662c8

HDDS-6070. Remove DELTA

d0d0615

ferhui approved these changes Dec 27, 2021

View reviewed changes

ferhui merged commit b570d0a into apache:master Dec 28, 2021

	if (conf.getMaxSizeEnteringTarget() < size) {
	if (conf.getMaxSizeEnteringTarget() <= size) {

	if (conf.getMaxSizeLeavingSource() < size) {
	if (conf.getMaxSizeLeavingSource() <= size) {

		LOG.info("balancing.iteration.interval should be at lease 10 minutes " +
		"larger than hdds.datanode.du.refresh.period.");

HDDS-6070. ContainerBalancerConfig doesn't read config from ozone-site.xml #2893

HDDS-6070. ContainerBalancerConfig doesn't read config from ozone-site.xml #2893

Uh oh!

Conversation

symious commented Dec 7, 2021

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

symious commented Dec 7, 2021

Uh oh!

siddhantsangwan commented Dec 8, 2021

Uh oh!

symious commented Dec 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddhantsangwan commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

symious commented Dec 10, 2021

Uh oh!

JacksonYao287 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddhantsangwan commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

symious commented Dec 15, 2021

Uh oh!

lokeshj1703 commented Dec 15, 2021

Uh oh!

siddhantsangwan commented Dec 16, 2021

Uh oh!

symious commented Dec 16, 2021

Uh oh!

symious commented Dec 16, 2021

Uh oh!

symious commented Dec 20, 2021

Uh oh!

siddhantsangwan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddhantsangwan commented Dec 20, 2021

Uh oh!

symious commented Dec 20, 2021

Uh oh!

siddhantsangwan left a comment

Choose a reason for hiding this comment

Uh oh!

ferhui commented Dec 23, 2021

Uh oh!

JacksonYao287 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

siddhantsangwan commented Dec 10, 2021 •

edited

Loading

siddhantsangwan commented Dec 13, 2021 •

edited

Loading