@DaanHoogland Can you clarify few questions/concerns

1. This change will likely to break restoreVirtualMachine functions. At the very least users should be able to turn off new behavior with a global setting that will enable/disable such cleanup.
2. Does this affect a normal template cleanup that will delete both full and linked clone templates from PS when all VMs that use them are gone?
3. You introduce vmware.full.clone.template.cleanup.period as vmware specific setting. Is this only applicable to vmware?
4. Some changes are done to Vmware-Host-Scan task that seems to be unrelated to scope of this PR.

@serg38
ad 1. yes they can, if you look in the ticket there is a part of description that describes this possibility. But that said I don't see how restore is going to be hindered if the vm was created using a full clone. The image is not deleted.
ad 2. No it is only marking templates for gc and not touching any cleanup process, it relies on the regular process to do the actual cleanup. The benefit is that short lived templates that will be replaced by cron jobs for instance will be cleaned without having to wait until the VMs cloned from it are all gone (on a per primary store basis btw)
ad 3. Yes so far it is I have not studied other hypervisors on this area. The code is now in the vmware plugin but not tightly coupled to the vmware API so generalizing it shoud be rather trivial.
ad 4. the host scan task was an empty placeholder with only todo. The vcente clusters are not scanned for new hosts atm. If you look at the old code you can find that the task consists of only a comment:

    private Runnable getHostScanTask() {
        return new Runnable() {
            @Override
            public void run() {
                // TODO scan vSphere for newly added hosts.
                // we are going to both support adding host from CloudStack UI and
                // adding host via vSphere server
                //
                // will implement host scanning later
            }
        };
    }

hope this is explanation to your satisfaction ;)

@DaanHoogland Thanks. But for #1 the requirement for the code changes is that the template is deleted from cloudstack. In this condition restoreVirtualMachines when template ID is not specified will use a local template copy on PS for either full or link clone.

ah ok, I was not aware of that @serg38 . I'll look it up, except from not enabling the feature, how would you handle this?
a. To not allow restore
b. To recognise that the disk is fully cloned and bypass the null-template (if almighty vmware allows)
c. To allow for choosing a successor-template
d. ...
Also, Is this a blocker for you?

@DaanHoogland Not for us since we still use link clones but I am sure tons of other might be affected. Interestingly enough that with PR 1773 merged the default behavior in API is not to allow template deletion if there are active VMs and only if 'forced' flag is used it will be executed
#c is an option already but I suggest one of the following
#d introduce another global config with default value = false e.g. vmware.cleanup.fullclonedtemplate that would be a switch old/new behaviur in combination with storage.template.cleanup.enabled
#e make 0 as a default value for vmware.full.clone.template.cleanup.period and when you interpret 0 it means that full clone cleanup thread is never executed

#e seems to be a good compromise that doesn't require too many changes

@serg38 I am confused by that last comment; storage.template.cleanup.enabled already switches between old and new behaviour. when false/default no extra work is done, when true marking of fully cloned deleted templates is done. So you #d does not make sense to me and #e would be a redundant check. Am I missing something?

@DaanHoogland storage.template.cleanup.enabled is a current global setting so you can't really use it to control a behavior you are about to introduce. For example, I want my template copies on PS to be cleaned up only when no VM using the template left on the particular PS. In this case we need to have both storage.template.cleanup.enabled and storage.cleanup.enabled to be set to 'true'. If we keep logic in your PR in this case it will also start cleaning up templates copies for full clone based templates which might be unwanted in some environments.

@blueorangutan package

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

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

Build fails with Could not find artifact org.apache.cloudstack:cloud-plugin-network-midonet:jar:4.10.0.0-SNAPSHOT as it was just retired.
@DaanHoogland can you please rebase against master, I think that would fix the error.

ok, @serg38 you have convinced me. I do think and extra option is overkill so I'll implement the 0 minutes idea.

@borisstoyanov rebase
@serg38 the 0 period already had that effect due to the library for scheduling but I now put a nice guard around it and made 0 the default
@borisstoyanov please not that I changed the default period from 1440 minutes to 0 have a look at e3ae25a

@DaanHoogland Thanks. LGTM on the code changes

@blueorangutan package

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

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

@blueorangutan test centos7 vmware-55u3

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests

Trillian test result (tid-1008)
Environment: vmware-55u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 49659 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2044-t1008-vmware-55u3.zip
Intermitten failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermitten failure detected: /marvin/tests/smoke/test_routers.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Test completed. 45 look ok, 6 have error(s)

@blueorangutan package

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

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

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests

Trillian test result (tid-1032)
Environment: vmware-55u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42738 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2044-t1032-vmware-55u3.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Test completed. 48 look ok, 3 have error(s)

@blueorangutan package

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

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

@blueorangutan test

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

Trillian test result (tid-1053)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36567 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2044-t1053-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Test completed. 50 look ok, 1 have error(s)

ACS CI BVT Run

Sumarry:
Build Number 800
Hypervisor xenserver
NetworkType Advanced
Passed=106
Failed=4
Skipped=12

Link to logs Folder (search by build_no): https://www.dropbox.com/sh/r2si930m8xxzavs/AAAzNrnoF1fC3auFrvsKo_8-a?dl=0

Failed tests:

• test_list_ids_parameter.py

• ContextSuite context=TestListIdsParams>:setup Failing since 27 runs

• test_volumes.py

• test_06_download_detached_volume Failing since 3 runs

• test_routers_network_ops.py

• test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true Failing since 4 runs

• test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false Failed

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_01_verify_libvirt
test_02_verify_libvirt_after_restart
test_03_verify_libvirt_attach_disk
test_04_verify_guest_lspci
test_05_change_vm_ostype_restart
test_06_verify_guest_lspci_again
test_static_role_account_acls
test_11_ss_nfs_version_on_ssvm
test_nested_virtualization_vmware
test_3d_gpu_support
test_deploy_vgpu_enabled_vm

Passed test suits:
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vm_snapshots.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_loadbalance.py
test_routers.py
test_reset_vm_on_reboot.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_router_dns.py
test_non_contigiousvlan.py
test_login.py
test_deploy_vm_iso.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_metrics_api.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py

@blueorangutan test centos7 vmware-55u3

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests

Trillian test result (tid-1168)
Environment: vmware-55u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48141 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2044-t1168-vmware-55u3.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_vgpu_enabled_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermitten failure detected: /marvin/tests/smoke/test_service_offerings.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Test completed. 48 look ok, 3 have error(s)

two LGM and clean rebase pushed; merging

@DaanHoogland can you explain (1) why this PR was merged without running latest tests, (2) last test results are at least two months old, master has changed a lot recently, (3) lastly the commits are not squashed and do not have JIRA ids in them.

sure i can: two LGM and clean rebase pushed. commitws should not be squashed and There is a hira id in the PR title, hence merge commit

@blueorangutan package

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

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