consider capacity of transferred commitments correctly

[wagnerd3]

Previously, we tried to fit the full commitment into the capacity when creating a new commitment or confirming commitments after transfer.
This was wrong/ very pessimistic, because when transferring commitments, we actually free up capacity.
In case of ConfirmPendingCommitments we keep the stats over multiple iterations, so we have to modify them according to the transfers.
In case of /commitments/new, we fortunately only load the stats once afterwards, so we can skip modifying them manually.

[majewsky]

Maybe I'm misreading this, but from what I can see: During commitment acceptance, we only present the commitment that gets confirmed, not the ones that get consumed. So when a commitment for amount 10 in project 1 gets confirmed while consuming one for amount 5 in project 2 and one for amount 3 in project 3, the CommitmentChangeRequest that gets evaluated only has the commitment (and increase in TotalConfirmed) in project 1, not the commitments (and decrease in TotalConfirmed) in projects 2 and 3. If this is so, then that ain't right. We should put everything into one CommitmentChangeRequest and notify that to LIQUID for either complete acceptance or complete rejection.

[majewsky]

On a related note, if LIQUID (or the DB) rejects the commitment confirmation here, the consumption must be rolled back. (This would be solved by accepting all of them at once and only committing the change after the CCR was accepted.)

This is a different concern from the one above; previously, it was not a big deal that consumption was not reversible, since it only happened when the CCR was already requested by LIQUID/DB.

[wagnerd3]

You are right with the comment above. I was thinking about it solely from the local standpoint and saw the CCRs as the payload for the audit events. For the liquid, it really does not make sense to check the transfers and confirmations separately.

EDIT: recording some questions here, that I stumbled across during change of the implementation:

• When we include all affected commitments in 1 CCR, I assume we want to have the CCRs raw if multiple consume-split events occur in a row while the mails should still be collated?
  • E.g. project1 post c1, project2 consumes part of c1 in c2, c3 is created, project3 consumes part of c3 in c4, c5 is created etc.
  • My assumption is that we want: 1 CCR for c2 being confirmed with all consumptions, 1 CCR for c4 being confirmed with all consumptions, etc.
  • While the mails for project1 should only have the last leftover, not all steps in between
• Can we possibly get rid of liquid.CommitmentHandlingNeedsProjectMetadata? I get, that the projectMetadata are a huge overhead for the ServiceUsageRequest and ServiceQuotaRequest, but for the CommitmentChangeRequest the payload is quite large anyways, so the relation of the about 150 chars to the rest is not as drastic. The back and forth for including and excluding the projectMetadata from the requests for internal/ external use is just a hassle. I now changed it so that the projectMetadata are filled always internally and we just exclude them before sending to the liquid, which streamlines everything a bit.
  • If we cannot get rid of the flag, what would be a good way to enforce that we always fill projectMetadata of that struct when using it internally? AFAIK the only feasible way would be to create a new struct, which holds the liquid.CommitmentChangeRequest and the projectMetadata separately - there is no other way to enforce this at design-time, right? I know that we could create a factory-func for runtime which panics, when they are empty, but this is kind of ugly because it relies on tests to surface the problem...

[github-actions]

Merging this branch changes the coverage (1 decrease, 2 increase)

Impacted Packages	Coverage Δ	🤖
github.com/sapcc/limes/internal/api	77.96% (-0.10%)	👎
github.com/sapcc/limes/internal/audit	95.00% (+0.13%)	👍
github.com/sapcc/limes/internal/collector	82.40% (ø)
github.com/sapcc/limes/internal/datamodel	87.05% (+1.14%)	👍

---

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/sapcc/limes/internal/api/commitment.go	79.67% (-0.17%)	6198 (-24)	4938 (-30)	1260 (+6)	👎
github.com/sapcc/limes/internal/audit/audit.go	95.00% (+0.13%)	200 (+5)	190 (+5)	10	👍
github.com/sapcc/limes/internal/datamodel/commitment.go	79.10% (-0.61%)	402 (-12)	318 (-12)	84	👎
github.com/sapcc/limes/internal/datamodel/confirm_project_commitments.go	87.60% (+2.17%)	726 (-180)	636 (-138)	90 (-42)	👍
github.com/sapcc/limes/internal/datamodel/consume_transferable_commitments.go	91.80% (+2.55%)	1098 (+372)	1008 (+360)	90 (+12)	👍

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

• github.com/sapcc/limes/internal/api/commitment_test.go
• github.com/sapcc/limes/internal/collector/capacity_scrape_test.go

[wagnerd3]

The changes are incorporated in a separate commit now.
@majewsky please re-check.
@VoigtS We discussed a bit during the last days, so maybe you can do a review, too.