Image Type interface redesign

[achilleas-k]

Background

https://issues.redhat.com/browse/COMPOSER-1944

Proposals

Both of the prototypes here are very similar. The common part is that both handle package selection inside the ImageType.Manifest() function. We need to do this because we dynamically select packages based on feature selection (either by the user or the image type) and this is evaluated in the pipeline generator code (see the OS pipeline for example).

The current manifest generation code requires running through parts of the pipeline generator code twice, once for package selection before depsolving and again a second time to create the manifest and then serialise it with the depsolved packages. This has caused a lot of confusion (and bugs).

Squashing these operations lets us create a Manifest only once and depsolve the packages before serialising it. It's functionally the same thing that we do now. The current approach was necessary when we didn't want to change the interface for ImageType, a limitation we don't have any more.

I opened PRs for each to make it easy to browse the changes.

1. Return manifest.Manifest and PackageSets from ImageType.Manifest()

This is the more straightforward change of the two. The ImageType.Manifest() function returns a manifest.Manifest object (instead of the already serialised distro.Manifest which is an alias for []byte) alongside the unresolved package sets. It puts the responsibility on the caller to depsolve the package sets and then call manifest.Serialize(packages) with the resolved packages.

This has a few side effects arising from import changes, since the distro package needs to import manifest instead of the other way around. On the other hand, it has a much smaller effect on the worker job implementations than the alternative.

See #3385 for a prototype of the changes and the ./cmd/new-imagetype/main.go for a quick PoC of what the calling code would look like.

2. Pass resolver functions to ImageType.Manifest()

The ImageType.Manifest() function returns a serialized manifest ([]byte) and does package selection and depsolving inside the distro package. The function needs access to a depsolver to call between the package selection and serialisation. It's essentially the same as the first approach, but requires the caller to define the resolver and pass it in to the function. I also considered having every resolver passed in to the function for consistency and included a container resolver in the PoC branch.

I personally prefer this but it's a bigger change. I like that it forces the caller of the ImageType.Manifest() function to know what the manifest generation requires (the resolvers), but doesn't require the caller to actually do the calling and combining of information. In our current flow (and the first approach), the caller gets a set of unresolved packages and needs to depsolve them before asking the ImageType (or Manifest) to serialise, which puts the responsibility on the caller to understand how to combine these two values it gets from the ImageType API (the manifest and the packages). In this approach, the resolver arguments force the caller to define functions that are (or should be) clear to understand.

See #3384 for a prototype of the changes and the ./cmd/new-imagetype/main.go for a quick PoC of what the calling code would look like.

Further

Certain things should also be part of the change that aren't reflected in the proposals.

Blueprint and ImageOptions

I believe neither of these should show up in the API. Instead, we should define a new structure for the options that are passed into the functions that generate the manifest. Both the blueprint and the options are user-facing objects and relate to how we define the external user interaction in the weldr API. In the cloud API, we don't have blueprints, yet we create one to accommodate this interface. We should instead define a new object that will always describe the user's options going forward, whether they are from a blueprint, command line options, or cloud API request.

Workers

The worker job implementations will be affected by these changes. The depsolve job and dependency order in particular. I think we can maintain backwards compatibility (newer composer versions with older workers) by keeping the old job implementations around for a while, but I don't know if the dependency order handling will be handled easily.

Out of scope

Any discussion about image types, image functions, the ImageKind, the Platform/Environment/Workload components is out of scope right now. The purpose is to define what manifest creation will look like for an external user of the new repository's API. This will affect how the weldr and Cloud APIs handle a compose job, but not how an image is defined in code.

[ondrejbudai]

At the first glance, I prefer the second variant, thus:

func Manifest(b *blueprint.Blueprint, options ImageOptions, repos []rpmmd.RepoConfig, seed int64, pkgResolver PackageResolver, contResolver ContainerResolver) (Manifest, []string, error)

type PackageResolver func(pkgSets map[string][]rpmmd.PackageSet) (map[string][]rpmmd.PackageSpec, error)

type ContainerResolver func(source, name string, tlsVerify *bool) (container.Spec, error)

However, there is an issue: Cloud API is currently resolving both packages and containers asynchronously and in parallel. However, PackageResolver and ContainerResolver functions are synchronous. Sure, it's absolutely possible to do the actual resolution asynchronously, but it's not possible to do it in parallel, because these functions are called sequentially. This will for sure affect the performance of the manifest generation process.

The solution might be to make the resolvers return channels:

type PackageResolver func(pkgSets map[string][]rpmmd.PackageSet) (chan map[string][]rpmmd.PackageSpec, error)

type ContainerResolver func(source, name string, tlsVerify *bool) (chan container.Spec, error)

(Of course, this has to be sprinkled with a bit of proper error handling.)

While this solution would certainly make the resolution process parallel again, I'm not entirely sure if it isn't making the code much more complicated.

This makes me wonder: Isn't the first approach actually better in the end? Sure, it's not error-proof by design, but the resulting code might be just simpler to follow and read.

[achilleas-k]

That's a great point that I only half-considered. The async-ness of the functions was something I considered we'd do, but I didn't think about how each would essentially block in the Manifest() function, both slowing it down and making them sequential.

I did have another idea in the meantime: we could do both approaches:

• Have the it.Manifest() function be the canonical one that returns a manifest.Manifest.
• Make the second one a helper that also resolves called it.ManifestResolved() which just wraps it.Manifest(), resolves stuff, then serialises.

This is making me prefer the first one for now and then we can add the second as an extension to the API down the line.

[ondrejbudai]

Another option is to pass in just one resolver function that would take all packages and containers to resolve in one call.

type PackageResolutionRequest struct {
    Sets map[string][]rpmmd.PackageSet
}
type PackageResolutionResult struct {
    Specs map[string][]rpmmd.PackageSpec
    Err error
}

type ContainerResolutionRequest struct {
    Source string
    Name string
    TLSVerify *bool
}
type ContainerResolutionResult struct {
    Spec container.Spec
    Err error
}

type Resolver func(pkgs []PackageResolutionRequest, containers []ContainerResolutionRequest) ([]PackageResolutionResult, []ContainerResolutionResult)

func Manifest(b *blueprint.Blueprint, options ImageOptions, repos []rpmmd.RepoConfig, seed int64, resolver Resolver) (Manifest, []string, error)

[achilleas-k]

I had a similar idea at some point: have a resolver interface (yeah, more interfaces...) and different implementations for each type (dependencies, ostree commits, containers, files/urls) and then pass a multi-resolver object into the Manifest() job. The job then submits resolve requests to the resolver and collects them all in the end.

I started sketching an idea in this comment of what that might look like with more concrete types instead of having interfaces everywhere, based on your suggestion, but I got a bit lost in the details so I'll have to play with it a bit more.

[achilleas-k]

Copying, rephrasing, and expanding on some comments from #3444 because I'll probably be referring to this discussion for documentation.

I'm trying to avoid making the manifest.Manifest an interface. It's a pretty simple type and there's little benefit to having it be an interface with a single implementation. The one benefit is that it would be easier to test, but I don't know if that's a good enough reason. That said, figuring out how these things can be tested helps us figure out how users who import the packages might interact with them.

One basic question is: do we want external users writing their own pipelines? I don't think users of the new image definition library should be concerned with osbuild stages, at least not as a requirement. We might end up making it possible to import the osbuild package and start putting stages together to write a pipeline, but that should be for experimentation or very advanced usage. Regular usage should consist of taking the existing pipelines and configuring them in different ways, with a clear understanding of what effect toggling each option has functionally (as opposed to how it changes the intermediate manifest).

My current strategy is to make as few things available publicly in the API as possible and open things up when the need arises. This means that initially, importing the new library will let users define their own image types but only as a combination of existing pipelines and options. I want to avoid having too many ways to make manifests that fail to build. Ensuring the image boots (or works as intended, in the case of non-bootable image types) is a whole other story.

Note that by "users" here I also mean osbuild-composer. Part of the problem here is separating what's defined where: If pipelines can be defined outside of the image definition library by implementing a couple of interfaces, we risk the temptation of defining image types in composer, which is the opposite of what we're working towards now.

[teg]

FWIW, I agree with your take. The idea should be that pipelines are defined in this library and exposing osbuild abstractions would be a layering violation.

We don't allow osbuild stages to be written in the image definitions, and by the same logic we should not allow pipelines to be defined in composer.

We should be very open to adding new pipelines to the library though, in the same way we don't really say no to stages in osbuild (as long as they conform to our development principles).

Erring on the side of exposing as little as possible at first also makes sense, as we discover what should be opened up and what development should happen where.