liquid, limes: add support for non-standard units#84
Merged
Conversation
Until now, only a fixed enumeration of units was supported in Limes. For example, the units "MiB", "GiB" and "TiB" were supported, but nothing inbetween those. Compute raised a demand to us to be able to have resources with more granular units in order to express RAM quota for flavor groups. Suppose that there is a flavor group where the smallest flavor is 128 GiB and all other flavors are an integer multiple of that, quota should only be given out and commitments only be made in 128 GiB increments. Since, within LIQUID and Limes, all amounts (quota, usage, capacity, commitments) are taken to be integer multiples of the resource's defined unit, it is therefore desirable to declare this resource with a unit of "128 GiB". I considered the option of retaining Unit with the basic definition of `type Unit string`, but found the idea of having to parse a Unit of e.g. "128 GiB" into a structured value for each operation to be undesirable. Therefore, Unit had to become a struct (even though that introduces its own set of pain points like having to manage the zero value). Because the parsing and serialization logic for Unit values ended up having significant functional overlap with the existing parsing and serialization for ValueWithUnit, I created a new base type Amount to avoid code duplication. Unit now holds just an instance of type Amount, and provides its own choices for parsing and serialization on top. ValueWithUnit retains its current structure because of backwards compatibility requirements, but defers most of its work into Amount. I had to move all those types (Amount, Unit, ValueWithUnit) into a single internal package because they need to access API within each other that I do not want to expose publicly. Specifically, ValueWithUnit needs to reach the Amount value hidden within type Unit. Amount is not an exported type and remains an implementation detail. Unit gained some new interface implementations to cover things that were previously handled by it being a plain string: - `encoding/json.Marshaler` for JSON serialization - `database/sql.Scanner` for SQL deserialization - `database/sql/driver.Valuer` for SQL serialization While moving things around, I also took the opportunity to remove several unused pieces from the public API: - `UnitUnspecified` has (de facto) been replaced by `Option[Unit]` - `FractionalValueError` and `IncompatibleUnitsError` are not explicitly used by any callers and have been replaced by `fmt.Errorf` **Testing done:** I have vendored this branch into both limes and limesctl, and got the tests to pass with only minimal changes: - Casts of the form `string(unit)` had to be replaced with `unit.String()`. - Where `Unit` was used as a struct field with `json:",omitempty"`, the linter reminded me to make those into `omitzero`.
majewsky
force-pushed
the
liquid-nonstandard-units
branch
from
836e546 to
b229cf4
Compare
wagnerd3
reviewed
Mar 9, 2026
| @@ -0,0 +1,190 @@ | |||
| // SPDX-FileCopyrightText: 2017-2026 SAP SE or an SAP affiliate company | |||
Contributor
There was a problem hiding this comment.
Just wondering, why this has a from-to copyright header? Wouldn't we have to update this periodically to the current $year? At least that's what imply from a from-to copyright statements.
Contributor
Author
There was a problem hiding this comment.
I moved code that already had one of these, and updated it because a significant portion was rewritten. I'm not 100% on what the rules are for stuff like this, and didn't want to get sidetracked looking up legal advice, so just followed the existing pattern.
wagnerd3
requested changes
Mar 9, 2026
Merging this branch changes the coverage (1 decrease, 3 increase)
Coverage by fileChanged files (no unit tests)
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
|
wagnerd3
approved these changes
Mar 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Until now, only a fixed enumeration of units was supported in Limes. For example, the units "MiB", "GiB" and "TiB" were supported, but nothing inbetween those.
Compute raised a demand to us to be able to have resources with more granular units in order to express RAM quota for flavor groups. Suppose that there is a flavor group where the smallest flavor is 128 GiB and all other flavors are an integer multiple of that, quota should only be given out and commitments only be made in 128 GiB increments. Since, within LIQUID and Limes, all amounts (quota, usage, capacity, commitments) are taken to be integer multiples of the resource's defined unit, it is therefore desirable to declare this resource with a unit of "128 GiB".
I considered the option of retaining Unit with the basic definition of
type Unit string, but found the idea of having to parse a Unit of e.g. "128 GiB" into a structured value for each operation to be undesirable. Therefore, Unit had to become a struct (even though that introduces its own set of pain points like having to manage the zero value).Because the parsing and serialization logic for Unit values ended up having significant functional overlap with the existing parsing and serialization for ValueWithUnit, I created a new base type Amount to avoid code duplication. Unit now holds just an instance of type Amount, and provides its own choices for parsing and serialization on top. ValueWithUnit retains its current structure because of backwards compatibility requirements, but defers most of its work into Amount.
I had to move all those types (Amount, Unit, ValueWithUnit) into a single internal package because they need to access API within each other that I do not want to expose publicly. Specifically, ValueWithUnit needs to reach the Amount value hidden within type Unit. Amount is not an exported type and remains an implementation detail.
Unit gained some new interface implementations to cover things that were previously handled by it being a plain string:
encoding/json.Marshalerfor JSON serializationdatabase/sql.Scannerfor SQL deserializationdatabase/sql/driver.Valuerfor SQL serializationWhile moving things around, I also took the opportunity to remove several unused pieces from the public API:
UnitandValueWithUnit(all uses of YAML for config in Limes have been replaced by JSON)UnitUnspecifiedhas (de facto) been replaced byOption[Unit]FractionalValueErrorandIncompatibleUnitsErrorare not explicitly used by any callers and have been replaced byfmt.ErrorfTesting done: I have vendored this branch into both limes and limesctl, and got the tests to pass with only minimal changes:
string(unit)had to be replaced withunit.String().Unitwas used as a struct field withjson:",omitempty", the linter reminded me to make those intoomitzero.