Add logs to the listLoadBalancerRuleInstances API

[joseflauzino]

Description

Currently, the listLoadBalancerRuleInstances API has no logs, which makes troubleshooting harder. This PR aims to add several logs with context on the processing performed by this API. This will allow cloud operators to better analyze possible errors or inconsistencies that may occur when using the API.

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 functionality)
• [ ] 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?

I performed different calls to the listLoadBalancerRuleInstances API and could notice that the processes were logged correctly.

[joseflauzino]

@blueorangutan package

[blueorangutan]

@joseflauzino a 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.

[blueorangutan]

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 3350

[acs-robot]

Found UI changes, kicking a new UI QA build @blueorangutan ui

[blueorangutan]

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

[blueorangutan]

UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6357 (SL-JID-1566)

[acs-robot]

Found UI changes, kicking a new UI QA build @blueorangutan ui

[blueorangutan]

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

[blueorangutan]

UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6357 (SL-JID-1567)

[sonarqubecloud[bot]]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication

[joseflauzino]

@blueorangutan package

[blueorangutan]

@joseflauzino a 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.

[blueorangutan]

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 3405

[github-actions[bot]]

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

[joseflauzino]

@blueorangutan package

[blueorangutan]

@joseflauzino a 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.

[joseflauzino]

Hi @DaanHoogland, sorry for the delay in responding.

Regarding your suggested code changes: In these particular code snippets, I have a preference for not using the isDebugEnabled() check.

The main reason is that this check can provide a significant performance improvement only when the message construction is very costly, which I don't think is the case. In these snippets, the messages are built by the concatenation of strings (no need for expensive calculations or processing).

Also, if the logger is enabled at the debug level, it incurs the cost of evaluating whether the logger is enabled or not twice: once in isDebugEnabled() and once in debug(). Since it is very common for ACS users to leave the logger enabled for the debug level (because of the useful messages for troubleshoting), I don't think it is worth doing this check (especially with simple strings).

[codecov[bot]]

Codecov Report

Merging #6357 (5f6d685) into main (adfaa73) will increase coverage by 0.00%. The diff coverage is 0.00%.

@@            Coverage Diff            @@
##               main    #6357   +/-   ##
=========================================
  Coverage     10.84%   10.84%           
- Complexity     7097     7099    +2     
=========================================
  Files          2485     2485           
  Lines        245392   245403   +11     
  Branches      38320    38321    +1     
=========================================
+ Hits          26606    26610    +4     
- Misses       215518   215524    +6     
- Partials       3268     3269    +1     

Impacted Files	Coverage Δ
...ain/java/com/cloud/network/dao/LoadBalancerVO.java	56.66% <0.00%> (-1.96%)	:arrow_down:
...loud/network/lb/LoadBalancingRulesManagerImpl.java	7.78% <0.00%> (-0.06%)	:arrow_down:
...dstack/network/contrail/model/ModelObjectBase.java	28.84% <0.00%> (+7.69%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[blueorangutan]

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 3988

[DaanHoogland]

@joseflauzino , I don´t think your argument holds, in the long run we should avoid debug statements in production environments. If you think your logging is valid in production you should give it a higher level. @blueorangutan test

[blueorangutan]

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

[blueorangutan]

Trillian Build Failed (tid-4695)

[blueorangutan]

Trillian test result (tid-4697) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 42409 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6357-t4697-kvm-centos7.zip Smoke tests completed. 100 look OK, 1 have errors Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_08_upgrade_kubernetes_ha_cluster	Failure	599.17	test_kubernetes_clusters.py

[DaanHoogland]

@joseflauzino , I don´t think your argument holds, in the long run we should avoid debug statements in production environments. If you think your logging is valid in production you should give it a higher level.

@joseflauzino please review your loging statements and make sure anything you want in present in production is at least at INFO level. You are right that this is not perfectly implemented this way, but that does not mean we shouldn´t improve on it.

[joseflauzino]

@DaanHoogland Most of the log messages added/improved in this PR are really useful for debugging purposes. There are only two exceptions, which are presented as warning and error. Considering the characteristics of all the messages involved, I think the log levels are appropriate.

[DaanHoogland]

ok @joseflauzino but where ever there is a string operation done in the parameter passing I would still like the if (log.isDebugEnabled()) guard around it. Other wise fine.

[joseflauzino]

@blueorangutan package

[blueorangutan]

@joseflauzino a 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.

[blueorangutan]

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4558

[sonarqubecloud[bot]]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication