send unsupported answer only when applicable by DaanHoogland · Pull Request #2714 · apache/cloudstack

DaanHoogland · 2018-06-19T08:17:29Z

Description

A pokemon catch was handling all errors in the agent with libvirt resource loaded.

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)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

rafaelweingartner · 2018-06-19T08:19:31Z

        return true;
    }

+    /**


Big 👍 . We really need more of these in our code base :)
Great job!

rafaelweingartner · 2018-06-19T08:20:27Z

+     * @return the for the {@see Command} appropriate {@see Answer} or {@see UnsupportedAnswer}
+     */
    @Override
    public Answer executeRequest(final Command cmd) {


What about a unit test cases? Then, you can have a test case that detects if we catch only RequestWrapper.CommandNotSupported exceptions.

that would require throwing everything else. I will add a subset to catch the most obvious cases

Ah, yes. If you want to check all possible cases. However, you can test with the most obvious ones such as Nullpointer, Exception, CloudRuntimeException, and RuntimeException

I am not really convinced that any of these make a lot of difference to one and another. How about the known and unknown case I just added?

It helps. I only think that you could go and test the method retryWhenAllFails instead of using executeRequest. If we want unit tests, we should write them for the units.

we could also unittest RequestWrapper, yes.
executeRequest is the unit here.
higher prios came up, i might revisit

No problems ;)
Thanks anyways for writing at least some tests.

rafaelweingartner · 2018-06-19T13:36:05Z


 public abstract class RequestWrapper {
    static public class CommandNotSupported extends NullPointerException {
+        Throwable reason = null;


What about overriding the method getCause to return this value here?

sorry, didn't think of it. should do.

GabrielBrascher

Code LGTM.
The code is documented and with test cases, well done :)

DaanHoogland · 2018-06-19T15:39:47Z

@blueorangutan package

blueorangutan · 2018-06-19T15:40:43Z

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

blueorangutan · 2018-06-19T16:12:27Z

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

PaulAngus · 2018-06-19T18:22:52Z

@blueorangutan test

blueorangutan · 2018-06-19T18:23:42Z

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

blueorangutan · 2018-06-20T05:50:55Z

Trillian test result (tid-2773)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39498 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2714-t2773-kvm-centos7.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_public_ip_range.py
Intermitten failure detected: /marvin/tests/smoke/test_templates.py
Intermitten failure detected: /marvin/tests/smoke/test_usage.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 59 look OK, 8 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	172.00	test_privategw_acl.py
test_04_extract_template	`Failure`	128.34	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
test_02_not_secured_vm_migration	`Error`	6.22	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	4.15	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	1.09	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	6.25	test_vm_life_cycle.py
test_06_download_detached_volume	`Failure`	138.59	test_volumes.py
test_01_cancel_host_maintenace_with_no_migration_jobs	`Failure`	0.09	test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	39.95	test_host_maintenance.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	3.46	test_hostha_kvm.py
test_hostha_kvm_host_fencing	`Error`	8.61	test_hostha_kvm.py

yadvr · 2018-06-20T05:58:59Z

@blueorangutan test

blueorangutan · 2018-06-20T05:59:42Z

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

borisstoyanov

LGTM

PaulAngus · 2018-06-20T08:16:57Z

@borisstoyanov what is your LGTM based on?

borisstoyanov · 2018-06-20T08:41:03Z

@PaulAngus, I've did a code review, and noticed that the errors reported in the last run are repeating with other PRs.

blueorangutan · 2018-06-20T11:44:21Z

Trillian test result (tid-2775)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 18865 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2714-t2775-kvm-centos7.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_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 57 look OK, 10 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup	`Error`	160.74	test_privategw_acl.py
ContextSuite context=TestDeployVM>:setup	`Error`	0.00	test_vm_life_cycle.py
test_01_secured_vm_migration	`Error`	1.14	test_vm_life_cycle.py
test_02_not_secured_vm_migration	`Error`	1.13	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	1.11	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	1.11	test_vm_life_cycle.py
ContextSuite context=TestVMLifeCycle>:setup	`Error`	4.25	test_vm_life_cycle.py
ContextSuite context=TestVmSnapshot>:setup	`Error`	2.02	test_vm_snapshots.py
ContextSuite context=TestCreateVolume>:setup	`Error`	0.00	test_volumes.py
ContextSuite context=TestVolumes>:setup	`Error`	0.00	test_volumes.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Error`	8.05	test_vpc_redundant.py
test_02_redundant_VPC_default_routes	`Error`	8.06	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Error`	7.05	test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics	`Error`	8.06	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	8.08	test_vpc_redundant.py
test_01_VPC_nics_after_destroy	`Error`	5.99	test_vpc_router_nics.py
test_02_VPC_default_routes	`Error`	6.03	test_vpc_router_nics.py
test_01_redundant_vpc_site2site_vpn	`Failure`	6.42	test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options	`Failure`	5.35	test_vpc_vpn.py
test_01_vpc_remote_access_vpn	`Failure`	4.23	test_vpc_vpn.py
test_01_vpc_site2site_vpn	`Failure`	5.37	test_vpc_vpn.py
test_01_cancel_host_maintenace_with_no_migration_jobs	`Failure`	0.12	test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	3.46	test_host_maintenance.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	4.58	test_hostha_kvm.py

DaanHoogland · 2018-06-20T15:50:15Z

@blueorangutan package

blueorangutan · 2018-06-20T15:50:42Z

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

blueorangutan · 2018-06-20T16:27:48Z

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

yadvr · 2018-06-20T16:30:10Z

@blueorangutan test

blueorangutan · 2018-06-20T16:30:43Z

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

blueorangutan · 2018-06-21T01:50:06Z

Trillian test result (tid-2778)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31826 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2714-t2778-kvm-centos7.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_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 64 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_02_vpc_privategw_static_routes	`Failure`	189.39	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	176.22	test_privategw_acl.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	4.66	test_hostha_kvm.py

yadvr · 2018-06-21T05:33:07Z

Test LGTM, failing tests are due to known issues or intermittent in nature, but not related to KVM agent/libvirt resource class changes.

Throw specific NPE child when command is known not to be known. Add unit tests.

Daan Hoogland added 2 commits June 18, 2018 14:01

throw specific NPE child when command is known not to be known

686da6f

only catch what needs catching, add doc

2a32fe4

DaanHoogland changed the title ~~sned unsupported answer only when applicable~~ send unsupported answer only when applicable Jun 19, 2018

rafaelweingartner reviewed Jun 19, 2018

View reviewed changes

rafaelweingartner approved these changes Jun 19, 2018

View reviewed changes

import

14ffb98

DaanHoogland added this to the 4.11.1.0 milestone Jun 19, 2018

unit tests for known - and UnknownAnswer

0789ae1

rafaelweingartner reviewed Jun 19, 2018

View reviewed changes

GabrielBrascher approved these changes Jun 19, 2018

View reviewed changes

PaulAngus added component:kvm component:packaging type:bug status:work-in-progress and removed component:packaging labels Jun 19, 2018

initCause

8040754

PaulAngus added status:testing and removed status:work-in-progress labels Jun 19, 2018

yadvr approved these changes Jun 20, 2018

View reviewed changes

borisstoyanov approved these changes Jun 20, 2018

View reviewed changes

yadvr merged commit f02e402 into apache:4.11 Jun 21, 2018

ernjvr pushed a commit to shapeblue/cloudstack that referenced this pull request Jul 3, 2018

kvm: send unsupported answer only when applicable (apache#2714)

fc48d74

Throw specific NPE child when command is known not to be known. Add unit tests.

Uh oh!

Conversation

DaanHoogland commented Jun 19, 2018

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Jun 19, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jun 19, 2018

Uh oh!

blueorangutan commented Jun 19, 2018

Uh oh!

blueorangutan commented Jun 19, 2018

Uh oh!

PaulAngus commented Jun 19, 2018

Uh oh!

blueorangutan commented Jun 19, 2018

Uh oh!

blueorangutan commented Jun 20, 2018

Uh oh!

yadvr commented Jun 20, 2018

Uh oh!

blueorangutan commented Jun 20, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

PaulAngus commented Jun 20, 2018

Uh oh!

borisstoyanov commented Jun 20, 2018

Uh oh!

blueorangutan commented Jun 20, 2018

Uh oh!

DaanHoogland commented Jun 20, 2018

Uh oh!

blueorangutan commented Jun 20, 2018

Uh oh!

blueorangutan commented Jun 20, 2018

Uh oh!

yadvr commented Jun 20, 2018

Uh oh!

blueorangutan commented Jun 20, 2018

Uh oh!

blueorangutan commented Jun 21, 2018

Uh oh!

yadvr commented Jun 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

rafaelweingartner Jun 19, 2018 •

edited

Loading