Randomize managed volume copy host by mlsorensen · Pull Request #5789 · apache/cloudstack

mlsorensen · 2021-12-18T17:08:29Z

Managed volume copy was always returning first host that could see storage pools
Fix null value in logging for ScaleIOPrimaryDataStoreDriver due to if/else logic error

Signed-off-by: Marcus Sorensen mls@apple.com

Description

This PR fixes #5788

It shuffles the list of valid host candidates that are used for processing volume migrations.

It also fixes a logic error in debug logging that caused 'null' to be printed. Originally concatenating a static string with potentially null value and then testing to see if the result is null, which would never be true.

A more ideal solution might be to track the jobs on each host and attempt to balance the load according to how busy each host is. This was investigated and there's currently no obvious way to look up job assignments to hosts via the DB, the Commands sent to Agents are kept live in memory via the AgentAttache. The ideal solution would be a much larger feature to coordinate Command assignments across the management server cluster, and possibly need improved agent comms to provide live status of Command progress.

I additionally spent some time looking into how to isolate and unit test findUpAndEnabledHostWithAccessToStoragePools(). Currently it would require mocking three Daos, the DataStoreProviderManager, a DataStoreProvider, and a DataStoreDriver. It's possible to write a test for this, however I was concerned that the unit test would be coding implementation across three methods into the test. These mocked Daos exist up to three methods deep - if the implementation of any of these methods changes then the test needs to be rewritten to code in the new implementation. I wasn't sure this was the right thing to do just to add a simple one-line shuffle but I'm happy to go that route if needed.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

How Has This Been Tested?

Have run this code against a live environment and tested volume migration.

sureshanaparti

thanks for the PR @mlsorensen , code LGTM

sureshanaparti · 2021-12-20T05:26:48Z

Hi @mlsorensen I think, this is good to go in 4.16.1. Can you rebase with 4.16 branch. Thanks.

sureshanaparti · 2021-12-20T07:10:59Z

@blueorangutan package

blueorangutan · 2021-12-20T07:12:04Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-12-20T08:08:51Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1956

* Managed volume copy was always returning first host that could see storage pools * Fix null value in logging for ScaleIOPrimaryDataStoreDriver due to if/else logic error Signed-off-by: Marcus Sorensen <mls@apple.com>

mlsorensen · 2021-12-20T16:19:57Z

Hi @mlsorensen I think, this is good to go in 4.16.1. Can you rebase with 4.16 branch. Thanks.

Done. Do I need a separate PR for main or will this get into main?

sureshanaparti · 2021-12-21T04:39:05Z

Hi @mlsorensen I think, this is good to go in 4.16.1. Can you rebase with 4.16 branch. Thanks.

Done. Do I need a separate PR for main or will this get into main?

Thanks @mlsorensen , No need of separate PR for main, this will be forward merged to main.

sureshanaparti · 2021-12-21T04:39:16Z

@blueorangutan package

sureshanaparti · 2021-12-21T05:07:31Z

@blueorangutan package

blueorangutan · 2021-12-21T05:08:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-12-21T05:34:25Z

Packaging result: ✖️ el7 ✔️ el8 ✖️ debian ✖️ suse15. SL-JID 1971

shwstppr · 2021-12-21T06:32:05Z

@blueorangutan package

blueorangutan · 2021-12-21T06:33:03Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-12-21T07:58:30Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1973

sureshanaparti · 2021-12-21T08:13:37Z

@blueorangutan test

blueorangutan · 2021-12-21T08:15:03Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

GutoVeronezi · 2021-12-21T14:02:54Z

+        String debugMessage = "Initiating copy from PowerFlex template volume on host ";
+        LOGGER.debug(destHost != null ? debugMessage + destHost.getId() : debugMessage);


We could use String.format here:

Suggested change

String debugMessage = "Initiating copy from PowerFlex template volume on host ";

LOGGER.debug(destHost != null ? debugMessage + destHost.getId() : debugMessage);

LOGGER.debug(String.format("Initiating copy from PowerFlex template volume on host %s", destHost != null ? destHost.getId() : ""));

GutoVeronezi · 2021-12-21T14:03:54Z

+        String debugMessage = "Initiating copy from PowerFlex volume on host ";
+        LOGGER.debug(destHost != null ? debugMessage + destHost.getId() : debugMessage);


We also could use String.format here.

GutoVeronezi · 2021-12-21T14:06:05Z

        if (hostIds.isEmpty()) {
            return null;
        }
+        Collections.shuffle(hostIds);


I think we could add some log to this method, about which host was selected (or not).

There's logging elsewhere, where the returned value is used.

I might be wrong on this one, but I think that there are not many logs regarding the chosen host to migrate.
Call hierarchy that I've taken a quick check does not present logs. Assuming your comment, this would be logged later on in the call hierarchy ( I haven't checked all the way). Even if it is, you can see that many exceptions can be thrown not too late after the host ID is retrieved.

> StorageManagerImpl.findUpAndEnabledHostWithAccessToStoragePools(List<Long>) (com.cloud.storage) > VolumeServiceImpl.copyManagedVolume(VolumeInfo, DataStore) (org.apache.cloudstack.storage.volume) > VolumeServiceImpl.copyVolume(VolumeInfo, DataStore) (org.apache.cloudstack.storage.volume) > VolumeOrchestrator.copyVolumeFromSecToPrimary(VolumeInfo, VirtualMachine, VirtualMachineTemplate, DataCenter, Pod, Long, ServiceOffering, ...) (org.apache.cloudstack.engine.orchestration) ... > VolumeOrchestrator.migrateVolume(Volume, StoragePool) (org.apache.cloudstack.engine.orchestration) ... > VolumeApiServiceImpl.orchestrateExtractVolume(long, long) (com.cloud.storage) ...

Additionally, the canHostAccessStoragePools called inside the findUpAndEnabledHostWithAccessToStoragePools can return false. If the host cannot access, not much is logged as well.

Please, let us know in case we are missing something here.

The logging is not of the randomised list but the selected host is logged by the deployment planner or during provisioning or volume operation.

blueorangutan · 2021-12-21T17:29:04Z

Trillian test result (tid-2698)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31743 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5789-t2698-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

Signed-off-by: Marcus Sorensen <mls@apple.com>

sureshanaparti · 2021-12-22T04:35:21Z

@blueorangutan package

blueorangutan · 2021-12-22T04:36:04Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-12-22T05:31:30Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1985

DaanHoogland

clgtm (none blocking suggestion added for the log message)

DaanHoogland · 2021-12-22T08:48:34Z

    private Answer copyTemplateToVolume(DataObject srcData, DataObject destData, Host destHost) {
        // Copy PowerFlex/ScaleIO template to volume
-        LOGGER.debug("Initiating copy from PowerFlex template volume on host " + destHost != null ? destHost.getId() : "");
+        LOGGER.debug(String.format("Initiating copy from PowerFlex template volume on host %s", destHost != null ? destHost.getId() : ""));


Suggested change

LOGGER.debug(String.format("Initiating copy from PowerFlex template volume on host %s", destHost != null ? destHost.getId() : ""));

LOGGER.debug(String.format("Initiating copy from PowerFlex template volume on host %s", destHost != null ? destHost.getId() : "<unknown>"));

Sure, why not?

Implementing this. Going to use "not specified" @DaanHoogland since that sounds less like an error. There is code later to resolve a destination host if this is not specified.

DaanHoogland · 2021-12-22T08:48:45Z

    private Answer copyVolume(DataObject srcData, DataObject destData, Host destHost) {
        // Copy PowerFlex/ScaleIO volume
-        LOGGER.debug("Initiating copy from PowerFlex volume on host " + destHost != null ? destHost.getId() : "");
+        LOGGER.debug(String.format("Initiating copy from PowerFlex template volume on host %s", destHost != null ? destHost.getId() : ""));


Signed-off-by: Marcus Sorensen <mls@apple.com>

yadvr · 2021-12-23T10:07:41Z

@blueorangutan package

blueorangutan · 2021-12-23T10:08:03Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-12-23T11:09:03Z

Packaging result: ✖️ el7 ✔️ el8 ✖️ debian ✔️ suse15. SL-JID 2003

sureshanaparti · 2021-12-23T12:13:57Z

@blueorangutan package

blueorangutan · 2021-12-23T12:15:04Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

weizhouapache

code LGTM !

blueorangutan · 2021-12-23T13:34:08Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2007

sureshanaparti · 2021-12-24T06:37:02Z

@blueorangutan test keepEnv

blueorangutan · 2021-12-24T06:38:03Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-12-24T15:36:32Z

Trillian test result (tid-2738)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30871 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5789-t2738-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

GabrielBrascher

Thanks for the PR, @mlsorensen.
Overall looks good, I just added a comment related to the @GutoVeronezi's log suggestion.

I particularly struggle sometimes to debug CloudStack issues due to the lack of information in the logs and it could be that this flow is one of these cases. I am not sure though, so I would need to double-check on this one.

I appreciate any feedback regarding it.

GabrielBrascher · 2021-12-28T14:52:22Z

        if (hostIds.isEmpty()) {
            return null;
        }
+        Collections.shuffle(hostIds);


I might be wrong on this one, but I think that there are not many logs regarding the chosen host to migrate.
Call hierarchy that I've taken a quick check does not present logs. Assuming your comment, this would be logged later on in the call hierarchy ( I haven't checked all the way). Even if it is, you can see that many exceptions can be thrown not too late after the host ID is retrieved.

> StorageManagerImpl.findUpAndEnabledHostWithAccessToStoragePools(List<Long>) (com.cloud.storage) > VolumeServiceImpl.copyManagedVolume(VolumeInfo, DataStore) (org.apache.cloudstack.storage.volume) > VolumeServiceImpl.copyVolume(VolumeInfo, DataStore) (org.apache.cloudstack.storage.volume) > VolumeOrchestrator.copyVolumeFromSecToPrimary(VolumeInfo, VirtualMachine, VirtualMachineTemplate, DataCenter, Pod, Long, ServiceOffering, ...) (org.apache.cloudstack.engine.orchestration) ... > VolumeOrchestrator.migrateVolume(Volume, StoragePool) (org.apache.cloudstack.engine.orchestration) ... > VolumeApiServiceImpl.orchestrateExtractVolume(long, long) (com.cloud.storage) ...

Additionally, the canHostAccessStoragePools called inside the findUpAndEnabledHostWithAccessToStoragePools can return false. If the host cannot access, not much is logged as well.

Please, let us know in case we are missing something here.

GabrielBrascher

Code LGTM

sureshanaparti added this to the 4.16.1.0 milestone Dec 20, 2021

sureshanaparti approved these changes Dec 20, 2021

View reviewed changes

sureshanaparti changed the base branch from main to 4.16 December 20, 2021 05:24

sureshanaparti changed the base branch from 4.16 to main December 20, 2021 05:25

apache deleted a comment from blueorangutan Dec 20, 2021

DaanHoogland added the status:needs-testing label Dec 20, 2021

mlsorensen force-pushed the merge-shuffle-volume-migration branch from 9b73c17 to 53e5c41 Compare December 20, 2021 16:15

Randomize managed volume copy host

1d1e128

* Managed volume copy was always returning first host that could see storage pools * Fix null value in logging for ScaleIOPrimaryDataStoreDriver due to if/else logic error Signed-off-by: Marcus Sorensen <mls@apple.com>

mlsorensen force-pushed the merge-shuffle-volume-migration branch from 53e5c41 to 1d1e128 Compare December 20, 2021 16:17

mlsorensen changed the base branch from main to 4.16 December 20, 2021 16:18

sureshanaparti requested review from DaanHoogland, harikrishna-patnala, shwstppr and yadvr December 21, 2021 04:39

GutoVeronezi reviewed Dec 21, 2021

View reviewed changes

Use String.format for ScaleIO debug message

0a39c33

Signed-off-by: Marcus Sorensen <mls@apple.com>

DaanHoogland approved these changes Dec 22, 2021

View reviewed changes

Update debug message for ScaleIO copy methods

919d66f

Signed-off-by: Marcus Sorensen <mls@apple.com>

yadvr approved these changes Dec 23, 2021

View reviewed changes

weizhouapache approved these changes Dec 23, 2021

View reviewed changes

apache deleted a comment from blueorangutan Dec 24, 2021

GabrielBrascher reviewed Dec 28, 2021

View reviewed changes

GabrielBrascher approved these changes Dec 30, 2021

View reviewed changes

yadvr merged commit dcdcd09 into apache:4.16 Dec 30, 2021

yadvr mentioned this pull request Dec 30, 2021

Parallel volume migrations on KVM are processed on same host #5788

Closed

DaanHoogland removed the status:needs-testing label Jan 28, 2026

		String debugMessage = "Initiating copy from PowerFlex template volume on host ";
		LOGGER.debug(destHost != null ? debugMessage + destHost.getId() : debugMessage);

	LOGGER.debug(String.format("Initiating copy from PowerFlex template volume on host %s", destHost != null ? destHost.getId() : ""));
	LOGGER.debug(String.format("Initiating copy from PowerFlex template volume on host %s", destHost != null ? destHost.getId() : "<unknown>"));

Conversation

mlsorensen commented Dec 18, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

How Has This Been Tested?

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Dec 20, 2021

Uh oh!

sureshanaparti commented Dec 20, 2021

Uh oh!

blueorangutan commented Dec 20, 2021

Uh oh!

blueorangutan commented Dec 20, 2021

Uh oh!

mlsorensen commented Dec 20, 2021

Uh oh!

sureshanaparti commented Dec 21, 2021

Uh oh!

sureshanaparti commented Dec 21, 2021

Uh oh!

sureshanaparti commented Dec 21, 2021

Uh oh!

blueorangutan commented Dec 21, 2021

Uh oh!

blueorangutan commented Dec 21, 2021

Uh oh!

shwstppr commented Dec 21, 2021

Uh oh!

blueorangutan commented Dec 21, 2021

Uh oh!

blueorangutan commented Dec 21, 2021

Uh oh!

sureshanaparti commented Dec 21, 2021

Uh oh!

blueorangutan commented Dec 21, 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!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Dec 21, 2021

Uh oh!

sureshanaparti commented Dec 22, 2021

Uh oh!

blueorangutan commented Dec 22, 2021

Uh oh!

blueorangutan commented Dec 22, 2021

Uh oh!

DaanHoogland 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!

yadvr commented Dec 23, 2021

Uh oh!

blueorangutan commented Dec 23, 2021

yadvr Dec 30, 2021 •

edited

Loading