removed unused HypervDummyResourceBase class

[pedro-martins]

Was removed the class com.cloud.hypervisor.hyperv.resource.HypervDummyResourceBase

• This class do not have the annotation @component and there are no beans that instantiate this class in XML files, then this class isn't in the spring lifecycle;
• The constructor of this class isn't called in the entire ACS code;
• There is no other class that extends this class.

Well, I think that this class isn't used in the ACS code, but if I'm wrong, please be free to comment.

Ty.

[terbolous]

Have you done any testing?

[rafaelweingartner]

@eriweb
There are things that do not need to be tested, we work in computer science; if there is no reference to an object/class, there is no way it is being used.

The only reference for that class name is in a log at line 250 of class “com.cloud.hypervisor.hyperv.discoverer.HypervServerDiscoverer”, the log says that it is creating the dummyResource, however, it creates the “com.cloud.hypervisor.hyperv.resource.HypervDirectConnectResource” at line 282.

This is a fair and simple removal of unused code/class. @pedro-martins , could you also fix the log message in “HypervServerDiscoverer” at line 250?

[pedro-martins]

@rafaelweingartner ty for your review.

Changed the log message.

Ty.

[terbolous]

@rafaelweingartner that's not really an answer, and I stand my question.

Have you done any testing? does the code compile? does unit tests run?

The code removed might not be used, but in a big sized project like this assumption is the mother of (almost) all fuckups.

[pedro-martins]

Hi @eriweb, thanks for reviewing this PR.

I did a maven install in the ACS project and it built and ran all tests cases with no errors. But I didn't the integration test.

Ty.

[rafaelweingartner]

@eriweb I saw the Jenkins build, and it was ok, that meant the code compiled and all of the tests cases (unit and integration tests) were executed properly.

I noticed that the functional tests with Marvin were not executed (there is not CI in place right now), but I did not find them needed for this kind of contribution.

Additionally, I know that mistakes can happen if the code is not properly tested and reviewed. I just said that it did not need functional testing after getting the code, and carefully checking it myself; that is how I found the only reference to that class in a log message.

@pedro-martins LGTM to this PR

[GabrielBrascher]

Given that @pedro-martins had no problems on running tests, the code LGTM.

[remibergsma]

@GabrielBrascher based on what tests? It was only compiled.

I'm with @eriweb here, this should not go in without testing. Ask on the ML if someone with HyperV can confirm it still works and run at least vm_life_cycle integration tests.

@rafaelweingartner "There are things that do not need to be tested"? C'mon please.

To be clear: -1:-1: on this until we have done integration testing.

[rafaelweingartner]

@remibergsma,
Let’s be reasonable, when you remove a method/class or something else, that do not have any reference anywhere in the code, in my opinion, we do not need functional tests (the tests you call integration).
When Jenkins is running the maven build, it is already running unit and integration test cases (test cases written in Java using mocks and other tricks to test the code). In my opinion that would be enough.

I respect a lot your work here, but I really do not want to prolong this discussion; it seems that it was taken too far.

Let’s wait until someone execute the functional tests.

[remibergsma]

I see your point @rafaelweingartner, no hard feelings here. If we simply always run the integration tests, there is no discussion whether or not we need to run them. Otherwise others will also state they don't need testing and we end up with all sorts of nasty bugs. Let someone verify it against a really hyperv box. And if we can't find anyone who cares to do this, I'll +1 a PR that removes hyperv all together ;-).

[rafaelweingartner]

I also got your point ;).
Once you open an exception, there are people that may try to abuse it.
Here we might be able to get our hands on Hyper-V license, I will see if next week I can get someone to create a host with Hyper-V and run the test you mentioned.

[yadvr]

A CI testing (one with HyperV) should confirm if this PR can be accepted

[swill]

I don't have the ability to test HyperV. Can anyone verify this PR for me?

[koushik-das]

@anshul1886@anshulgangwar Can you review this?

[anshul1886]

LGTM. This file seems to be remains from the previous attempt of HyperV support which was talking to SCVMM.

[swill]

I think the only thing we are missing with this one is verification that the change works and does not break anything. @pedro-martins is there anything you can provide to show this is working?

[bvbharatk]

ACS CI BVT Run

Sumarry:
Build Number 144
Hypervisor xenserver
NetworkType Advanced
Passed=72
Failed=1
Skipped=3

Link to logs Folder (search by build_no):https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0

Failed tests:

• test_routers.py
  • test_03_restart_network_cleanup Failed

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm

Passed test suits:
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vpc_vpn.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.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

[cloudmonger]

ACS CI BVT Run

Sumarry:
Build Number 1016
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=9
Skipped=12

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

Failed tests:

• test_loadbalance.py

• test_01_create_lb_rule_src_nat Failed

• test_02_create_lb_rule_non_nat Failed

• test_assign_and_removal_lb Failed

• test_deploy_vm_iso.py

• test_deploy_vm_from_iso Failing since 71 runs

• test_vm_life_cycle.py

• test_10_attachAndDetach_iso Failing since 71 runs

• test_routers_network_ops.py

• test_01_isolate_network_FW_PF_default_routes_egress_true Failing since 104 runs

• test_02_isolate_network_FW_PF_default_routes_egress_false Failing since 104 runs

• test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true Failing since 102 runs

• test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false Failing since 102 runs

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_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_list_ids_parameter.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_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_disk_offerings.py

[yadvr]

LGTM, merging.