Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add depsolving and package search to cloudapi #4390

Closed
wants to merge 9 commits into from

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Sep 27, 2024

This pull request includes:

  • adequate testing for the new functionality or fixed issue
  • adequate documentation informing people about the change such as
    • submit a PR for the READMEs listed here
    • submit a PR for the osbuild.org website repository if this PR changed any behavior not covered by the automatically updated READMEs

@bcl bcl mentioned this pull request Sep 27, 2024
4 tasks
@bcl
Copy link
Contributor Author

bcl commented Sep 27, 2024

Note, this builds on the depsolving PR so I've closed it in favor of this one. Last 2 commits are package search and the previous ones lay the groundwork and implement depsolve.

@bcl bcl changed the title Main cloudapi packages Add depsolving and package search to cloudapi Sep 27, 2024
@bcl bcl force-pushed the main-cloudapi-packages branch 2 times, most recently from ef7f046 to 80d2024 Compare October 15, 2024 21:14
Copy link

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 15, 2024
@bcl bcl removed the Stale label Nov 15, 2024
@bcl bcl force-pushed the main-cloudapi-packages branch from 80d2024 to 2a20e5c Compare November 15, 2024 18:18
@bcl bcl requested a review from thozza November 26, 2024 23:29
@bcl bcl force-pushed the main-cloudapi-packages branch from 2a20e5c to e7c1d68 Compare November 26, 2024 23:36
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this PR delivers on its promises, I don't like the approach from an architectural point of view.

The server handles the depsolving and package search functionality directly, which is wrong. We already have a DepsolveJob type that the worker handles. So, the correct approach should be for the server to enqueue the DepsolveJob and use its result.

The same applies to package search—a new worker Job type should be added, and the search should occur on the worker.

Comment on lines +1151 to +1164
// If there is an architecture in the blueprint it overrides the request's arch
if len(bp.Arch) > 0 {
reqArch = bp.Arch
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just overriding the value does not seem like the best solution. We should ensure that the values in the image request and BP match if both are set or produce an error if they don't (as it is an invalid request).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the value in the blueprint should override the other (or maybe the request should override the optional arch in the bp, but they are not expected to match). This is how it works in the weldr API, if arch is set in a blueprint it overrides the default host arch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client should ensure a valid API call. This would mean extracting the architecture from the BP and making the correct compose request. The question is whether the architecture property should be added to the BP structure. Specifically, the API is already structured so that the requests are for specific environments and architecture, while BP is agnostic of these properties.

The Cloud API has no notion of the host arch, and everything must be specified in the request.

Comment on lines +855 to +867
request := &ComposeRequest{
Distribution: "fedora-40",
ImageRequest: &ImageRequest{
Architecture: "x86_64",
ImageType: ImageTypesAws,
UploadOptions: &uo,
Repositories: []Repository{},
},
Blueprint: &Blueprint{
Name: "arch-test",
Architecture: common.ToPtr("aarch64"),
},
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, this compose request should not be treated as valid. The architecture values don't match.

/depsolve/blueprint:
post:
operationId: postDepsolveBlueprint
summary: Depsolve one or more blueprints
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint depsolves only a single BP.

// slightly different from the Cloudapi's Customizations section
// This starts with a new empty blueprint.Customization object
// If there are no customizations, it returns nil
func (request *ComposeRequest) GetCustomizationsFromBlueprintRequest() (*blueprint.Customizations, error) {
if request.Blueprint.Customizations == nil {
func (rbp *Blueprint) GetCustomizationsFromBlueprintRequest() (*blueprint.Customizations, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name should be GetCustomizationsFromBlueprint, judging from the comment above it.

@bcl
Copy link
Contributor Author

bcl commented Dec 2, 2024

So, the correct approach should be for the server to enqueue the DepsolveJob and use its result.

What's the benefit of doing that? We already have the solver available in the server.

@thozza
Copy link
Member

thozza commented Dec 3, 2024

So, the correct approach should be for the server to enqueue the DepsolveJob and use its result.

What's the benefit of doing that? We already have the solver available in the server.

I can think of these:

  • To maintain reasonable architecture of Cloud API and not taint it with how things used to be done in Weldr API (which made a lot of assumptions of everything running on the same host).
  • To enable the SaaS version, eventually take advantage of any functionality you'll add (e.g., the package search).
  • The fact that the solver and the worker are on the same host as the API server in the on-prem scenario is an implementation detail in this case.

@bcl bcl marked this pull request as draft December 17, 2024 16:46
bcl added 9 commits January 10, 2025 16:21
If included it overrides the architecture in the compose image request.

Related: RHEL-60125
This will allow depsolving blueprints and returning package metadata for
the dependencies.

Related: RHEL-60125
This function only depends on the Blueprint (cloudapi request type, not
the internal/blueprint) so move it to a function on that so that it can
be reused by other users of the cloudapi Blueprint.

Related: RHEL-60125
Related: RHEL-60125
In order to reuse PackageMetadata with DepsolveResponse and not include
unused fields this changes the sigmd5 entry to an optional field. This
doesn't effect the use of PackageMetadata in the Compose response since
it is always set, and it allows it to be omitted in the response for
depsolving.

Also adds a basic test for stagesToPackageMetadata

Related: RHEL-60125
This also adds an actual repository json file for the test-disro.
Without this the repo.ListDistros() function doesn't return any actual
distros.

Related: RHEL-60125
This converts the request's blueprint to an internal/blueprint,
optionally selects the blueprint's distro and arch to override the
host's and depsolves the blueprint.

Also adds mock dnfjson depsolving to the test framework. And tests for
the new behavior.

Resolves: RHEL-60125
This will be used to retrieve information about specific packages, or
packages matching a search glob pattern.

Related: RHEL-60136
This takes the same input as depsolving (a blueprint and an optional
list of repositories), searches the package metadata and returns the
info about the matching packages in a structure that is similar to that
returned by the weldrapi projects API.

If the distro and/or arch are set in the blueprint they override the
host's distro and arch.

Resolves: RHEL-60136
@bcl bcl force-pushed the main-cloudapi-packages branch from e7c1d68 to ea59f30 Compare January 11, 2025 00:25
Copy link
Contributor

@kingsleyzissou kingsleyzissou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to split the package search out into another PR?

Merely for the fact that package search is more pressing than depsolve for the frontend

@bcl bcl mentioned this pull request Jan 18, 2025
4 tasks
@bcl
Copy link
Contributor Author

bcl commented Jan 18, 2025

Replaced by #4564

@bcl bcl closed this Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants