v1: add immutable distributions endpoint (HMS-9269)#1775
v1: add immutable distributions endpoint (HMS-9269)#1775ksiekl wants to merge 1 commit intoosbuild:mainfrom
Conversation
|
Why is it a separate endpoint from just |
My bad, I assumed this would be a new endpoint. It makes sense to merge it with the existing for sure. |
|
As a starting point, I think creating just RHEL 10.0 with very limited scope will be the best so we can start developing the UI early and add more combinations later. I now have better idea on how data will be pushed in ECR, we will have dedicated registry just for this purpose and there are no organizations so the resulting URL we will pull from will look like this: Therefore, I propose to shorten the initial configuration to just this: distributions:
- id: rhel-10.0-ec2
name: "Red Hat Enterprise Linux 10.0"
type: ec2
image: "rhel/10.0-ec2"
- id: rhel-10.0-azure
name: "Red Hat Enterprise Linux 10.0"
type: azure
image: "rhel/10.0-azure"
- id: rhel-10.0-gcp
name: "Red Hat Enterprise Linux 10.0"
type: gcp
image: "rhel/10.0-gcp"
- id: rhel-10.0-qcow2
name: "Red Hat Enterprise Linux 10.0"
type: qcow2
image: "rhel/10.0-qcow2"We can add more versions later. @croissanne how do you like such config file? |
lzap
left a comment
There was a problem hiding this comment.
Looks good overall, as I said I think the compose endpoint changes should be a separate PR which we can merge later.
You can keep drafting this as one commit, but going forward it would be really nice if this could be separated into two or more commits.
|
Just a question to merging with existing distributions - I imagine it could be done through a parameter, like this: |
If you just need a list then yea, if you need more details per distro then maybe add something in |
internal/v1/handler_compose_image.go
Outdated
| } | ||
| if composeRequest.ImageRequests[0].BootcRef != nil && *composeRequest.ImageRequests[0].BootcRef != "" { | ||
| entry, _ := common.FindImmutableDistributionByID(h.server.immutableDistributions, *composeRequest.ImageRequests[0].BootcRef) | ||
| fullRef := strings.TrimSuffix(h.server.ecrURL, "/") + "/" + strings.TrimPrefix(entry.Image, "/") |
There was a problem hiding this comment.
I wonder if we could use https://pkg.go.dev/net/url#URL.JoinPath
|
Ok ok so I did:
Only one thing remains, and that is Sanne's comment:
And honestly, idk. this way we wouldn't have to have another file and config var to maintain, but on the other hand it would made managing new entries for the dropdown kinda tedious since it would be spread throughout multiple other JSON files... Soooo I guess I leave that up to you @lzap @croissanne to decide 😄 |
I think merging this into current distributions JSON makes sense, it is just simply cleaner. That would be something like: {
"module_platform_id": "platform:el10",
"oscap_name": "rhel10",
"distribution": {
"name": "rhel-10.1",
"composer_name": "rhel-10.1",
"description": "Red Hat Enterprise Linux (RHEL) 10",
"no_package_list": true
},
"x86_64": {
"bootc": [{
"id": "rhel-10.0-ec2",
"type": "ec2",
"image": "rhel/10.0-ec2"
},
... ]
...
}This actually revealed that I forgot about architecture. While those base images are multiarch and we will build both x86_64 and aarm64, the UI user must indicate which architecture do they want. So by putting this under the architecture, this allows is. We can also drop the Are these JSON files being generated somehow? Will we need to update those generators? @croissanne Why do we have Fedoras and CentOSes here when the service is RHEL-only? |
39d554a to
d37658e
Compare
lzap
left a comment
There was a problem hiding this comment.
Looks very good, you left architecture out of equation. Also, I suggest to add filtering for less UI code but it is up to you if you want this or not.
internal/v1/api.yaml
Outdated
| type: string | ||
| description: | | ||
| Name of the content template. Used when registering the system to Insights. | ||
| bootc_ref: |
There was a problem hiding this comment.
I know we haven't implemented a compose handler just yet. But having the bootc ref in the image request differs slightly from the way it was implemented in composer. This just means that cockpit and the hosted apis will diverge slightly and it might be trickier to handle this in the frontend, so this is something to keep in mind.
For composer the compose request would look like:
{
// we omit distribution for bootc compose requests
// and have a top-level bootc item
"bootc": {
"reference": "quay.io/centos-bootc/centos-bootc:stream9"
},
"image_request": {
"architecture": "x86_64",
"image_type": "aws",
"repositories": [],
"upload_options": {
...
}
}
}And for ib-crc it would be:
{
"distribution": "rhel-10",
"image_requests": [
{
"architecture": "x86_64",
"image_type": "aws",
"bootc_ref": "rhel-10.0-ec2",
"upload_request": {
"type": "aws",
"options": {
...
}
}
}
],
"customizations": {}
}There was a problem hiding this comment.
I've changed the bootc field to mirror the composer's structure 😄 now it is in the top level of the compose request
There was a problem hiding this comment.
Good observation, this is a chance to sync up on the terminology because I made a mistake of referring some fields as "reference" or "boot_ref" but the reality in container world is:
quay.io/xxx/yyyy:aaaa: called image reference or ref for shortquay.io: called registry (this will be private information on crc)xxx: called organization or namespace (there is none on crc)yyyy: called image name or repository (most likely justrhel)aaaa: called tag (most likely something like10.1-ec2)
This is just a lot of guesswork, we do not have the crc pipeline yet but I assume it will be something similar to that.
Now, since we do not want to reveal how/where those derived containers are stored, for crc compose request the field should be named something like "bootc_id" or just "id" underneeth "boots" struct. Then the backend can lookup the actual image name and tag part and figure out the complete reference.
There was a problem hiding this comment.
Now, since we do not want to reveal how/where those derived containers are stored, for crc compose request the field should be named something like "bootc_id" or just "id" underneeth "boots" struct. Then the backend can lookup the actual image name and tag part and figure out the complete reference.
Intentionally changing this though makes it incredibly frustrating to work on the frontend though and adds to the diverging APIs. I am 100% for accuracy and correctness, but then we should either change it in osbuild-composer too. Or wait until we do a v3 api and consolidate everything there. At the moment in the frontend we have to interact with both APIs and it's a massive headache already when there are inconsistencies between the two
There was a problem hiding this comment.
No maybe it's me that's misunderstood, my bad. So to clarify, is the idea that the user chooses a dropdown for the image they want and we send for example rhel-10.1-ec2 in the request and not the full reference?
If that's the case, I guess this means the APIs will 100% diverge and we will have to handle that differently on-prem
There was a problem hiding this comment.
How about this - I've renamed bootc.id to bootc.reference so the field name matches composer's API. This way the frontend sends bootc.reference to both APIs, and cockpit will put in a full container reference and hosted service just the part that we call now image_name. On the ib-crc side, the compose handler (future PR) will validate the reference against the known list of image_name values returned by GET /distributions?kind=bootc, and resolve it to a full container reference before forwarding to composer.
There was a problem hiding this comment.
i'd do whatever is in composer currently, I think the benefit of not having diverging APIs is the main thing.
10ec592 to
e6a74b7
Compare
lzap
left a comment
There was a problem hiding this comment.
Few more things. Using existing distributions really helped.
a359d1c to
6919504
Compare
|
@croissanne fyi we talked with @ksiekl and @kingsleyzissou about dropping "id" and using just libcontainer "short name" in bootc-ref. So something like |
468f509 to
da78fae
Compare
croissanne
left a comment
There was a problem hiding this comment.
Overall lgtm, one api tingy and it needs a commit message :)
lzap
left a comment
There was a problem hiding this comment.
Yeah, the API could be improved a bit and tests might be tabular. Looks good over all! I think this is now ready so you can start working against stage on the UI.
Add bootc image entries in distribution JSON files. Add BootcDistributionItem schema and edit the general DistributionResponse - it now returns bootc distributions if the parameter kind=bootc was in the query. Also possible to filter through them via distro/arch/type parameters in the query. Add CollectBootcFromRegistry which gathers all the bootc entries.
GET /api/image-builder/v1/distributions?kind=bootcreturns the list of immutable distributions (id, name, type, image) for the UI dropdown in Image modeconfig/immutable_distributions.jsonwith the structure