cloudstack icon indicating copy to clipboard operation
cloudstack copied to clipboard

Create API to list Quota credits

Open winterhazel opened this issue 1 year ago β€’ 8 comments

Description

When credits are added to an account, an entry that represents the operation is created in cloud_usage.quota_credits. Some information about these entries is currently returned alongside an account's balance through the quotaBalance API. However, this is not consistent with the API's purpose (viewing an account's balance), and it is not possible to view the credit addition history for a whole domain with a single API call.

In order to make quotaBalance more consistent with its purpose, the credit addition history will be removed from its response alongside some enhancements to the API in the future. To list the credit addition history, this PR adds the quotaCreditsList, which has the following parameters:

  • accountid: ID of the account for which the credit statement will be generated.
  • domainid: ID of the domain for which credit statement will be generated. Available only for administrators.
  • isrecursive: Whether to generate the credit statement for the provided domain and its children. Defaults to false.
  • startdate: Start date of the statement. If not provided, the first day of the current month will be considered as the start date.
  • enddate: End date of the statement. If not provided, the current date will be considered as the end date.

No parameters are flagged as required, but at least accountid or domainid must be provided.

Types of changes

  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [X] 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
  • [X] Minor

How Has This Been Tested?

In my environment, there were the following domains and accounts:

  • ROOT: admin (root admin)
  • ROOT/d1: d1 (domain admin), u1 (user)

First, in the root admin account, I added 100 credits to the three accounts; in the domain admin account, I added credits to d1 and u1. Then, I tested the behavior of the API for each account.

For the root admin account:

  • I listed admin's credits history and verified that the response had the expected results (accountid tested).
(admin) 🐒 > quota creditslist accountid=60b92e66-28b2-11ef-a04e-6ac09c2b3d6b 
{
  "count": 1,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:39+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    }
  ]
}
  • I listed d1's credits history and verified that the response had the expected results.
(admin) 🐒 > quota creditslist accountid=a6b8f3b0-0f4f-4762-86eb-df229fe602ed 
{
  "count": 2,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:53+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:06+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    }
  ]
}
  • I listed u1's credits history and verified that the response had the expected results.
(admin) 🐒 > quota creditslist accountid=2560f486-38f1-42bf-aa0f-c68abe7ad15e 
{
  "count": 2,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:58+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:10+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    }
  ]
}
  • I listed d1's credits specifying the interval so that only the first addition was shown (enddate tested).
(admin) 🐒 > quota creditslist accountid=a6b8f3b0-0f4f-4762-86eb-df229fe602ed enddate=2024-08-18T22:46:00+0000
{
  "count": 1,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:53+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    }
  ]
}
  • I listed d1's credits specifying the interval so that only the last addition was shown (startdate tested).
(admin) 🐒 > quota creditslist accountid=a6b8f3b0-0f4f-4762-86eb-df229fe602ed startdate=2024-08-18T22:46:00+0000
{
  "count": 1,
  "credit": [
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:06+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    }
  ]
}
  • I listed ROOT's credits and verified that the response had the expected results (domainid tested).
(admin) 🐒 > quota creditslist domainid=40815ad7-28b2-11ef-a04e-6ac09c2b3d6b 
{
  "count": 1,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:39+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    }
  ]
}
  • I listed ROOT's credits recursively and verified that the response had the expected results (isrecursive tested).
(admin) 🐒 > quota creditslist domainid=40815ad7-28b2-11ef-a04e-6ac09c2b3d6b isrecursive=true
{
  "count": 5,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:39+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:53+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:58+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:06+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    },
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:10+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    }
  ]
}

For the domain admin account:

  • I tried to list admin's credits history and verified that my permission was denied.
(d1) 🐒 > quota creditslist accountid=2
πŸ™ˆ Error: (HTTP 531, error code 4365) Caller does not have permission to operate with provided resource.
  • I listed d1's credits history and verified that the response had the expected results.
(d1) 🐒 > quota creditslist accountid=a6b8f3b0-0f4f-4762-86eb-df229fe602ed 
{
  "count": 2,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:53+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:06+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    }
  ]
}
  • I listed u1's credits history and verified that the response had the expected results.
(d1) 🐒 > quota creditslist accountid=2560f486-38f1-42bf-aa0f-c68abe7ad15e 
{
  "count": 2,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:58+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:10+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    }
  ]
}
  • I listed ROOT/d1's credits history and verified that the response had the expected results.
(d1) 🐒 > quota creditslist domainid=02da877c-615f-4a7d-a68d-bde6b6d5d01c 
{
  "count": 4,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:53+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:58+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:06+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    },
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:10+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    }
  ]
}

For the user account:

  • I tried to list admin's credits history and verified that my permission was denied.
(u1) 🐒 > quota creditslist accountid=2
πŸ™ˆ Error: (HTTP 531, error code 4365) Caller does not have permission to operate with provided resource.
  • I tried to list d1's credits history and verified that my permission was denied.
(u1) 🐒 > quota creditslist accountid=a6b8f3b0-0f4f-4762-86eb-df229fe602ed 
πŸ™ˆ Error: (HTTP 531, error code 4365) Caller does not have permission to operate with provided resource.
  • I listed u1's credits history and verified that the response had the expected results.
(u1) 🐒 > quota creditslist accountid=2560f486-38f1-42bf-aa0f-c68abe7ad15e
{
  "count": 2,
  "credit": [
    {
      "credit": 100,
      "creditedon": "2024-08-18T22:45:58+0000",
      "creditoruserid": "60ba931c-28b2-11ef-a04e-6ac09c2b3d6b",
      "creditorusername": "admin",
      "currency": "$"
    },
    {
      "credit": 200,
      "creditedon": "2024-08-18T22:46:10+0000",
      "creditoruserid": "5bd602d7-5d43-4d68-9619-d413d0458a2d",
      "creditorusername": "d1",
      "currency": "$"
    }
  ]
}
  • I tried to list the credits history for ROOT/d1 and verified that my permission was denied.
(u1) 🐒 > quota creditslist domainid=2
πŸ™ˆ Error: (HTTP 531, error code 4365) Regular users are not allowed to generate domain statements.

winterhazel avatar Aug 26 '24 11:08 winterhazel

@blueorangutan package

winterhazel avatar Aug 26 '24 11:08 winterhazel

@winterhazel 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 avatar Aug 26 '24 11:08 blueorangutan

Codecov Report

Attention: Patch coverage is 68.58974% with 49 lines in your changes missing coverage. Please review.

Project coverage is 16.08%. Comparing base (d1cf453) to head (542c4c5). Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
...ache/cloudstack/quota/dao/QuotaCreditsDaoImpl.java 0.00% 18 Missing :warning:
...he/cloudstack/api/command/QuotaCreditsListCmd.java 69.76% 13 Missing :warning:
.../main/java/com/cloud/domain/dao/DomainDaoImpl.java 0.00% 6 Missing :warning:
...udstack/api/response/QuotaResponseBuilderImpl.java 91.04% 2 Missing and 4 partials :warning:
...org/apache/cloudstack/quota/vo/QuotaCreditsVO.java 0.00% 3 Missing :warning:
.../cloudstack/api/response/QuotaBalanceResponse.java 0.00% 2 Missing :warning:
.../org/apache/cloudstack/quota/QuotaServiceImpl.java 0.00% 1 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9590      +/-   ##
============================================
+ Coverage     16.07%   16.08%   +0.01%     
- Complexity    12890    12914      +24     
============================================
  Files          5642     5643       +1     
  Lines        494087   494196     +109     
  Branches      59931    59940       +9     
============================================
+ Hits          79425    79514      +89     
- Misses       405844   405863      +19     
- Partials       8818     8819       +1     
Flag Coverage Ξ”
uitests 4.01% <ΓΈ> (ΓΈ)
unittests 16.93% <68.58%> (+0.01%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 26 '24 11:08 codecov[bot]

Packaging result [SF]: βœ”οΈ el8 βœ”οΈ el9 βœ”οΈ debian βœ”οΈ suse15. SL-JID 10810

blueorangutan avatar Aug 26 '24 12:08 blueorangutan

@blueorangutan test keepEnv

DaanHoogland avatar Aug 28 '24 08:08 DaanHoogland

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

blueorangutan avatar Aug 28 '24 08:08 blueorangutan

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

Test Result Time (s) Test File

blueorangutan avatar Aug 28 '24 22:08 blueorangutan

@winterhazel , I think this is good to go safe one concern; removing response data from an API response is a backwards incompatibility. I can not judge the gravity (and haven't tested this PR and am not a quota user so just take my concern a little seriously ;) but the principle should only be broken after careful consideration. Can you give some stronger arguments than consistency for removing it, please?

DaanHoogland avatar Aug 29 '24 07:08 DaanHoogland

Hey @DaanHoogland, my idea of removing the credits listing from quotaBalance was to make each API have a single responsibility and make credits-related code changes easier in the future (as we would not have to worry about checking and updating quotaBalance accordingly anymore when introducing these changes). I don't have a stronger argument for it πŸ˜…

Regarding the gravity of removing it: Quota users would be advised to use quotaCreditsList to list credit additions instead. However, scripts/automation tools/external software that use quotaBalance to get the credit addition history of accounts and were not updated before upgrading CloudStack would break. The Quota UI currently uses it to show a list containing the balance and credit addition entries at the same time, but this page will be reworked in the future to separate them, using quotaCreditsList for listing credit additions instead.

As it would definitely have an impact, I understand why we would want to keep this information in the response, so I'm okay with not removing it. The enhancements to quotaBalance will still be made without this change.

winterhazel avatar Aug 30 '24 02:08 winterhazel

ok, @winterhazel . I know you and your are the most heavy users of quota so I am not going to -1 this by any means, but my advice would be to leave it in and shout about removing it before you do. you might get work mitigating any regressions if you don't.

DaanHoogland avatar Aug 30 '24 08:08 DaanHoogland

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

github-actions[bot] avatar Dec 04 '24 07:12 github-actions[bot]

@blueorangutan package

winterhazel avatar Dec 05 '24 18:12 winterhazel

@winterhazel 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 avatar Dec 05 '24 18:12 blueorangutan

Packaging result [SF]: βœ”οΈ el8 βœ”οΈ el9 βœ”οΈ debian βœ”οΈ suse15. SL-JID 11731

blueorangutan avatar Dec 05 '24 19:12 blueorangutan

@blueorangutan package

DaanHoogland avatar Jan 07 '25 12:01 DaanHoogland

@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 avatar Jan 07 '25 12:01 blueorangutan

Packaging result [SF]: βœ”οΈ el8 βœ”οΈ el9 βœ”οΈ debian βœ”οΈ suse15. SL-JID 12004

blueorangutan avatar Jan 07 '25 13:01 blueorangutan

@blueorangutan test

DaanHoogland avatar Jan 07 '25 13:01 DaanHoogland

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

blueorangutan avatar Jan 07 '25 13:01 blueorangutan

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

Test Result Time (s) Test File
test_02_unsecure_vm_migration Error 450.11 test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration Error 380.41 test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration Error 359.40 test_vm_life_cycle.py

blueorangutan avatar Jan 08 '25 08:01 blueorangutan

applying insanity

DaanHoogland avatar Jan 08 '25 08:01 DaanHoogland

@blueorangutan test keepEnv

DaanHoogland avatar Jan 08 '25 08:01 DaanHoogland

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

blueorangutan avatar Jan 08 '25 08:01 blueorangutan

[SF] Trillian test result (tid-12058) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 52614 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9590-t12058-kvm-ol8.zip Smoke tests completed. 140 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_secure_vm_migration Error 134.27 test_vm_life_cycle.py
test_01_secure_vm_migration Error 134.27 test_vm_life_cycle.py

blueorangutan avatar Jan 08 '25 23:01 blueorangutan

test_vm_life_cycle has a more generic problem especially in the secure chapter. The errors are different both times and probably environmental by nature.

DaanHoogland avatar Jan 09 '25 08:01 DaanHoogland

Hey @DaanHoogland, my idea of removing the credits listing from quotaBalance was to make each API have a single responsibility and make credits-related code changes easier in the future (as we would not have to worry about checking and updating quotaBalance accordingly anymore when introducing these changes). I don't have a stronger argument for it πŸ˜…

Regarding the gravity of removing it: Quota users would be advised to use quotaCreditsList to list credit additions instead. However, scripts/automation tools/external software that use quotaBalance to get the credit addition history of accounts and were not updated before upgrading CloudStack would break. The Quota UI currently uses it to show a list containing the balance and credit addition entries at the same time, but this page will be reworked in the future to separate them, using quotaCreditsList for listing credit additions instead.

As it would definitely have an impact, I understand why we would want to keep this information in the response, so I'm okay with not removing it. The enhancements to quotaBalance will still be made without this change.

@winterhazel , clear, your call..!

DaanHoogland avatar Jan 15 '25 15:01 DaanHoogland

@winterhazel , clear, your call..!

This one should be good for merge as well πŸ™‚

winterhazel avatar Jan 15 '25 16:01 winterhazel