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

src/cmd-koji-upload: introduce file mutators #699

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@darkmuggle
Copy link
Contributor

commented Aug 8, 2019

This is a rework of the file upload logic. One of the problems with Koji
Content Generators (cmd-koji-upload is such a thing) is that unless Koji
know about the content type being uploaded and the content type matches
a specific pattern than an upload will be rejected.

In the past we have uploaded uncompressed artifacts and renamed some
files to raw. A problem was hit when we tried to upload vhd.gz files.
We could have done the rename approach or come up with a more dynamic
solution.

This also addresses some wonky code. This refactor allow us to do the Koji
uploads, with out mucking with the rest of COSA by doing an upload
conversion for renaming true "raw" object like the kernel, and when a
supported format is not allowed to be uploaded in compressed fashion, do
a run-time decompress into a temporary directory.

This should allow almost all uploads to be compressed while we purse
PR's like https://pagure.io/koji/pull-request/1608 to percolate.

@darkmuggle darkmuggle requested a review from ashcrow Aug 8, 2019

@darkmuggle

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Calling this WIP. I need to test this and do some more work on it. I'd appreciate an early review!

@cgwalters
Copy link
Member

left a comment

Superficial LGTM

Show resolved Hide resolved src/cmd-koji-upload Outdated
@cgwalters

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I have no problem with this, code looks sane! However at a high level I do wonder whether it's worth fighting this battle - if we view Koji just as a "backup", we could just tar up all the artifacts into a single rhcos-$version.tar.gz and provide that, and never have to touch the Koji database schema ever again for example.

(Or alternatively, just create a .tar.gz for each artifact)

@cgwalters

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Per this doc as people know I view Koji as a legacy thing for the most part. Not everyone agrees 😉 - just stating my 2¢. (Also not unrelated to this it is definitely frustrating that the upload part takes 1.5 hours but that's partially orthogonal)

Show resolved Hide resolved src/cmd-koji-upload Outdated
@ashcrow

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

So far it looks good.

I have no problem with this, code looks sane! However at a high level I do wonder whether it's worth fighting this battle - if we view Koji just as a "backup", we could just tar up all the artifacts into a single rhcos-$version.tar.gz and provide that, and never have to touch the Koji database schema ever again for example.

(Or alternatively, just create a .tar.gz for each artifact)

That's a possibility. @imcleod do you think the above would be acceptable if Ben decided to go that route?

@miabbott

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

I have no problem with this, code looks sane! However at a high level I do wonder whether it's worth fighting this battle - if we view Koji just as a "backup", we could just tar up all the artifacts into a single rhcos-$version.tar.gz and provide that, and never have to touch the Koji database schema ever again for example.

(Or alternatively, just create a .tar.gz for each artifact)

My only hesitation about this approach is getting those artifacts in a format that are consumable by the RHT Errata Tool. We are already getting feedback about the inability to properly attach the container tarball to errata because the container scanning tool doesn't know about tarballs. I'd rather not see us introduce additional reasons why our artifacts are difficult to work with in that scope.

Since RHT still ships everything via errata, I think it would look good if we could interoperate with that tooling more easily than not.

@cgwalters

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I think Errata Tool needs to understand the coreos-assembler meta.json file.

@cgwalters

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

We discussed this in a quick chat and as of today, we are not attaching RHCOS bootimages to Errata Tool and there is no ask to do so.

@darkmuggle darkmuggle force-pushed the darkmuggle:weak-koji branch from 8a14242 to 8d705b5 Aug 12, 2019

src/cmd-koji-upload: introduce file mutators
This is a rework of the file upload logic. One of the problems with Koji
Content Generators (cmd-koji-upload is such a thing) is that unless Koji
know about the content type being uploaded and the content type matches
a specific pattern than an upload will be rejected.

In the past we have uploaded uncompressed artifacts and renamed some
files to raw. A problem was hit when we tried to upload `vhd.gz` files.
We could have done the rename approach or come up with a more dynamic
solution.

This also addresses some wonky code. This refactor allow us to do the Koji
uploads, with out mucking with the rest of COSA by doing an upload
conversion for renaming true "raw" object like the kernel, and when a
supported format is not allowed to be uploaded in compressed fashion, do
a run-time decompress into a temporary directory.

This should allow almost _all_ uploads to be compressed while we purse
PR's like https://pagure.io/koji/pull-request/1608 to percolate.

@darkmuggle darkmuggle force-pushed the darkmuggle:weak-koji branch from 8d705b5 to eb7a61f Aug 12, 2019

@darkmuggle

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

I have no problem with this, code looks sane! However at a high level I do wonder whether it's worth fighting this battle - if we view Koji just as a "backup", we could just tar up all the artifacts into a single rhcos-$version.tar.gz and provide that, and never have to touch the Koji database schema ever again for example.

(Or alternatively, just create a .tar.gz for each artifact)

I considered having a single tar.gz and calling it a day.

I do agree with @cgwalters sediment. Fighting with Koji is painful. The only reason, AFAIK, that fighting this battle is worth it is simply because it is a release requirement. Beyond "having the build in Koji" checkbox, I do not see any real value. Perhaps in the 4.3 cycle, we could challenge the requirement?

@darkmuggle darkmuggle changed the title WIP: src/cmd-koji-upload: introduce file mutators src/cmd-koji-upload: introduce file mutators Aug 12, 2019

@darkmuggle

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@ashcrow ready for final review and merge. It's all tested.

self._tmpdir = tempfile.mkdtemp(prefix="koji-build")
_Build.__init__(self, *args, **kwargs)

def __del__(self):

This comment has been minimized.

Copy link
@ashcrow

ashcrow Aug 12, 2019

Collaborator

Future: This may makes sense to be pushed up into the _Build class in the future.

@@ -95,6 +96,10 @@ KOJI_CG_TYPES = {
# against content types.
RENAME_RAW = ['initramfs.img', '-kernel', "ostree-commit"]

# These are compressed extensions that are used to determine if name managling

This comment has been minimized.

Copy link
@ashcrow

ashcrow Aug 12, 2019

Collaborator

Super nit: #: for documenting module level variables/constants. Not worth changing IMHO right now.

@ashcrow
Copy link
Collaborator

left a comment

👍

@ashcrow

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

/cc @vrutkovs for a second review

@vrutkovs
Copy link
Contributor

left a comment

LGTM

@vrutkovs
Copy link
Contributor

left a comment

LGTM

@ashcrow ashcrow merged commit 55eddd5 into coreos:master Aug 13, 2019

2 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.