Skip to content

Conversation

@JoaoJandre
Copy link
Contributor

@JoaoJandre JoaoJandre commented Jan 24, 2023

Description

This PR normalizes ACS's loggers and upgrades log4j 1.2 to log4j 2.19. Back in 2015, PR #714 tried to normalize the loggers but, because that normalization caused some issues with the VMware module, it ended up being reverted. This PR intends to follow that PR's footsteps and normalize all of the loggers in ACS. The objective of reducing unnecessary logger declarations, and reduce the area of impact to upgrade ACS's logger framework from log4j 1.2 to log4 2.19.

The log4j upgrade was done similarly to the proposed here #2992 (comment), and is more detailed ahead.

The logger normalization includes:

  • Classes that could have their loggers inherited from their ancestors had their own loggers deleted;
  • Most loggers didn't have to be static, thus most of them were normalized so that they wouldn't be;
  • loggers are protected now;
  • Static logger's names are now 'LOGGER';
  • Non-static logger's names are now 'logger';
  • New class DbUpgradeAbstractImpl created so that all Upgraders extend it and inherit its logger;
  • The following classes now have loggers (so that all their children can inherit their loggers):
    BaseElement, PipelineImpl, AbstractRect, AbstractSmartLifeCycle, ManagedContextRunnable, Command, RuleApplier, VmWork, ModelObjectBase, SnapshotStrategyBase, VifDriver.

To normalize all the loggers in ACS, the following steps were performed:

  1. The DbUpgradeAbstractImpl class was created;
  2. This script https://pastebin.pl/view/6ff71b26 was used to make all the Upgraders extend the DbUpgradeAbstractImpl class (I manually had to correct some of them afterward);
  3. I ran this script https://pastebin.pl/view/89931790 for all ACS java classes; the, I checked if classes can inherit the logger from their ancestor*; if they can, then remove their current logger instantiation. If they cannot then leave the instantiation, but make it non-static. And in some cases, rename the logger references to logger;
  4. I tried to build ACS many times and manually fixed the errors, most of them were because some loggers did indeed have to be static.

*The ancestor list I created came from a previous iteration of this process, where I tried to simply remove logger instantiation from any class that extended any other class, the list was created while manually fixing the errors.

For the log4j upgrade, the following steps were performed:

  1. The relevant pom.xml files were updated;
  2. This script https://pastebin.pl/view/0b6b77f9 was used to update all the logger imports;
  3. I tried to build ACS many times and manually fixed the errors.

If you need to fix conflicts because of this PR, you can try using this script to help you:

About the log4j upgrade:

  • The DOMConfigurator does not exist in log4j2, its functionality was moved to the Configurator class;
  • Both NDC and MDC were merged into the ThreadContext class;

Disclaimers:

  • The new log4j2 configuration file format is not backwards compatible with the old one, users must update their configuration files to match with the new format. I used the official apache documentation to upgrade the default configurations, available here.

  • If you call EncryptorCLI manually, you must also pass the log4j2 jar path (which should be in the same directory as cloud-utils).

The are still some points to address/discuss in order to make this PR ready:

  • Refactor the following classes so that they can be used in log4j2:SnmpTrapAppender, AlertsSyslogAppender, TestAppender, SnmpEnhancedPatternLayout;
  • Convert CglibThrowableRenderer functionality to log4j2;
  • The log4j 1.2 dependency could not be completely removed because juniper contrail has log4j as a dependency.
  • Test logging for management, agent, usage, SSVMs and CPVMs

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

I tested if both the console and the file appenders were logging correctly in the management server, agents, and system VMs.

All classes that could have their loggers inherited from their fathers had their own loggers deleted;
Most loggers didn't have to be static, so most of them were normalized so that they wouldn't be;
All loggers are protected now;
Static logger's name are now 'LOGGER';
Non-static logger's name are now 'logger';
New class DbUpgradeAbstractImpl created so that all Upgraders extend it and inherit its logger
@JoaoJandre
Copy link
Contributor Author

@DaanHoogland @GutoVeronezi could you please review?

@weizhouapache weizhouapache modified the milestones: 4.19.0.0, 4.18.1.0 Jan 24, 2023
@DaanHoogland
Copy link
Contributor

@JoaoJandre , I didn´t yet review 2000 files, but on first sight it looks good. I hope you have scripts to regenerate the changes, because this PR will be a conflict magnet.

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Attention: 271 lines in your changes are missing coverage. Please review.

Comparison is base (2bfa9de) 30.94% compared to head (3cfc93a) 30.77%.

Files Patch % Lines
agent/src/main/java/com/cloud/agent/Agent.java 0.00% 93 Missing ⚠️
...nt/resource/consoleproxy/ConsoleProxyResource.java 0.00% 44 Missing ⚠️
...in/java/com/cloud/agent/api/storage/OVFHelper.java 10.71% 24 Missing and 1 partial ⚠️
...gent/src/main/java/com/cloud/agent/AgentShell.java 9.09% 20 Missing ⚠️
...rc/main/java/com/cloud/agent/mockvm/MockVmMgr.java 0.00% 11 Missing ⚠️
...va/com/cloud/agent/dao/impl/PropertiesStorage.java 11.11% 8 Missing ⚠️
...i/command/admin/vm/ImportUnmanagedInstanceCmd.java 0.00% 6 Missing ⚠️
...api/command/admin/systemvm/MigrateSystemVMCmd.java 0.00% 4 Missing ⚠️
...k/api/command/admin/systemvm/ScaleSystemVMCmd.java 0.00% 4 Missing ⚠️
...min/network/DeleteManagementNetworkIpRangeCmd.java 0.00% 3 Missing ⚠️
... and 37 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7131      +/-   ##
============================================
- Coverage     30.94%   30.77%   -0.18%     
+ Complexity    34219    33070    -1149     
============================================
  Files          5353     5352       -1     
  Lines        375914   374419    -1495     
  Branches      54634    54609      -25     
============================================
- Hits         116338   115221    -1117     
+ Misses       244301   243889     -412     
- Partials      15275    15309      +34     
Flag Coverage Δ
simulator-marvin-tests 24.64% <3.36%> (-0.10%) ⬇️
uitests 4.38% <ø> (ø)
unit-tests 16.41% <7.40%> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Jan 25, 2023

SonarCloud Quality Gate failed. Quality Gate failed

Bug C 28 Bugs Vulnerability B 9 Vulnerabilities Security Hotspot E 6 Security Hotspots Code Smell C 11993 Code Smells

4.5% 4.5% Coverage 5.1% 5.1% Duplication

auch, I checked the security issues and marked them safe. @JoaoJandre can you go through the vulnerabilities and bugs, please. I think we must ignore the code smells for this PR. We can sample them but this is going to be a manyear of work to clear them all, even the more serious ones.

@github-actions
Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

1 similar comment
@github-actions
Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@DaanHoogland
Copy link
Contributor

shall we keep count @JoaoJandre ? first conflict. (/me joking, let's not!)
I suggest we merge this as soon after 4.18 is out as we can.

@JoaoJandre
Copy link
Contributor Author

shall we keep count @JoaoJandre ? first conflict. (/me joking, let's not!) I suggest we merge this as soon after 4.18 is out as we can.

I think it's best we don't keep :D. Sounds great, I'll try to be as fast as possible in completing this PR and addressing any reviews and conflicts.

SonarCloud Quality Gate failed. Quality Gate failed
Bug C 28 Bugs Vulnerability B 9 Vulnerabilities Security Hotspot E 6 Security Hotspots Code Smell C 11993 Code Smells
4.5% 4.5% Coverage 5.1% 5.1% Duplication

auch, I checked the security issues and marked them safe. @JoaoJandre can you go through the vulnerabilities and bugs, please. I think we must ignore the code smells for this PR. We can sample them but this is going to be a manyear of work to clear them all, even the more serious ones.

Sure, I'll go through them.

@JoaoJandre
Copy link
Contributor Author

SonarCloud Quality Gate failed. Quality Gate failed
Bug C 28 Bugs Vulnerability B 9 Vulnerabilities Security Hotspot E 6 Security Hotspots Code Smell C 11993 Code Smells
4.5% 4.5% Coverage 5.1% 5.1% Duplication

auch, I checked the security issues and marked them safe. @JoaoJandre can you go through the vulnerabilities and bugs, please. I think we must ignore the code smells for this PR. We can sample them but this is going to be a manyear of work to clear them all, even the more serious ones.

@DaanHoogland, as this PR touched a lot of lines, any "bugs" or "vulnerabilities" that already existed are now being reported by Sonarcloud. I fixed one of them that I deemed relevant to this PR; however, in order to avoid raising the complexity of this PR, I think it would be better to create another PR to fix the other bugs and vulnerabilities. What do you think?

…variable that was mistakenly renamed on the normalization commit
@DaanHoogland
Copy link
Contributor

SonarCloud Quality Gate failed. Quality Gate failed
Bug C 28 Bugs Vulnerability B 9 Vulnerabilities Security Hotspot E 6 Security Hotspots Code Smell C 11993 Code Smells
4.5% 4.5% Coverage 5.1% 5.1% Duplication

auch, I checked the security issues and marked them safe. @JoaoJandre can you go through the vulnerabilities and bugs, please. I think we must ignore the code smells for this PR. We can sample them but this is going to be a manyear of work to clear them all, even the more serious ones.

@DaanHoogland, as this PR touched a lot of lines, any "bugs" or "vulnerabilities" that already existed are now being reported by Sonarcloud. I fixed one of them that I deemed relevant to this PR; however, in order to avoid raising the complexity of this PR, I think it would be better to create another PR to fix the other bugs and vulnerabilities. What do you think?

SonarCloud Quality Gate failed. Quality Gate failed
Bug C 28 Bugs Vulnerability B 9 Vulnerabilities Security Hotspot E 6 Security Hotspots Code Smell C 11993 Code Smells
4.5% 4.5% Coverage 5.1% 5.1% Duplication

auch, I checked the security issues and marked them safe. @JoaoJandre can you go through the vulnerabilities and bugs, please. I think we must ignore the code smells for this PR. We can sample them but this is going to be a manyear of work to clear them all, even the more serious ones.

@DaanHoogland, as this PR touched a lot of lines, any "bugs" or "vulnerabilities" that already existed are now being reported by Sonarcloud. I fixed one of them that I deemed relevant to this PR; however, in order to avoid raising the complexity of this PR, I think it would be better to create another PR to fix the other bugs and vulnerabilities. What do you think?

Where it comes to code smells i would agree, but I would rather assess all bugs and vulnerabilities maybe even before merging this.

@JoaoJandre
Copy link
Contributor Author

I updated the SnpmTrapAppender to work with log4j2 and have already tested if it gets created, and if its append method is called; however I did not test the SNMP functionality as it was not changed.

I removed the TestAppender, as it was only being used in a single test in a single class, and have rewritten the test.

About the AlertSyslogAppender, I have spent some time analyzing it, and it just does not seem to work at all. It will only append if _syslogAppenders is not empty nor null, but as far as I know this attribute cannot be configured via XML, and it is not being set anywhere else in the code, aside from a test class. @DaanHoogland, do you know how to configure and use this appender? If indeed it has never worked properly, could we remove it entirely?

@github-actions
Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@JoaoJandre
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@GutoVeronezi
Copy link
Contributor

GutoVeronezi commented Feb 7, 2024

Seems that packaging got stuck on this one; running again:

@blueorangutan package

@blueorangutan
Copy link

@GutoVeronezi a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@DaanHoogland
Copy link
Contributor

Seems that packaging got stuck on this one; running again:

might have been just the github api @GutoVeronezi (but rebuilding works;)

@GutoVeronezi
Copy link
Contributor

Seems that packaging got stuck on this one; running again:

might have been just the github api @GutoVeronezi (but rebuilding works;)

BO seems a bit under the weather today. At first, I thought it could be a problem with the PR; however, I built the packages normally, the tests ran successfully, and other PR's packaging tasks are not being reported as well (vide #8605 (comment)). Any clue if the problem is only with reporting the results?

@weizhouapache
Copy link
Member

Seems that packaging got stuck on this one; running again:

might have been just the github api @GutoVeronezi (but rebuilding works;)

BO seems a bit under the weather today. At first, I thought it could be a problem with the PR; however, I built the packages normally, the tests ran successfully, and other PR's packaging tasks are not being reported as well (vide #8605 (comment)). Any clue if the problem is only with reporting the results?

@GutoVeronezi
packages are ready 6 hours ago. it seems we have reached the github limits again.

@blueorangutan test matrix

@blueorangutan
Copy link

@weizhouapache a [SL] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-9124)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42129 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7131-t9124-xenserver-71.zip
Smoke tests completed. 128 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_cancel_host_maintenace_with_no_migration_jobs Error 308.53 test_host_maintenance.py

@GutoVeronezi
Copy link
Contributor

@GutoVeronezi packages are ready 6 hours ago. it seems we have reached the github limits again.

I see, thanks @weizhouapache.

@blueorangutan
Copy link

[SF] Trillian test result (tid-9126)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46966 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7131-t9126-kvm-centos7.zip
Smoke tests completed. 129 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@blueorangutan
Copy link

[SF] Trillian test result (tid-9125)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server r8
Total time taken: 47402 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7131-t9125-vmware-67u3.zip
Smoke tests completed. 129 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@GutoVeronezi
Copy link
Contributor

Hello guys

I checked the test that failed in #7131 (comment), and it seems not related to the PR; therefore, as the last run passed successfully, I will merge this one. If necessary, further improvements or fixes will be addressed in new PRs. A note was already sent in the mailing list notifying the users and devs regarding the next steps; however, if you find someone struggling with the logs or anything that needs adjusting, just ping me or @JoaoJandre, so we can act as fast as possible.

@GutoVeronezi GutoVeronezi merged commit 49cecae into apache:main Feb 8, 2024
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Feb 20, 2024
* Normalize logs

All classes that could have their loggers inherited from their fathers had their own loggers deleted;
Most loggers didn't have to be static, so most of them were normalized so that they wouldn't be;
All loggers are protected now;
Static logger's name are now 'LOGGER';
Non-static logger's name are now 'logger';
New class DbUpgradeAbstractImpl created so that all Upgraders extend it and inherit its logger

* Upgrade log4j

* fix errors caused by the merge

* Refactor cglibThrowableRenderer functionality to log4j2 and upgrade the last configuration files

* fix sonarcloud bug

* Fix errors caused by merge, remove some unused loggers, and rename a variable that was mistakenly renamed on the normalization commit

* Readd snmpTrapAppender, remove TestAppender

* Regenerate changes

* regenerate changes

* refactor last custom appender

* fix systemvm configuration xml

* Regenerate changes

* Regenerate changes

* regenerate changes

* Regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* Fix utils pom

* fix some tests

* regenerate changes

* Fix jar being printed on exception

* fix logging in system VMs, fix commands not having log4j2 classpath.

* regenerate changes

* Fix some unwanted renomeations

* fix end of file

* regenerate changes

* regenerate changes

* fix merge error

* regenerate changes

* fix tests

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* readd reload4j to tungsten as juniper depends on it

* Regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* re-add reload4j dependency to network-contrail, as juniper depends on it

* regenerate changes

* regenerate changes

* regenerate changes

* fix typo

* regenerate changes

* regenerate changes

* Fix end of files

* regenerate changes

* add logj42 to cloud-utils-SHADED.jar

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* regenerate changes

* Regenerate changes

* Regenerate changes

* Regenerate changes

* regenerate changes

* Regenerate changes

* regenerate changes

* Regenerate changes

* Regenerate changes

* Regenerate changes

* regenerate changes

* Regenerate changes

* Regenerate changes

* fix some tests

* Regenerate changes

* Regenerate changes

* fix test

* Regenerate changes

* Regenerate changes
@JoaoJandre JoaoJandre mentioned this pull request Jul 3, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.