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

Build opm cache locally to enable multi-arch builds #465

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

yashvardhannanavati
Copy link
Collaborator

This change is triggered by an OPM upgrade which introduced cache for FBC indexes. More specifically, OPM > 1.26.2

Refers to CLOUDDST-16852

@yashvardhannanavati
Copy link
Collaborator Author

@release-engineering/exd-guild-hello-operator PTAL

@lipoja
Copy link
Contributor

lipoja commented Dec 15, 2022

LGTM, but it would be nice if Joe had a quick look as well.

@@ -406,6 +406,11 @@ def opm_generate_dockerfile(
log.info('Rewriting Dockerfile %s with newly generated by opm.', dockerfile_path)
os.rename(dockerfile_path_opm_default, dockerfile_path)

local_cache_path = os.path.join(base_dir, 'cache')
if not os.path.exists(local_cache_path):

Choose a reason for hiding this comment

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

Is there a case where the local_cache_path would already exist? If it does exist, how do we know if it is synced from the FBC we're about to build the image with?

Copy link

@joelanford joelanford Dec 16, 2022

Choose a reason for hiding this comment

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

I'm pretty sure it's safe to just always run /bin/opm serve {fbc_dir} --cache-dir={local_cache_path} --cache-only, even if the cache dir already exists. It will check cache integrity and only rebuild if hashes don't match.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Changed it to run every time

'RUN ["/bin/opm", "serve", "/configs", "--cache-dir=/tmp/cache", "--cache-only"]',
f'COPY {local_cache_path} /tmp/cache',
)

Choose a reason for hiding this comment

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

Unrelated question for this replacement. Is there a way to detect if the replacement didn't replace anything? In the future, if the dockerfile generation changes again, it would be nice if the failure message is something like: "failed to replace cache building execution in dockerfile" rather than the eventual/cryptic: "exec /bin/opm: exec format error" error that would happen during the image build.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a check to confirm the COPY line is added to the file and throw a more comprehendible error message if the replacement fails.

iib/workers/tasks/opm_operations.py Outdated Show resolved Hide resolved
iib/workers/tasks/opm_operations.py Outdated Show resolved Hide resolved
tests/test_workers/test_tasks/test_opm_operations.py Outdated Show resolved Hide resolved
This change is triggered by an OPM upgrade which introduced cache
for FBC indexes. More specifically, OPM > 1.26.2

Refers to CLOUDDST-16852
@yashvardhannanavati yashvardhannanavati merged commit c045240 into master Dec 19, 2022
@yashvardhannanavati yashvardhannanavati deleted the opm_upgrade_hack branch December 19, 2022 17:28
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