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

Various fixes #108

Merged
merged 4 commits into from
Nov 21, 2022
Merged

Various fixes #108

merged 4 commits into from
Nov 21, 2022

Conversation

gicmo
gicmo previously approved these changes Oct 25, 2022
@gicmo gicmo enabled auto-merge (rebase) October 25, 2022 09:21
ondrejbudai
ondrejbudai previously approved these changes Oct 25, 2022
@thozza thozza dismissed stale reviews from ondrejbudai and gicmo via d0d3c98 October 25, 2022 15:13
@thozza
Copy link
Member Author

thozza commented Oct 25, 2022

I had to update the terraform SHA, because the one currently on main is from a branch that was used for developing a PR and it got deleted...

ondrejbudai
ondrejbudai previously approved these changes Oct 25, 2022
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.

Ouch, thanks!

@thozza
Copy link
Member Author

thozza commented Oct 26, 2022

CI revealed an issue in the schema. By making the bucket optional for GCP upload options and using oneOf for the upload options, the schema now does not have a good way to distinguish AWS and GCP upload options based on their required fields (they are the same). I'll have to think a bit about a proper solution to this... 🤔

@ondrejbudai
Copy link
Member

Yeah, osbuild/osbuild-composer#3018 might fix it I think.

@thozza
Copy link
Member Author

thozza commented Oct 26, 2022

Yeah, osbuild/osbuild-composer#3018 might fix it I think.

I don't think so. The issue happens in the hub plugin, which reports issue when trying to validate a request against schema. The issue is that it matches two of the options, which is in conflict with the oneOf.

@thozza
Copy link
Member Author

thozza commented Nov 3, 2022

To sum up the schema issue that manifested after I marked more properties from upload_options as not required:

  • The problem is that a the upload_options for AWS EC2 can sometimes also match the GCP upload_options schema. Because the the upload_options uses oneOf, the validation fails in such a case, because exactly one option must match the object.
  • In osbuild-composer, we have a similar problem, which will be indeed solved by cloudapi: make ImageRequests tied to their UploadOptions osbuild-composer#3018. Specifically by tying the image-type into the upload_options schema.
  • We can't use the approach from cloudapi: make ImageRequests tied to their UploadOptions osbuild-composer#3018 in koji-osbuild, becuase we have a different schema in the Hub and image-type is defined on a different level. We could rework the schema, but to make it backward compatible, it would become ugly (we would most probably end up with oneOf choice of the old schema and new "fixed" schema).

After discussing our options with @ondrejbudai, we agreed that falling back to defining upload_options just as an empty object with additionalProperties set to True is probably the easiest and still reasonable approach. The Builder defines the Cloud API version that it uses, so if we ever move to v3, we would handle the backward compatibility as an additional option in the schema (CLI argument), which would specify the Cloud API version.

After thinking about this more, I realized that there is a compromise between completely reworking schema and using an empty object with additional properties. That's to use anyOf, instead of oneOf for upload_options. This way we can still validate the request in the Hub plugin. It does not really matter too much that the options may match more than one definition, since we are just passing it through in all the plugins to the Cloud API.

@thozza thozza disabled auto-merge November 3, 2022 09:48
@thozza thozza force-pushed the various-fixes branch 2 times, most recently from b88c9ff to 042f135 Compare November 15, 2022 14:07
The builder plugin could produce a traceback when image build failed,
but it didn't contain any details.

```
Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/koji/daemon.py", line 1468, in runTask
    response = (handler.run(),)
  File "/usr/lib/python3.6/site-packages/koji/tasks.py", line 335, in run
    return koji.util.call_with_argcheck(self.handler, self.params, self.opts)
  File "/usr/lib/python3.6/site-packages/koji/util.py", line 271, in call_with_argcheck
    return func(*args, **kwargs)
  File "/usr/lib/koji-builder-plugins/osbuild.py", line 719, in handler
    status = client.wait_for_compose(cid, callback=self.on_status_update)
  File "/usr/lib/koji-builder-plugins/osbuild.py", line 483, in wait_for_compose
    status = self.compose_status(compose_id)
  File "/usr/lib/koji-builder-plugins/osbuild.py", line 454, in compose_status
    return ComposeStatus.from_dict(res.json())
  File "/usr/lib/koji-builder-plugins/osbuild.py", line 251, in from_dict
    ImageStatus.from_dict(s) for s in data["image_statuses"]
  File "/usr/lib/koji-builder-plugins/osbuild.py", line 251, in <listcomp>
    ImageStatus.from_dict(s) for s in data["image_statuses"]
  File "/usr/lib/koji-builder-plugins/osbuild.py", line 228, in from_dict
    error = ComposeStatusError(error_id=error_id, **error)
TypeError: __init__() missing 1 required positional argument: 'details'
```

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
The GCP Bucket may be (and usually will be) set on the composer-worker.

Related to osbuild/osbuild-composer#3023.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
The Azure Resource Group Location is no longer required and
osbuild-composer can determine the correct Location from the
provided Resource Group.

Related to osbuild/osbuild-composer#3093.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
It turned out, that the upload options for AWS EC2 or GCP with just the
required properties specified, would match both schemas. This is causing
the validation fail with `oneOf` used for `upload_options`.

This will be fixed in osbuild-composer PR#3018. However, we can't use
the same approach for koji-osbuild, while keeping the schema backward
compatible and sane.

Another discussed option would be to define `upload_options` as an empty
object with `additionalProperties` set to `True`. This would
effectively mean no validation of `upload_options`. None of the plugins
actually modify the `upload_options` in any way. It is passed as
provided to `osbuild-composer`.

I think that the change in this commit is a compromise. The validation
of the `upload_options` schema is kept, but it is relaxed to `anyOf`.

[1] osbuild/osbuild-composer#3018

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
@ondrejbudai ondrejbudai merged commit 3d1c24f into osbuild:main Nov 21, 2022
@thozza thozza deleted the various-fixes branch December 5, 2022 09:24
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