CLOUDSTACK-8855 Improve Error Message for Host Alert State and reconnect host API. by rafaelweingartner · Pull Request #2387 · apache/cloudstack

rafaelweingartner · 2018-01-05T11:10:39Z

@DaanHoogland and @marcaurele I opened a new PR because it gets easier for me to fix problems and conflicts that may appear.

This closes#2380
This also closes#837

yadvr · 2018-01-05T11:26:28Z

@blueorangutan package

blueorangutan · 2018-01-05T11:27:32Z

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

blueorangutan · 2018-01-05T12:05:15Z

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

yadvr

Can you also fix conflicts @rafaelweingartner ?

yadvr · 2018-01-05T11:33:03Z

+ HostResponse response = _responseGenerator.createHostResponse(result);
+ response.setResponseName(getCommandName());
+ this.setResponseObject(response);
+ }catch (InvalidParameterValueException e) {


Space between catch and closing brace?

yadvr · 2018-01-05T11:33:25Z

+ }catch (InvalidParameterValueException e) {
+ throw new ServerApiException(ApiErrorCode.PARAM_ERROR, e.getMessage());
+ }
+ catch (CloudRuntimeException e) {


Fix this to be one same line as closing brace }

yadvr · 2018-01-05T11:33:46Z

+ catch (CloudRuntimeException e) {
+ s_logger.warn("Exception: ", e);
+ throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, e.getMessage());
+ }catch (AgentUnavailableException e) {


Same as above^^

rafaelweingartner · 2018-01-07T21:29:29Z

@rhtyd thanks for spotting that!
Changes applied and conflicts solved.

rafaelweingartner · 2018-01-27T12:00:41Z

@DaanHoogland and @marcaurele these last two days I have been working to fix all conflicts and rebase my PRs against the new master. However, this one that we were working together I did not understand why one of the travis tests are failing. Could you guys lend me your fresh eyes here?

DaanHoogland · 2018-01-27T18:48:09Z

I restarted the travis run. I don't see any reason but there is some link; the test is a certificate error and the changes are involving reconnects , might be a travis error, though.

DaanHoogland · 2018-02-07T09:38:42Z

@bvbharatk care to review this?

rafaelweingartner · 2018-03-01T16:08:42Z

@DaanHoogland I fixed that test that was not working. I isolated it, and then everything started working just fine. I think it was a concurrency problem.

@marcaurele would you mind finish the review here?

rafaelweingartner · 2018-03-06T13:59:48Z

@borisstoyanov could you run tests here?

borisstoyanov · 2018-03-06T14:05:44Z

@blueorangutan package

blueorangutan · 2018-03-06T14:06:52Z

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

blueorangutan · 2018-03-06T15:31:33Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1760

borisstoyanov · 2018-03-06T19:33:12Z

@blueorangutan test

blueorangutan · 2018-03-06T19:33:55Z

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

blueorangutan · 2018-03-07T03:51:56Z

Trillian test result (tid-2327)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28075 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2387-t2327-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.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_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 63 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_extract_template	`Failure`	128.29	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
test_06_download_detached_volume	`Failure`	137.84	test_volumes.py
test_01_redundant_vpc_site2site_vpn	`Failure`	324.51	test_vpc_vpn.py

borisstoyanov

LGTM

rafaelweingartner · 2018-03-07T12:02:25Z

@borisroman are those errors/failures ok?

borisstoyanov · 2018-03-07T12:06:10Z

Let me run smoke tests again, but I think we've seen them quite frequently on random PRs.
@blueorangutan test

blueorangutan · 2018-03-07T12:06:55Z

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

rafaelweingartner · 2018-03-07T12:08:55Z

ok, thanks.
@DaanHoogland have you approaved this PR?

DaanHoogland · 2018-03-08T08:48:59Z

@rafaelweingartner i did i think but my approval is kind of stale. Let me find time to revisit it, please

rafaelweingartner · 2018-03-08T10:01:17Z

Sure, no problem.

rafaelweingartner · 2018-03-12T10:04:42Z

@DaanHoogland since your approval the code did not change. I only isolated a smoke test case that was causing problems.

DaanHoogland · 2018-03-13T16:43:02Z

@rafaelweingartner sorry for sickness and €dayjob preventing me to review again, can you resolve conflicts?

Remove declaration of throws CloudRuntimeException I also removed some unused variables and comments left behind This closesapache#837

rafaelweingartner · 2018-03-13T16:50:20Z

@DaanHoogland done.

DaanHoogland

some queries

DaanHoogland · 2018-03-13T18:57:54Z

- _agentMgr.reconnect(host.getId());
+ try {
+ _agentMgr.reconnect(host.getId());
+ } catch (Exception e ) {


This pokemon was caught with the next commit :)

DaanHoogland · 2018-03-13T19:05:09Z

 **/
 public class AgentManagerImpl extends ManagerBase implements AgentManager, HandlerFactory, Configurable {
 protected static final Logger s_logger = Logger.getLogger(AgentManagerImpl.class);
- protected static final Logger status_logger = Logger.getLogger(Status.class);


this seems a functional backwards incompatible change. Did you considder that?

Yes, I did. It is a change in the logging class that is used on the left side of the log message in the log file. People normally lock down their log parse in the message per se, but for sure if they are considering the class as well, this can break things.
I changed this for a simple reason. It is misleading. It will show as if the “Status” class is generating that message, which is not true. I see no reason for this type of code.
I can revert this change, but my position is that it does not make sense. It is not used consistently. The same type of message is being logged also with the other logger.

I agree it is a corner case and we can revert that if someone complains but it is an interface of the system, through the log configuration.
I am fine with it, just wanted to be sure you realised. Also so we have a placeholder to easily find where to fix if someone complains

DaanHoogland · 2018-03-13T19:09:58Z


 public class AlertManagerImpl extends ManagerBase implements AlertManager, Configurable {
 private static final Logger s_logger = Logger.getLogger(AlertManagerImpl.class.getName());
- private static final Logger s_alertsLogger = Logger.getLogger("org.apache.cloudstack.alerts");


another interface change! Please realise that this is incompatible!

I do not consider this as an interface, explanation is above.

rafaelweingartner · 2018-03-14T18:18:01Z

@DaanHoogland thank you very much!

We will finally push this PR forward.

blueorangutan · 2018-03-15T18:35:24Z

Trillian test result (tid-2373)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 19887 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2387-t2373-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 66 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_hostha_enable_ha_when_host_in_maintenance	`Error`	0.93	test_hostha_kvm.py

Refatoração nos _parsers_ do Usage Closesapache#1056, apache#2373, and apache#2387 See merge request scclouds/scclouds!966

rafaelweingartner mentioned this pull request Jan 5, 2018
CLOUDSTACK-8855 Improve Error Message for Host Alert State and reconnect host API. #2380
Closed

rafaelweingartner force-pushed the pr/837 branch from 0807df2 to 5de055eCompare January 5, 2018 11:25

rafaelweingartner force-pushed the pr/837 branch from 5de055e to 7f94c2eCompare January 5, 2018 11:54

rafaelweingartner force-pushed the pr/837 branch from 7f94c2e to 126307bCompare January 5, 2018 12:57

yadvr mentioned this pull request Jan 6, 2018
CLOUDSTACK-8855 Improve Error Message for Host Alert State and reconnect host API. #837
Closed

yadvr previously requested changes Jan 7, 2018
View reviewed changes

rafaelweingartner force-pushed the pr/837 branch from 126307b to b18c49eCompare January 7, 2018 21:28

rafaelweingartner force-pushed the pr/837 branch 5 times, most recently from 901cfc9 to 9cb40d8Compare January 16, 2018 00:50

rafaelweingartner force-pushed the pr/837 branch 5 times, most recently from 0f744cd to 00ae7eeCompare January 26, 2018 01:08

rafaelweingartner self-assigned this Jan 29, 2018

rafaelweingartner added type:enhancement component:management-server labels Jan 30, 2018

rafaelweingartner added this to the 4.12 milestone Jan 30, 2018

rafaelweingartner requested review from DaanHoogland and marcaurele February 7, 2018 01:22

rafaelweingartner force-pushed the pr/837 branch from 53ed450 to b34afb1Compare March 1, 2018 15:09

borisstoyanov approved these changes Mar 7, 2018
View reviewed changes

bvbharatand others added 3 commits March 13, 2018 13:46

CLOUDSTACK-8855 Improve Error Message for Host Alert State
ff09a77

[CLOUDSTACK-9846] create column to save the content of alert messages
cec82f6
Remove declaration of throws CloudRuntimeException I also removed some unused variables and comments left behind This closesapache#837

Isolate a problematic test "smoke/test_certauthority_root"
929c9d8

rafaelweingartner force-pushed the pr/837 branch from b34afb1 to 929c9d8Compare March 13, 2018 16:49

DaanHoogland reviewed Mar 13, 2018
View reviewed changes

DaanHoogland approved these changes Mar 14, 2018
View reviewed changes

rafaelweingartner merged commit 972b8b7 into apache:masterMar 14, 2018

yadvr mentioned this pull request May 3, 2018
Backports for 4.11 branch #2621
Merged
12 tasks

Uh oh!

Conversation

rafaelweingartner commented Jan 5, 2018

Uh oh!

yadvr commented Jan 5, 2018

Uh oh!

blueorangutan commented Jan 5, 2018

Uh oh!

blueorangutan commented Jan 5, 2018

Uh oh!

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

rafaelweingartner commented Jan 7, 2018

Uh oh!

rafaelweingartner commented Jan 27, 2018

Uh oh!

DaanHoogland commented Jan 27, 2018

Uh oh!

DaanHoogland commented Feb 7, 2018

Uh oh!

rafaelweingartner commented Mar 1, 2018

Uh oh!

rafaelweingartner commented Mar 6, 2018

Uh oh!

borisstoyanov commented Mar 6, 2018

Uh oh!

blueorangutan commented Mar 6, 2018

Uh oh!

blueorangutan commented Mar 6, 2018

Uh oh!

borisstoyanov commented Mar 6, 2018

Uh oh!

blueorangutan commented Mar 6, 2018

Uh oh!

blueorangutan commented Mar 7, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner commented Mar 7, 2018

Uh oh!

borisstoyanov commented Mar 7, 2018

Uh oh!

blueorangutan commented Mar 7, 2018

Uh oh!

rafaelweingartner commented Mar 7, 2018

Uh oh!

DaanHoogland commented Mar 8, 2018

Uh oh!

rafaelweingartner commented Mar 8, 2018

Uh oh!

rafaelweingartner commented Mar 12, 2018

Uh oh!

DaanHoogland commented Mar 13, 2018

Uh oh!

rafaelweingartner commented Mar 13, 2018

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!

DaanHooglandMar 14, 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

DaanHooglandMar 14, 2018•
edited
Loading