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

cmd-generate-hashlist: rename to buildextend-hashlist-experimental #3248

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Nov 29, 2022

That way we can call it using the same pattern we call other buildextend commands. It's not a bona-fide artifact (we don't add anything to meta.json yet) because it's still experimental. But doing it this way will allow us to easily skip running this on pipeliens where we don't need it.

Motivated by issues we're currently hitting in RHCOS with generating the hashlist being excruciatingly slow. Current theory is that the disk is rotational and the random reads that the command triggers by checking out the full commit and hashing everything is extremely expensive on hard drives. I'd like to skip hashlists in RHCOS for now until we can either move parts of it to tmpfs or ideally move to a builder with better storage.

But regardless, this will also help us clean up in the pipeline how this command is called.

cgwalters
cgwalters previously approved these changes Nov 30, 2022
dustymabe
dustymabe previously approved these changes Nov 30, 2022
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@dustymabe
Copy link
Member

suggestion: cosa buildextend-hashlist (drop the exp).

Also, we don't have to backport this to any RHCOS branches because the goal is to not generate the hashlist there?

jlebon added a commit to jlebon/fedora-coreos-pipeline that referenced this pull request Nov 30, 2022
We've made hashlist generation look more like just another buildextend
command. This allows us to clean up our code related to this.

See: coreos/coreos-assembler#3248
@jlebon
Copy link
Member Author

jlebon commented Nov 30, 2022

Will wait until coreos/fedora-coreos-pipeline#776 is stamped before merging this since I didn't bother making this switch ratchetable. Nothing else should be using it.

suggestion: cosa buildextend-hashlist (drop the exp).

I'd like to keep some marker that this is experimental and shouldn't be used by anyone other than devs currently. Probably before "stabilizing" it, we should make it a more proper artifact by adding it to the schema under the images dict (that would also allow us to drop the separate checksum file).

Also, we don't have to backport this to any RHCOS branches because the goal is to not generate the hashlist there?

Right. We haven't been generating it so far in the old pipeline. Short-term, we can keep not generating it. Once we work out the ppc64le I/O issues, we could turn it on for 4.13 and forward (or wait for it to be explicitly requested).

@dustymabe
Copy link
Member

suggestion: cosa buildextend-hashlist (drop the exp).

I'd like to keep some marker that this is experimental and shouldn't be used by anyone other than devs currently. Probably before "stabilizing" it, we should make it a more proper artifact by adding it to the schema under the images dict (that would also allow us to drop the separate checksum file).

counter proposal: cosa buildextend-experimental-hashlist? For me, I'm afraid exp isn't going to immediately lead to experimental in my head.

Again ^^ optional.

That way we can call it using the same pattern we call other buildextend
commands. It's not a bona-fide artifact (we don't add anything to
`meta.json` yet) because it's still experimental. But doing it this way
will allow us to easily skip running this on pipeliens where we don't
need it.

Motivated by issues we're currently hitting in RHCOS with generating the
hashlist being excruciatingly slow. Current theory is that the disk is
rotational and the random reads that the command triggers by checking
out the full commit and hashing everything is extremely expensive on
hard drives. I'd like to skip hashlists in RHCOS for now until we can
either move parts of it to tmpfs or ideally move to a builder with
better storage.

But regardless, this will also help us clean up in the pipeline how this
command is called.
@jlebon jlebon dismissed stale reviews from dustymabe and cgwalters via d4ac721 November 30, 2022 21:38
@jlebon jlebon changed the title cmd-generate-hashlist: rename to buildextend-hashlist-exp cmd-generate-hashlist: rename to buildextend-hashlist-experimental Nov 30, 2022
@dustymabe
Copy link
Member

cosa buildextend-hashlist-experimental works too

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@dustymabe dustymabe enabled auto-merge (rebase) November 30, 2022 21:39
jlebon added a commit to jlebon/fedora-coreos-pipeline that referenced this pull request Nov 30, 2022
We've made hashlist generation look more like just another buildextend
command. This allows us to clean up our code related to this.

See: coreos/coreos-assembler#3248
@dustymabe dustymabe merged commit d82d501 into coreos:main Dec 1, 2022
dustymabe pushed a commit to coreos/fedora-coreos-pipeline that referenced this pull request Dec 1, 2022
We've made hashlist generation look more like just another buildextend
command. This allows us to clean up our code related to this.

See: coreos/coreos-assembler#3248
@jlebon jlebon deleted the pr/hashlist-exp branch April 22, 2023 23:35
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.

None yet

3 participants