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

cloudapi: Add ability to skip uploading and save image locally #3585

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Jul 28, 2023

During development it can be very useful to store the results locally instead of uploading to a remote system. This implements a development only option to help with that.

To use it you need to add CLOUD_LOCALSAVE to the server's environment. This can be done by editing /usr/lib/systemd/system/osbuild-composer.service and adding:

Environment="CLOUD_LOCALSAVE=1"

You can then use an 'upload_options' object to skip trying to upload to the default service for the type of image, eg:

"image_requests": [
{
  "architecture": "x86_64",
  "image_type": "guest-image",
  "upload_options": {
      "local-save": true
  },
  ...
}]

The results will be saved to /var/lib/osbuild-composer/artifacts/UUID/ using the default filename for the image type.

This pull request includes:

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

I chose not to try to add testing for this since it is meant as developer only, and any bugs that crop up will be caught in use by someone who can then fix it.

@bcl bcl requested a review from achilleas-k July 28, 2023 19:06
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

I like this idea. The only minor comment would be to maybe namespace the env var to something like IMAGEBUILDER_CLOUD_LOCALSAVE or something. I doubt there would be any conflict for anyone running the cloud API, but you never know where these things might end up in the future.

Another idea, to make it really truly a devel option, might be to put it behind a build flag, but that might be a step too far.

@bcl
Copy link
Contributor Author

bcl commented Jul 28, 2023

CLOUD_ is the namespace I randomly picked over OSBUILD_ :) IMAGEBUILDER_ seems pretty awkward though. I considered a cmdline option (plumbing that into the cloud v2 module would be a pain), and buildtime option but I can see a situation where I would want to just switch this on without rebuilding to examine something. Checking an environmental variable is safe and fast so I think it's ok.

@teg
Copy link
Member

teg commented Jul 29, 2023

I wonder if we want to have a generic mechanism for these type of developer features to avoid them becoming API by accident (shipped in RHEL, discovered and relied upon by a customer).

A compile time option is one way, another might be to introduce an option in the API that you have to specify to indicate you understand you can't rely on the behavior. "unstable": true or something like that.

Specifically for this option (for the future), I wonder if there is a way to make this useful outside of development. On prem the worker API allows you to upload the image to composer, in the cloud we clearly can't support that (composer has no state). Though would it make sense to support a generic HTTP POST target and on prem it just happens to point to composer itself, but we could point it at anything? I'm not sure if that's useful or how to make it generic enough / deal with authentication, but if there is a way to make it work similarly to the generic S3 target I think that might be neat.

@bcl
Copy link
Contributor Author

bcl commented Jul 31, 2023

I agree that we don't want this to become implicit API. As far as extending it to do HTTP POST, etc. I think that's a different issue that could be addressed later along with removing all the state of the weldr API.

I think the requirement for an environmental variable is enough to avoid users depending on it, unless they really know what they are doing. You need local access, and need to edit the .service file and need to re-edit it after upgrading. FWIW I'll change the 'namespace' to OSBUILD to make it more specific to our use.

Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

Making development of composer easier is always nice. I agree that having a unified method for introducing development-only (power user-only?) features is currently a heavily discussed topic, and we should solve it at some point, but I don't want to block this PR on this decision.

Other than my the hyphen in the API (see below), this looks good. :)

internal/cloudapi/v2/openapi.v2.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD 82bbf12 with the main merge-base 4c7b3dd). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/4792260662/artifacts/browse

internal/cloudapi/v2/handler.go Outdated Show resolved Hide resolved
During development it can be very useful to store the results locally
instead of uploading to a remote system. This implements a development
only option to help with that.

To use it you need to add OSBUILD_LOCALSAVE to the server's environment.
This can be done by editing /usr/lib/systemd/system/osbuild-composer.service
and adding:

Environment="OSBUILD_LOCALSAVE=1"

You can then use an 'upload_options' object to skip trying to upload to
the default service for the type of image, eg:

    "image_requests": [
    {
      "architecture": "x86_64",
      "image_type": "guest-image",
      "upload_options": {
          "local_save": true
      },
      ...
    }]

The results will be saved to /var/lib/osbuild-composer/artifacts/UUID/
using the default filename for the image type.

If local_save is used without OSBUILD_LOCALSAVE being set it will return
an error with id=36 saying 'local_save is not enabled'.
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.

LGTM

@thozza
Copy link
Member

thozza commented Aug 8, 2023

Out of curiosity, the adequate documentation informing people about the change such as is checked, but I'm not sure where is this feature documented 🤔 Do you plan to open a PR for the guides repo?

@bcl
Copy link
Contributor Author

bcl commented Aug 8, 2023

Out of curiosity, the adequate documentation informing people about the change such as is checked, but I'm not sure where is this feature documented thinking Do you plan to open a PR for the guides repo?

I figured the commit message was the documentation since this isn't really meant for end user consumption. I wouldn't want to add it to the normal guide.

@bcl bcl dismissed ondrejbudai’s stale review August 8, 2023 15:21

Fixed it, using underscore now.

@thozza
Copy link
Member

thozza commented Aug 8, 2023

I figured the commit message was the documentation since this isn't really meant for end user consumption. I wouldn't want to add it to the normal guide.

Guides actually have a developer section. I still think it would be useful for anyone new joining the project. See https://github.com/osbuild/guides/tree/main/src/developer-guide

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

LGTM. Good to merge since guides are external, right?

@thozza
Copy link
Member

thozza commented Aug 9, 2023

LGTM. Good to merge since guides are external, right?

Yup... 😉

@thozza thozza merged commit 139bf4d into osbuild:main Aug 9, 2023
32 checks passed
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.

6 participants