Skip to content

Conversation

@KlausDornsbach
Copy link
Contributor

@KlausDornsbach KlausDornsbach commented Feb 29, 2024

Description

With the new version of Log4j there is some space for improving log readability using the new features. This PR refactors the logs from the cloud-api module.

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)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

I have done some operations on cloudstack to induce the generation of the logs
No problems or changes to the log output were found

@codecov
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

❌ Patch coverage is 17.14286% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.50%. Comparing base (f417c6b) to head (7eebf2e).
⚠️ Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
...in/java/com/cloud/agent/api/storage/OVFHelper.java 16.66% 15 Missing ⚠️
...k/api/command/user/template/CreateTemplateCmd.java 0.00% 4 Missing ⚠️
...i/command/admin/vm/ImportUnmanagedInstanceCmd.java 0.00% 3 Missing ⚠️
...in/java/com/cloud/agent/api/storage/OVFParser.java 0.00% 2 Missing ⚠️
...and/user/firewall/CreateEgressFirewallRuleCmd.java 0.00% 2 Missing ⚠️
...loadbalancer/ListLoadBalancerRuleInstancesCmd.java 0.00% 2 Missing ⚠️
...he/cloudstack/api/command/user/vm/DeployVMCmd.java 0.00% 2 Missing ⚠️
...dstack/api/command/user/vm/ResetVMPasswordCmd.java 0.00% 2 Missing ⚠️
...java/org/apache/cloudstack/context/LogContext.java 0.00% 2 Missing ⚠️
.../api/command/admin/autoscale/DeleteCounterCmd.java 0.00% 1 Missing ⚠️
... and 23 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8728      +/-   ##
============================================
+ Coverage     17.46%   17.50%   +0.03%     
- Complexity    15516    15568      +52     
============================================
  Files          5913     5914       +1     
  Lines        529385   529623     +238     
  Branches      64679    64683       +4     
============================================
+ Hits          92448    92695     +247     
+ Misses       426518   426481      -37     
- Partials      10419    10447      +28     
Flag Coverage Δ
uitests 3.58% <ø> (-0.01%) ⬇️
unittests 18.56% <17.14%> (+0.04%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-authored-by: Vishesh <vishesh92@gmail.com>
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm, I think we need to implement some policies on when to log stack traces though. cc @JoaoJandre

Copy link
Contributor

@JoaoJandre JoaoJandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLGTM

@JoaoJandre
Copy link
Contributor

clgtm, I think we need to implement some policies on when to log stack traces though. cc @JoaoJandre

I think that some guidelines would be good, sadly the discussion regarding this never got traction on the dev mailing list.

@DaanHoogland
Copy link
Contributor

clgtm, I think we need to implement some policies on when to log stack traces though. cc @JoaoJandre

I think that some guidelines would be good, sadly the discussion regarding this never got traction on the dev mailing list.

maybe this will work better : #8746

@github-actions
Copy link

github-actions bot commented Mar 8, 2024

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

@github-actions
Copy link

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

@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

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland 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.

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10219

@GutoVeronezi GutoVeronezi self-assigned this Dec 13, 2025
@GutoVeronezi GutoVeronezi marked this pull request as draft December 13, 2025 14:42
@GutoVeronezi GutoVeronezi marked this pull request as ready for review December 15, 2025 03:00
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm generally, but foun d one possible problem

Co-authored-by: dahn <daan.hoogland@gmail.com>
@GutoVeronezi
Copy link
Contributor

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

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 16064

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland 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.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16071

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

[SF] Trillian Build Failed (tid-15014)

@blueorangutan
Copy link

[SF] Trillian test result (tid-15016)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 54993 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8728-t15016-kvm-ol8.zip
Smoke tests completed. 149 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_03_deploy_and_scale_kubernetes_cluster Failure 28.76 test_kubernetes_clusters.py

@DaanHoogland
Copy link
Contributor

@GutoVeronezi , are you satisfied like this or do you want more testing? test_03_deploy_and_scale_kubernetes_cluster needs attention but seems unrelated.

@GutoVeronezi
Copy link
Contributor

@GutoVeronezi , are you satisfied like this or do you want more testing? test_03_deploy_and_scale_kubernetes_cluster needs attention but seems unrelated.

It is fine for me.

@JoaoJandre, could you review it again as well?

@JoaoJandre
Copy link
Contributor

Left a minor nit

…mportUnmanagedInstanceCmd.java

Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com>
Copy link
Member

@vishesh92 vishesh92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@vishesh92 vishesh92 merged commit 13f805f into apache:main Dec 19, 2025
27 of 28 checks passed
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.

6 participants