Skip to content

Copy template to target KVM host if needed when migrating local <> local storage - #3154

Merged
GabrielBrascher merged 4 commits into
apache:masterfrom
CLDIN:copy-template-on-host-kvm-local-migration
Feb 5, 2019
Merged

Copy template to target KVM host if needed when migrating local <> local storage#3154
GabrielBrascher merged 4 commits into
apache:masterfrom
CLDIN:copy-template-on-host-kvm-local-migration

Conversation

@GabrielBrascher

@GabrielBrascherGabrielBrascher commented Jan 28, 2019

Copy link
Copy Markdown
Member

PR #2997 allowed KVM VM live migration with ROOT volume on local storage (filesystem), if the template is already on the target host, the migration will be successful; however, if the VM template is not on the target host the migration fails.

This PR fixes the bug introduced by #2997, copying the VM template to the target host if needed.

Description

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)

Screenshots (if appropriate):

How Has This Been Tested?

  • Tested migrating a VM to a host that did not download the template to its primary storage. The added code is then executed, copying the template to the target host/primary storage; after migrating the template, the execution flows keep as it was previously and the migration is succesfull.
    Additionally, table template_spool_ref is correctly updated with the template reference matching with the target storage pool.
  • Successfully migrated a VM in the case where both storage pools (source and destiny host) have the VM template.

@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan package

@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@dhlaluku I could not add you on the requested reviewers list then I am pinging here ;)

@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2576

@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@GabrielBrascher
GabrielBrascherforce-pushed the copy-template-on-host-kvm-local-migration branch 4 times, most recently from 7bc0d48 to 8ec40e8CompareJanuary 29, 2019 03:56

@DaanHooglandDaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

overall neat code. some questions though


/**
* If the template is not on the target primary storage then it migrates the template.
* Otherwise the execution flow fails during migration and creates a null file created on the target storage pool.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this comments confuses me. you wrote in the description that the template would be copied and this problem would no longer be there. if that is true this "Otherwise ..." is not true or badly formulated. Can you explain?
As it is written now it seems that the condition after "Otherwise" can still occur.
If I understand this correctly you should start with "If we don't do this ...", or "This is needed to prevent ...". Am I correct?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

You are right @DaanHoogland. I will remove the Otherwise the execution flow fails during migration and creates a null file created on the target storage pool. from the Javadoc and leave just the If the template is not on the target primary storage then it migrates the template.. Thanks for pointing it!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done!

if (copyCommandAnswer.getDetails() != null) {
failureDetails = "; details: " + copyCommandAnswer.getDetails();
}
LOGGER.debug(String.format("Failed to copy template [id=%s, name=%s] to the primary storage pool [id=%s]%s", sourceTemplate.getId(), sourceTemplate.getName(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems like an 'ERROR' to me

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I will update this to 'ERROR' I will also extract it to a method that returns the message, avoiding duplicated code.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fixed!

Answer copyCommandAnswer = agentManager.send(destHost.getId(), copyCommand);
logInCaseOfTemplateCopyFailure(copyCommandAnswer, sourceTemplate, destDataStore);
} catch (AgentUnavailableException | OperationTimedoutException e) {
throw new CloudRuntimeException(String.format("Failed to copy template [id=%s, name=%s] to the primary storage pool [id=%s].", sourceTemplate.getId(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is this string constructed twice, here and in below logInCaseOfTemplateCopyFailure()?
they seem to describe the exact same thing

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Extracted to a method 👍

}

/**
* For this strategy it is not necessary to check and migrate the template; however, classes that extend this one may need to check if the template is on the target storage pool (and if not then migrate) before migrating the VM.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you try to wrap this to fit in a github window?

* For this strategy it is not necessary to check and migrate the template; however, classes that extend this one may need to check if the template is on the target storage pool (and if not then migrate) before migrating the VM.
*/
protected void migrateTemplateToTargetFilesystemStorageIfNeeded(VolumeInfo srcVolumeInfo, DataStore destDataStore, StoragePool destStoragePool, Host destHost) {
// This method is used by classes that extend this one

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am wary of this pattern. Can you explain why you implemented it this way? It seems you are implementing a pattern for genericity while already knowing it is only applicable for localstorage.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@DaanHoogland The class KvmNonManagedStorageDataMotionStrategy was implemented extending the StorageSystemDataMotionStrategy and adding methods such as this one in the superclass execution flow. This was the best that I could think regarding using the current architecture and easying future maintenance and extensions. Ideally, we should refactor this class structure and use proper O.O. However, that was not a simple task, and as such I think it is best executed separated from any new feature introduction.

ConfigKey.Scope.Cluster,
null);

ConfigKey<Integer> PRIMARY_STORAGE_DOWNLOAD_WAIT = new ConfigKey<Integer>("Storage", Integer.class, "primary.storage.download.wait", "10800",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't such a variable already in place somewhere? there are other places where it would have use. If not, I think it should be applied to the initial deploy equally.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Config.PrimaryStorageDownloadWait has this configuration indeed.

I can pick from there then and removed this one. However, on the other hand, Config.PrimaryStorageDownloadWait is deprecated. Should I use Config.PrimaryStorageDownloadWait, or remove the Config.PrimaryStorageDownloadWait and keep with the new ConfigKey<Integer>...?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd say keep your new one and find usages of the old one to unify them. If need be move the key to a more centralised place.

@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-3353)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29187 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3154-t3353-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_primary_storage.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 66 look OK, 4 have error(s)
Only failed tests results shown below:

TestResultTime (s)Test File
test_01_add_primary_storage_disabled_hostError0.89test_primary_storage.py
test_01_primary_storage_nfsError0.15test_primary_storage.py
ContextSuite context=TestStorageTags>:setupError0.26test_primary_storage.py
test_02_list_snapshots_with_removed_data_storeError1.17test_snapshots.py
test_11_migrate_vmError19.01test_vm_life_cycle.py
test_14_secure_to_secure_vm_migrationError61.78test_vm_life_cycle.py
test_01_cancel_host_maintenace_with_no_migration_jobsFailure0.14test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobsError2.40test_host_maintenance.py

@GabrielBrascher

GabrielBrascher commented Jan 29, 2019

Copy link
Copy Markdown
MemberAuthor

@DaanHoogland I will ping you when updating the code. Thanks for the review!

Fix KVM VM local storage live migration by migrating its template to the
target host if needed.
@GabrielBrascher
GabrielBrascherforce-pushed the copy-template-on-host-kvm-local-migration branch from 8ec40e8 to 48c08a2CompareFebruary 1, 2019 06:18
@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-2587

@GabrielBrascher
GabrielBrascherforce-pushed the copy-template-on-host-kvm-local-migration branch from 20550e4 to ce0747dCompareFebruary 1, 2019 10:36
@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-2589

@GabrielBrascher
GabrielBrascherforce-pushed the copy-template-on-host-kvm-local-migration branch from ce0747d to 6507741CompareFebruary 1, 2019 11:28
@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2591

@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-3368)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 19505 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3154-t3368-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_primary_storage.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 66 look OK, 4 have error(s)
Only failed tests results shown below:

TestResultTime (s)Test File
test_01_add_primary_storage_disabled_hostError0.51test_primary_storage.py
test_01_primary_storage_nfsError0.08test_primary_storage.py
ContextSuite context=TestStorageTags>:setupError0.15test_primary_storage.py
test_02_list_snapshots_with_removed_data_storeError1.11test_snapshots.py
test_11_migrate_vmError38.09test_vm_life_cycle.py
test_14_secure_to_secure_vm_migrationError39.51test_vm_life_cycle.py
test_15_secured_to_nonsecured_vm_migrationError78.12test_vm_life_cycle.py
test_01_cancel_host_maintenace_with_no_migration_jobsFailure1.09test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobsError3.24test_host_maintenance.py

@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2592

@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-3369)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 20578 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3154-t3369-kvm-centos7.zip
Smoke tests completed. 70 look OK, 0 have error(s)
Only failed tests results shown below:

TestResultTime (s)Test File

@dhlalukudhlaluku left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@GabrielBrascher

GabrielBrascher commented Feb 4, 2019

Copy link
Copy Markdown
MemberAuthor

Thanks for the review, @DaanHoogland! Are you ok with the updated code addressing your requests?

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2596

@GabrielBrascher

Copy link
Copy Markdown
MemberAuthor

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@GabrielBrascher
GabrielBrascher merged commit 7c5eca9 into apache:masterFeb 5, 2019
@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-3375)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 15686 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3154-t3375-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermittent failure detected: /marvin/tests/smoke/test_reset_vm_on_reboot.py
Intermittent failure detected: /marvin/tests/smoke/test_resource_accounting.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dhcphosts.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dns.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dnsservice.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_iptables_default_policy.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_routers.py
Intermittent failure detected: /marvin/tests/smoke/test_secondary_storage.py
Intermittent failure detected: /marvin/tests/smoke/test_service_offerings.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Intermittent failure detected: /marvin/tests/smoke/test_templates.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 50 look OK, 20 have error(s)
Only failed tests results shown below:

TestResultTime (s)Test File
ContextSuite context=TestResetVmOnReboot>:setupError0.00test_reset_vm_on_reboot.py
ContextSuite context=TestRAMCPUResourceAccounting>:setupError0.00test_resource_accounting.py
ContextSuite context=TestRouterDHCPHosts>:setupError0.00test_router_dhcphosts.py
ContextSuite context=TestRouterDHCPOpts>:setupError0.00test_router_dhcphosts.py
ContextSuite context=TestISOUsage>:setupError0.00test_usage.py
ContextSuite context=TestLBRuleUsage>:setupError0.00test_usage.py
ContextSuite context=TestNatRuleUsage>:setupError0.00test_usage.py
ContextSuite context=TestPublicIPUsage>:setupError0.00test_usage.py
ContextSuite context=TestSnapshotUsage>:setupError0.00test_usage.py
ContextSuite context=TestVmUsage>:setupError0.00test_usage.py
ContextSuite context=TestVolumeUsage>:setupError0.00test_usage.py
ContextSuite context=TestVpnUsage>:setupError0.00test_usage.py
ContextSuite context=TestRouterDns>:setupError0.00test_router_dns.py
ContextSuite context=TestRouterDnsService>:setupError0.00test_router_dnsservice.py
ContextSuite context=TestRouterIpTablesPolicies>:setupError0.00test_routers_iptables_default_policy.py
ContextSuite context=TestVPCIpTablesPolicies>:setupError0.00test_routers_iptables_default_policy.py
test_01_isolate_network_FW_PF_default_routes_egress_trueError0.16test_routers_network_ops.py
test_02_isolate_network_FW_PF_default_routes_egress_falseError0.18test_routers_network_ops.py
ContextSuite context=TestRedundantIsolateNetworks>:setupError1.35test_routers_network_ops.py
ContextSuite context=TestRouterServices>:setupError0.00test_routers.py
test_01_sys_vm_startFailure0.11test_secondary_storage.py
ContextSuite context=TestCpuCapServiceOfferings>:setupError0.00test_service_offerings.py
ContextSuite context=TestServiceOfferings>:setupError0.18test_service_offerings.py
ContextSuite context=TestSnapshotRootDisk>:setupError0.00test_snapshots.py
test_01_list_sec_storage_vmFailure0.05test_ssvm.py
test_02_list_cpvm_vmFailure0.04test_ssvm.py
test_03_ssvm_internalsFailure0.04test_ssvm.py
test_04_cpvm_internalsFailure0.04test_ssvm.py
test_05_stop_ssvmFailure0.04test_ssvm.py
test_06_stop_cpvmFailure0.04test_ssvm.py
test_07_reboot_ssvmFailure0.04test_ssvm.py
test_08_reboot_cpvmFailure0.03test_ssvm.py
test_09_destroy_ssvmFailure0.04test_ssvm.py
test_10_destroy_cpvmFailure0.04test_ssvm.py
test_02_create_template_with_checksum_sha1Error65.46test_templates.py
test_03_create_template_with_checksum_sha256Error65.46test_templates.py
test_04_create_template_with_checksum_md5Error65.44test_templates.py
test_05_create_template_with_no_checksumError65.44test_templates.py
test_02_deploy_vm_from_direct_download_templateError1.28test_templates.py
test_03_deploy_vm_wrong_checksumError1.36test_templates.py
ContextSuite context=TestTemplates>:setupError17.66test_templates.py
ContextSuite context=Test01DeployVM>:setupError0.00test_vm_life_cycle.py
ContextSuite context=Test02VMLifeCycle>:setupError0.00test_vm_life_cycle.py
test_14_secure_to_secure_vm_migrationError11.40test_vm_life_cycle.py
test_15_secured_to_nonsecured_vm_migrationError84.62test_vm_life_cycle.py
test_16_nonsecured_to_secured_vm_migrationError1.26test_vm_life_cycle.py
ContextSuite context=TestVmSnapshot>:setupError2.03test_vm_snapshots.py
ContextSuite context=TestCreateVolume>:setupError0.00test_volumes.py
ContextSuite context=TestVolumes>:setupError0.00test_volumes.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACLError2.93test_vpc_redundant.py
test_02_redundant_VPC_default_routesError1.92test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routersError1.90test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nicsError5.01test_vpc_redundant.py
test_05_rvpc_multi_tiersError2.93test_vpc_redundant.py
test_01_VPC_nics_after_destroyError4.95test_vpc_router_nics.py
test_02_VPC_default_routesError3.96test_vpc_router_nics.py
test_01_redundant_vpc_site2site_vpnFailure2.32test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_optionsFailure3.31test_vpc_vpn.py
test_01_vpc_remote_access_vpnFailure1.18test_vpc_vpn.py
test_01_vpc_site2site_vpnFailure2.31test_vpc_vpn.py

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@GabrielBrascher@blueorangutan@DaanHoogland@rafaelweingartner@dhlaluku