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

Cloud API: Support selecting multiple upload targets and add Pulp OSTree uploads #3744

Merged
merged 16 commits into from
Nov 17, 2023

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented Oct 16, 2023

This PR is a follow-up to #3636 and replaces #3672. It requires #3636 but isn't based on it (it wont compile until it is).


The purpose of this PR is to add the ability to upload edge and iot commits built through the cloud API to a Pulp server that supports ostree repos. However, at the same time, we need to be able to support the old behaviour of uploading ostree commits to AWS S3. This is a required part of HMS-1798.

I also think it's a good idea in general to be able to specify alternative upload targets for certain artifacts. We could support uploading any artifact to S3 for example.

The main change in this PR is that an image request now includes an (optional) upload_target option which is an enum of the names of all the upload targets we support in the cloud API (in the form org.osbuild.<target>, e.g., org.osbuild.aws.s3). Omitting the upload_target maintains the current behaviour, which means each image type has a default target. Also, we define a mapping between upload targets and image types that they support and any invalid combination is a bad request.

The commit that adds the pulp ostree upload target to the cloud API was cherry picked from #3672 (thanks @kingsleyzissou).


This pull request includes:

  • adequate testing for the new functionality or fixed issue
  • adequate documentation informing people about the change such as

@achilleas-k achilleas-k force-pushed the decouple-upload-targets branch 2 times, most recently from c8ad6ab to f85e067 Compare October 18, 2023 20:06
@achilleas-k achilleas-k marked this pull request as ready for review October 18, 2023 20:06
@achilleas-k
Copy link
Member Author

#3636 has been merged. Rebased and marked as ready.

@thozza thozza self-requested a review October 20, 2023 13:39
@achilleas-k achilleas-k marked this pull request as draft October 20, 2023 15:28
@achilleas-k
Copy link
Member Author

Converted to draft to block this while I work on supporting multiple targets.

@achilleas-k
Copy link
Member Author

achilleas-k commented Oct 26, 2023

Sorry this took so long. Multiple upload targets added.

This now supports what @croissanne suggested and also adds the default upload options with the default target for the image type if specified, so all the following examples are valid:

Single explicit upload target:

{
  "architecture": "x86_64",
  "image_type": "edge-commit",
  "upload_targets": [
    {
      "type": "org.osbuild.aws.s3",
      "upload_options": { ... }
    }
  ]
}

{
  "architecture": "x86_64",
  "image_type": "edge-commit",
  "upload_targets": [
    {
      "type": "org.osbuild.pulp.ostree",
      "upload_options": { ... }
    }
  ]
}

{
  "architecture": "x86_64",
  "image_type": "guest-image",
  "upload_targets": [
    {
      "type": "org.osbuild.aws.s3",
      "upload_options": { ... }
    }
  ]
}

Default only (will select the default for the image type):

{
  "architecture": "x86_64",
  "image_type": "edge-commit",
  "upload_options": { ... },
  "upload_targets": []
}

{
  "architecture": "x86_64",
  "image_type": "guest-image",
  "upload_options": { ... },
  "upload_targets": []
}

One explicit upload target + a default:

{
  "architecture": "x86_64",
  "image_type": "edge-commit",
  "upload_options": { ... },
  "upload_targets": [
    {
      "type": "org.osbuild.pulp.ostree",
      "upload_options": { ... }
    }
  ]
}

{
  "architecture": "x86_64",
  "image_type": "guest-image",
  "upload_options": { ... },
  "upload_targets": [
    {
      "type": "org.osbuild.aws.s3",
      "upload_options": { ... }
    }
  ]
}

Multiples of the same upload target are supported.

@achilleas-k
Copy link
Member Author

achilleas-k commented Oct 26, 2023

We should add some tests for this. I'm thinking we can change some of the compose requests in the API tests to use the new UploadTargets and for types that get uploaded to S3, we can define multiple upload targets and check that all of them succeeded.

@achilleas-k achilleas-k added the WIP+test Work in progress but run Gitlab CI. label Oct 26, 2023
@achilleas-k
Copy link
Member Author

Edge test failures are failures to fetch from the rpm repos for the osbuild-composer build. A glitch, I guess?

@achilleas-k
Copy link
Member Author

achilleas-k commented Oct 27, 2023

The response for the compose status is

    ComposeStatus:
      allOf:
      - $ref: '#/components/schemas/ObjectReference'
      - type: object
        required:
          - status
          - image_status
        properties:
          status:
            $ref: '#/components/schemas/ComposeStatusValue'
          image_status:
            $ref: '#/components/schemas/ImageStatus'
          image_statuses:
            type: array
            items:
              $ref: '#/components/schemas/ImageStatus'
          koji_status:
            $ref: '#/components/schemas/KojiStatus'

which means we can use the image_statuses array to send responses for the multiple uploads:

    ImageStatus:
      required:
       - status
      properties:
        status:
          $ref: '#/components/schemas/ImageStatusValue'
        upload_status:
          $ref: '#/components/schemas/UploadStatus'
        error:
          $ref: '#/components/schemas/ComposeStatusError'

but what should we do with the image_status at the top level? Should it mirror the request? Meaning, if the request defines upload_options in the old top level part of the request, the result of that specific upload request goes into the image_status. Any upload defined in upload_targets goes in the image_statuses. Do we need to keep the order the same so they match the request? Am I overcomplicating this?

EDIT: Spoke with @croissanne and we decided instead to add upload_statuses to image_status.

@achilleas-k achilleas-k force-pushed the decouple-upload-targets branch 2 times, most recently from 4aa3d9b to 19da7a6 Compare October 30, 2023 12: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.

Nice work, the PR looks good! I have just one small nitpick...

internal/cloudapi/v2/imagerequest.go Outdated Show resolved Hide resolved
@achilleas-k achilleas-k marked this pull request as ready for review October 30, 2023 16:56
@achilleas-k
Copy link
Member Author

Added support for multiple upload targets and the response now includes an array of upload_status objects under upload_statuses. The top-level upload_status is always a copy of the first element of upload_statuses, so in the most common case where we only have one upload target, the upload_statuses array has only one element that is the same as the existing upload_status.

The structure was discussed and agreed with @croissanne.

@achilleas-k achilleas-k removed the WIP+test Work in progress but run Gitlab CI. label Oct 30, 2023
@croissanne
Copy link
Member

Rebased to prevent OCI private key leaking. Apologies :(

@achilleas-k
Copy link
Member Author

achilleas-k commented Nov 8, 2023

I think the RHEL 8.8 RPM builds are getting hungry and killed again:

Nov 08 17:40:46 ip-172-31-22-179.ec2.internal kernel: oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),cpuset=/,mems_allowed=0,global_oom,task_memcg=/machine.slice/machine-cb421a227be348e29fe19c158933ca72.scope,task=compile,pid=29577,uid=1000
Nov 08 17:40:46 ip-172-31-22-179.ec2.internal kernel: Out of memory: Killed process 29577 (compile) total-vm:2798720kB, anon-rss:1645056kB, file-rss:0kB, shmem-rss:1088kB, UID:1000 pgtables:640kB oom_score_adj:0

From RHEL 8.8 aarch64 rpm build journal log: https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/5490961946

This time it's happening on all 8.8 builds, regardless of architecture. The last time we had this issue it was on all EL8 builds for aarch64. Weird.

@achilleas-k achilleas-k force-pushed the decouple-upload-targets branch 2 times, most recently from 0d55230 to ca76d14 Compare November 13, 2023 19:01
achilleas-k and others added 16 commits November 15, 2023 10:07
Add an upload_targets field to the image request.  This lets the API
caller specify multiple upload targets and upload options to be used.
If the upload target type does not match the upload options, the request
is invalid.

For backwards compatibility, the upload targets field is optional.  If
it is not specified, the default upload target and upload options for
the image type are assumed, which is the same as the old behaviour.

Adding an explicit selection to the request makes it possible to support
multiple upload targets for the same image type.  We plan to support
ostree commits being uploaded to both aws.s3 and pulp.

To report on the multiple upload requests, we add an upload_statuses
field to the ImageStatus response.
To report on the multiple upload requests, we add an upload_statuses
property to the ImageStatus response.
Separate the handling of each individual target type into its own
function called by GetTarget()'s case switch.  This makes the function
more readable and the target object creation reusable.

Added an empty line after each creation of irTarget to make it easier to
visually distinguish the cases that fall through.
Separate the target selection in GetTarget() into two steps.  First
determine the default target name for the image type and then use the
name to initialise the target object.  This is a bit more work (and
double switching) but will be needed to support selecting targets
externally.
Add an array of targets in the imageRequest and return an array from
ImageRequest.GetTargets() (renamed from GetTarget()).  Currently, the
function still only returns one target, the default for the image type
with the top level upload options.
Read the upload target types and options in the UploadTargets array of
the ImageRequest and initialise the Target array.  If the top-level
(old) UploadOptions are also specified, prepend them to the array using
the image type's default target type.

Each upload target type is checked against a support map for
compatibility.
It is now valid for UploadOptions to be nil but only if there is at
least one UploadTarget defined.
Test some valid and invalid combinations for the GetTargets() upload
target selection.
Includes tests with and without the upload options for the default
target.
Add the new upload_statuses under the image_status in the result of the
ComposeStatus object.  The first status is also included in the old
top-level 'upload_status' property for backwards compatibility.

Tests are updated to match the new results.
Add the pulp.ostree upload target to the cloud API and enable it for
edge/iot commits.

Co-Authored-By: Achilleas Koutsou <[email protected]>
When making the upload request for edge commit image types, use the new
upload_targets array to define the aws.s3 upload options.
Leave other upload target definitions as is for now to test the old
options.
Check that the first element of the upload_statuses array matches the
top-level upload_status.
We only test one upload target for now.
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

thank you! this was a big effort ._.

@croissanne croissanne merged commit 901393d into osbuild:main Nov 17, 2023
72 checks passed
@achilleas-k achilleas-k deleted the decouple-upload-targets branch November 17, 2023 15:59
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.

5 participants