You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/02/18 14:52:52 UTC
[GitHub] [cloudstack] GutoVeronezi opened a new pull request #4707: Improve logs on HAManagerImpl
GutoVeronezi opened a new pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707
### Description
This PR intends to improve logging in the class `HAManagerImpl` to facilitate troubleshooting.
### 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)
- [x] Enhancement (improves an existing feature and functioanlity)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
### How Has This Been Tested?
It has been tested locally on a test lab.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#issuecomment-831536365
<b>Trillian test result (tid-595)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32761 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4707-t595-kvm-centos7.zip
Smoke tests completed. 88 look OK, 0 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r584515890
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -315,14 +321,10 @@ public Status getHostStatus(final Host host) {
final HAConfig haConfig = haConfigDao.findHAResource(host.getId(), HAResource.ResourceType.Host);
if (haConfig != null) {
if (haConfig.getState() == HAConfig.HAState.Fenced) {
- if (LOG.isDebugEnabled()){
- LOG.debug("HA: Agent is available/suspect/checking Up " + host.getId());
- }
+ LOG.debug(String.format("HA: Agent [%s] is available/suspect/checking Up.", host.getId()));
return Status.Down;
} else if (haConfig.getState() == HAConfig.HAState.Degraded || haConfig.getState() == HAConfig.HAState.Recovering || haConfig.getState() == HAConfig.HAState.Fencing) {
- if (LOG.isDebugEnabled()){
- LOG.debug("HA: Agent is disconnected " + host.getId());
- }
+ LOG.debug(String.format("HA: Agent [%s] is disconnected. State: %s, %s.", host.getId(), haConfig.getState(), haConfig.getState().getDescription()));
Review comment:
I still really don't like this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r619169728
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -298,14 +308,10 @@ public Boolean isVMAliveOnHost(final Host host) throws Investigator.UnknownVM {
final HAConfig haConfig = haConfigDao.findHAResource(host.getId(), HAResource.ResourceType.Host);
if (haConfig != null) {
if (haConfig.getState() == HAConfig.HAState.Fenced) {
- if (LOG.isDebugEnabled()){
- LOG.debug("HA: Host is fenced " + host.getId());
- }
+ LOG.debug(String.format("HA: Host [%s] is fenced.", host.getId()));
Review comment:
I totally agree that debug level has been highly abused in the project. lot's of production environments use debug level by default for that reason. This should not have an impact on this PR though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#issuecomment-831184835
Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 509
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r578499510
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -298,14 +308,10 @@ public Boolean isVMAliveOnHost(final Host host) throws Investigator.UnknownVM {
final HAConfig haConfig = haConfigDao.findHAResource(host.getId(), HAResource.ResourceType.Host);
if (haConfig != null) {
if (haConfig.getState() == HAConfig.HAState.Fenced) {
- if (LOG.isDebugEnabled()){
- LOG.debug("HA: Host is fenced " + host.getId());
- }
+ LOG.debug(String.format("HA: Host [%s] is fenced.", host.getId()));
Review comment:
please put those checks back in
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -156,19 +156,16 @@ public boolean transitionHAState(final HAConfig.Event event, final HAConfig haCo
if (result) {
final String message = String.format("Transitioned host HA state from:%s to:%s due to event:%s for the host id:%d",
currentHAState, nextState, event, haConfig.getResourceId());
- if (LOG.isTraceEnabled()) {
- LOG.trace(message);
- }
+ LOG.debug(message);
Review comment:
why do we need to up this level?
also why remove the isEnabled check? I'd rather move the message creation inside such a check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r582684759
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -156,19 +156,16 @@ public boolean transitionHAState(final HAConfig.Event event, final HAConfig haCo
if (result) {
final String message = String.format("Transitioned host HA state from:%s to:%s due to event:%s for the host id:%d",
currentHAState, nextState, event, haConfig.getResourceId());
- if (LOG.isTraceEnabled()) {
- LOG.trace(message);
- }
+ LOG.debug(message);
Review comment:
log4j 3 has my vote, big job though. this is a pattern that finds following and this is what i want to prevent, we need very elaborate Sting construction at times. I wouldn't hassle about info or above, but debug and trace, yes. (also aggree on readible code btw, but isEnabled is very explicit. And also agree on your use of `format()`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland merged pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r580853088
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -156,19 +156,16 @@ public boolean transitionHAState(final HAConfig.Event event, final HAConfig haCo
if (result) {
final String message = String.format("Transitioned host HA state from:%s to:%s due to event:%s for the host id:%d",
currentHAState, nextState, event, haConfig.getResourceId());
- if (LOG.isTraceEnabled()) {
- LOG.trace(message);
- }
+ LOG.debug(message);
Review comment:
@GutoVeronezi you are right they do the check and this is an often returning discussion; they do the check after parameter evaluation which in this case is just the passing of the string. This is why I'd like the evaluation/construction of the String inside the `isEnabled` guarded block as well. no need to perform an expensive string contruction if you're not going to use it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r617768243
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -298,14 +308,10 @@ public Boolean isVMAliveOnHost(final Host host) throws Investigator.UnknownVM {
final HAConfig haConfig = haConfigDao.findHAResource(host.getId(), HAResource.ResourceType.Host);
if (haConfig != null) {
if (haConfig.getState() == HAConfig.HAState.Fenced) {
- if (LOG.isDebugEnabled()){
- LOG.debug("HA: Host is fenced " + host.getId());
- }
+ LOG.debug(String.format("HA: Host [%s] is fenced.", host.getId()));
Review comment:
I was wondering if some of the HA messages should be logged as DEBUG or INFO levels. I think that there are some quite important information to raise for ADMINs regardless of they wanting a debug, info, or trace log level.
For example, when a host gets fenced.
What do you think @DaanHoogland @GutoVeronezi? Does that make sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r584515416
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -156,19 +156,16 @@ public boolean transitionHAState(final HAConfig.Event event, final HAConfig haCo
if (result) {
final String message = String.format("Transitioned host HA state from:%s to:%s due to event:%s for the host id:%d",
currentHAState, nextState, event, haConfig.getResourceId());
- if (LOG.isTraceEnabled()) {
- LOG.trace(message);
- }
+ LOG.debug(message);
Review comment:
Ah, that's why I asked you to move the string creation inside the if block as well.
```suggestion
if (LOG.isDebugEnabled()) {
final String message = String.format("Transitioned host HA state from:%s to:%s due to event:%s for the host id:%d",
currentHAState, nextState, event, haConfig.getResourceId());
LOG.debug(message);
}
```
I see now that the message is used later in the block. I don't like this pattern, but you are right.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r582794740
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -156,19 +156,16 @@ public boolean transitionHAState(final HAConfig.Event event, final HAConfig haCo
if (result) {
final String message = String.format("Transitioned host HA state from:%s to:%s due to event:%s for the host id:%d",
currentHAState, nextState, event, haConfig.getResourceId());
- if (LOG.isTraceEnabled()) {
- LOG.trace(message);
- }
+ LOG.debug(message);
Review comment:
Well, if the real problem is only the cost of building this string, then I would ask you to take another look at the preview code, because, in this case, the string is already built when it reach the `if` statement, I just took off the `if` and changed it to `debug`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r580226027
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -156,19 +156,16 @@ public boolean transitionHAState(final HAConfig.Event event, final HAConfig haCo
if (result) {
final String message = String.format("Transitioned host HA state from:%s to:%s due to event:%s for the host id:%d",
currentHAState, nextState, event, haConfig.getResourceId());
- if (LOG.isTraceEnabled()) {
- LOG.trace(message);
- }
+ LOG.debug(message);
Review comment:
@DaanHoogland About removing the `isEnabled`, if you take a look at the methods (`debug`, `trace`, or others) on [Logger](https://github.com/apache/log4j/blob/trunk/src/main/java/org/apache/log4j/Logger.java) and [Category](https://github.com/apache/log4j/blob/trunk/src/main/java/org/apache/log4j/Category.java), you will notice that they already do the same validation as the `isEnabled` do, the only thing that the `if` would avoid is that in some cases the string to be logged would not be created; In an async web app, like `ACS`, it is not relevant to safe this "cost" by adding more lines to the codebase.
About changing the log level to `debug`, in this context, these messages work better as `debug`; as well, it is quite unusual to call `trace` on `ACS`, so, it is interesting to keep a pattern and use `debug`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#discussion_r582384873
##########
File path: server/src/main/java/org/apache/cloudstack/ha/HAManagerImpl.java
##########
@@ -156,19 +156,16 @@ public boolean transitionHAState(final HAConfig.Event event, final HAConfig haCo
if (result) {
final String message = String.format("Transitioned host HA state from:%s to:%s due to event:%s for the host id:%d",
currentHAState, nextState, event, haConfig.getResourceId());
- if (LOG.isTraceEnabled()) {
- LOG.trace(message);
- }
+ LOG.debug(message);
Review comment:
@DaanHoogland I understand your concern, however it is not like we are running `ACS` on an ENIAC; the cost to build a string (with `java.lang`) is barely notable.
Besides that, it is better to read a code without 'useless' lines.
Furthermore, if we update `log4j` to `log4j 2` it would not be a problem. =)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4707: Improve logs on HAManagerImpl
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4707:
URL: https://github.com/apache/cloudstack/pull/4707#issuecomment-836354590
only logging, trusting the user's requirements
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org