datasize, blueprint: add new datasizes.Size type and use in blueprints

[mvo5]

This is split out from #1041 and also includes a cleanup from #951

The idea is to make simplify the filesystem customization decoding to only use primitive types (string and the new datasizes.Size type) which means simpler code (as the mountpoint to string conversion will be done automatically for us) and also that we can use meta.Undecoded() for the toml library.

The tradeoff is of couse that it's slightly ugly that we now need to cast MinSize to uint64, if that is considered too ugly I can close this again.

[schutzbot]

This PR changes the images API or behaviour causing integration failures with osbuild-composer. The next update of the images dependency in osbuild-composer will need work to adapt to these changes.

This is simply a notice. It will not block this PR from being merged.

[achilleas-k]

if that is considered too ugly I can close this again.

I don't think it's ugly at all. Maybe it could be ever so slightly cleaner if we have a size.Uint64()? Not a big difference though.

[achilleas-k]

I would argue that the original test is more correct, since it creates an expected value that is the same type as the one being tested (uint64) rather than converting the value being tested to match the type of the literal, but it doesn't really matter much. Unless we make a size.Uint64() method.

Not an issue at all though.

[mvo5]

I like the suggestion with the size.Uint64() and I agree that the original test was/is nicer - I updated the PR. Thanks for the excellent feedback.

[bcl]

I don't see the point in making it a custom type. It's a uint64 so that's the type it should have, and the toml parser should be doing the conversion to that, not a custom type that you then have to always call a helper function on.
If there was more to it than that then it would make sense, but in this limited set of features I think it just makes it harder to understand the code.

[mvo5]

I don't see the point in making it a custom type. It's a uint64 so that's the type it should have, and the toml parser should be doing the conversion to that, not a custom type that you then have to always call a helper function on. If there was more to it than that then it would make sense, but in this limited set of features I think it just makes it harder to understand the code.

Thanks for your review! Maybe it makes more sense now in this slightly broader context (after #1049) is merged (as the new type is used more and there is more custom parsing that can be avoided now).

There are certainly drawbacks, it increases complexity by moving things into a different package, adds a new type and the error message are less less nice (because encoding(json is not very good with that for custom unmarshalers).

To me the benefits are separation of concern, i.e. a central place that can decode json/toml (and later yaml I presume) numbers that can be str/int/str-with-unit. From a certain PoV the existing MinSize uint64 is a "lie" because while this is what ends up in the datastruct its not what we read from the on-disk representation. Using datasizes.Size here hints that something more is going on there. Another (small) advantage is that toml is not good with complex decoding, it will always mark the complex type as "undecoded" which lead to issues in the past osbuild/bootc-image-builder#656

But as everything it's about tradeoffs, if it's considered too ugly, it can be closed.

[ondrejbudai]

I like this approach, it centralizes the parsing logic in one place and since we seem to be dealing with datasizes quite a lot in blueprints, I think this is less error-prone in the future.

[thozza]

I'm a bit with @bcl on this PR. While I like the changes for (un)marshaling values from/to JSON/TOML, I find the use of datasize.SIze in disk module adding unnecessary complexity. I think that we should internally use primitive types if possible and hide all the complexity in custom (un)marshallers. The new data type does not add anything for any use case other than (un)marshaling.

Maybe the BP customizations structure could keep using the primitive type, and then its (un)mashalers could use a private struct that uses datasize.Size, and converts the value to uint64.

[mvo5]

Thanks for the feedback @thozza - I will stare long and hard at this and see if I can find a more agreeable way of doing this. Part of the issue I think is the "quantum" state nature of our blueprints, they are both the "resolved" (unmarshaled) structs and also contain the json/toml keywords and the custom unmarshaler. Which leads to the situation now where the consumer of the structs want a simple uint64 for minsize (as that is what the resolved result is and should be) but the implementation of the struct must know that it's not really a uint64 and not forget to add the custom unmarshaling into all the right places (and do it right which for nested structs is not trivial(IMHO)). Which then leads to bugs like: 3b3912b that we currently have where the "DiskPartition" structre needs a custom unmarshaler to deal with minsize as a string (I will push a fix using the existing pattern shortly). Anyway, going back to starring long and hard at this :)

[edit: fwiw, the fix with datasize.Size for the above issue is:

diff --git a/pkg/blueprint/disk_customizations.go b/pkg/blueprint/disk_customizations.go
index 09b92099f..76ea92e2f 100644
--- a/pkg/blueprint/disk_customizations.go
+++ b/pkg/blueprint/disk_customizations.go
 type DiskCustomization struct {
        // TODO: Add partition table type: gpt or dos
-       MinSize    uint64                   `json:"minsize,omitempty" toml:"minsize,omitempty"`
+       MinSize    datasizes.Size           `json:"minsize,omitempty" toml:"minsize,omitempty"`
        Partitions []PartitionCustomization `json:"partitions,omitempty" toml:"partitions,omitempty"`
 }

(and it got fixed "by-accident" by just introducing the new datatype)

the fix using the exiting pattern:

diff --git a/pkg/blueprint/disk_customizations.go b/pkg/blueprint/disk_customizations.go
index 09b92099f..53aa4e471 100644
--- a/pkg/blueprint/disk_customizations.go
+++ b/pkg/blueprint/disk_customizations.go
@@ -18,6 +18,27 @@ type DiskCustomization struct {
        Partitions []PartitionCustomization `json:"partitions,omitempty" toml:"partitions,omitempty"`
 }
 
+func (dc *DiskCustomization) UnmarshalJSON(data []byte) error {
+       var dcAnySize struct {
+               MinSize    any                      `json:"minsize,omitempty" toml:"minsize,omitempty"`
+               Partitions []PartitionCustomization `json:"partitions,omitempty" toml:"partitions,omitempty"`
+       }
+       if err := json.Unmarshal(data, &dcAnySize); err != nil {
+               return err
+       }
+
+       dc.Partitions = dcAnySize.Partitions
+
+       if dcAnySize.MinSize != nil {
+               size, err := decodeSize(dcAnySize.MinSize)
+               if err != nil {
+                       return err
+               }
+               dc.MinSize = size
+       }
+       return nil
+}
+
 // PartitionCustomization defines a single partition on a disk. The Type
 // defines the kind of "payload" for the partition: plain, lvm, or btrfs.
 //   - plain: the payload will be a filesystem on a partition (e.g. xfs, ext4).

[thozza]

@mvo5 FWIW, I like your approach with datasize.Size in general. I just don't like its universal use. I would limit its use only to (un)marshalers because, in my opinion, it adds unnecessary complexity everywhere else.

My suggestion is something along these lines:

diff --git a/pkg/blueprint/filesystem_customizations.go b/pkg/blueprint/filesystem_customizations.go
index fba50da52..dc00c8aad 100644
--- a/pkg/blueprint/filesystem_customizations.go
+++ b/pkg/blueprint/filesystem_customizations.go
@@ -9,11 +9,16 @@ import (
        "github.com/osbuild/images/pkg/pathpolicy"
 )
 
-type FilesystemCustomization struct {
+type filesystemCustomizationMarshaling struct {
        Mountpoint string         `json:"mountpoint,omitempty" toml:"mountpoint,omitempty"`
        MinSize    datasizes.Size `json:"minsize,omitempty" toml:"minsize,omitempty"`
 }
 
+type FilesystemCustomization struct {
+       Mountpoint string
+       MinSize    uint64
+}
+
 // CheckMountpointsPolicy checks if the mountpoints are allowed by the policy
 func CheckMountpointsPolicy(mountpoints []FilesystemCustomization, mountpointAllowList *pathpolicy.PathPolicies) error {
        var errs []error
@@ -35,14 +40,14 @@ func (fsc *FilesystemCustomization) UnmarshalJSON(data []byte) error {
        // if the custom unmarshal for minsize failed (as encoding/json
        // provides sadly no context), c.f.
        // https://github.com/golang/go/issues/58655
-       type filesystemCustomization FilesystemCustomization
-       var fc filesystemCustomization
+       var fc filesystemCustomizationMarshaling
        if err := json.Unmarshal(data, &fc); err != nil {
                if fc.Mountpoint != "" {
                        return fmt.Errorf("JSON unmarshal: error decoding minsize value for mountpoint %q: %w", fc.Mountpoint, err)
                }
                return err
        }
-       *fsc = FilesystemCustomization(fc)
+       fsc.Mountpoint = fc.Mountpoint
+       fsc.MinSize = fc.MinSize.Uint64()
        return nil
 }

We should ideally decouple the external API for (un)marshaling for any internally used and exported structures. Initially, the external API for (un)marshaling was supposed to be the blueprint package in osbuild-composer, and the one in images was supposed to be the internal representation. We then added tags for (un)marshaling to the blueprint structures in images for testing purposes, and eventually, it grew into another external API for (un)marshaling user data.

This is IMO an initially unintended outcome, and it creates a mess when we need to modify internally used structures and complicate them just to handle the external (un)marshaling API. We shouldn't need to do this.

This is similar to our (never finished) goal to not directly use osbuild stage options for image customizations and in OSCustomizations structure.

The bottom line is that IMHO, pkg/blueprints in images should not handle user-facing (un)marshaling of any kind for its exported structures. These should be decoupled so as not to taint the implementation.

[mvo5]

I moved this to draft and opened #1057 with the first part of your idea, if it is well received we can continue to convert the other DiskPartitioning structs to the same pattern - which will be a bit verbose but go does not make this easy it seems :(

[mvo5]

Closing in favor of #1057