feat(containers): Remove reliance on "chained builds" architecture for image builds #924
Conversation
andyatmiami
force-pushed
the
feat/no-chained-builds
branch
from
40aa1f4 to
13e78b0
Compare
andyatmiami
changed the title
| #################### | ||
| # base # | ||
| #################### | ||
| FROM registry.access.redhat.com/ubi9/python-311:latest AS base |
There was a problem hiding this comment.
Since we're moving to konflux soon anyway, what do you think about moving from latest to an actual sha/tag for the base images? This may help also #901 maybe?
@jiridanek WDYT?
Note: no need to do it as part of this PR so that such change won't be lost in all the other changes.
There was a problem hiding this comment.
absolutely not in this PR
I'm guessing if we let renovate to run, it will rewrite the tags first thing ;p so maybe no manual effort needed
good thought, tho
There was a problem hiding this comment.
| fix-permissions /opt/app-root -P | ||
|
|
||
| WORKDIR /opt/app-root/src | ||
|
|
||
| CMD ["/opt/app-root/bin/run-code-server.sh"] |
There was a problem hiding this comment.
Eventually I think we should unify to use ENTRYPOINT for all our images in the future. Or do we have some reason to use ENTRYPOINT somewhere and CMD command elsewhere that I'm missing? 🤔
https://www.docker.com/blog/docker-best-practices-choosing-between-run-cmd-and-entrypoint/
Note: again, not something to deal with in this PR.
There was a problem hiding this comment.
It's that somewhere we're using the s2i-provided entrypoint and somewhere we decided not to, I think... We should stop using s2i base images and use normal ubi ;P
There was a problem hiding this comment.
- Eventually I think we should unify to use ENTRYPOINT for all our images in the future. Or do we have some reason to use ENTRYPOINT somewhere and CMD command elsewhere that I'm missing? 🤔 #926
- It's that somewhere we're using the s2i-provided entrypoint and somewhere we decided not to, I think... We should stop using s2i base images and use normal ubi ;P #927
|
/testwith opendatahub-io/notebooks/main/notebook-jupyter-ubi9-python-3-11-pr-image-mirror openshift/release#62160 |
|
@atheo89, |
|
/testwith opendatahub-io/notebooks/main/jupyter-ds-ubi9-python-3-11-pr-image-mirror openshift/release#62160 |
|
@atheo89, |
|
@andyatmiami thank you for this. I scanned through these changes; LGTM in general. Didn't do the extended thorough check though. Just to be sure - you checked the image differences by your script for all the images we are touching here? Regarding the testing - apart from manual effort, I think that the first step should be to make the pytest tests pass here in this PR for these changes so we utilize at least that minimal automated checks in this repository. I can take a look on some of the failing CI checks, just feel free to let me know so we aren't duplicating our work. I put some comments, but these are more just an ideas for the future or discussion and shouldn't be addressed in this PR anyway. |
|
@jstourac - appreciate the review... w.r.t the testing I did do so far.. it was using the (admittedly initially diificult to work with) so all these images are of "comparable" (in most cases identical) image size... The few places the image size is different - its due to minor corrections made in the PR (example: explicitly adding |
|
/testwith opendatahub-io/notebooks/main/notebook-jupyter-ubi9-python-3-11-pr-image-mirror |
|
@andyatmiami /testwith does not work for testing changes done in openshift/release, if you read the thread that andriana posted the link to, that's what we concluded |
|
we have to merge andriana's pr first, and then it's possible to rerun the ocp-ci checks and they should start running |
…r image builds
Commit contains following changes:
- Refactors all `Dockerfile`s to to be "self-contained" - not rely on any "build chains" for internal images
- please note in the final form of this PR - many intermediate stages will be combined... this current form makes it easier to understand how the `Dockerfile`s present in the "build chain" are being combined - long term - we'd expect the following stages: `base`, `base-<accelerator>` (if applicable), `<final>`
- `Dockerfile` file now has a file extension/suffix that indicates CPU or accelerator
- `LABEL` directives now should be consistent/accurate
- this probably needs a little more attention
- `base/` directory removed as its no longer relevant/required
- `rocm/` directory removed as its no longer relevant/required
- `wheel` + `setuptools` explicitly added to `runtime-` images `Pipfile`
- requirements.txt also regenerated as a result
- Makefile updated to remove pre-reqs on container image targets
- Makefile updated to handle support better across releases
- _read: MOWR variables_
- `pytorch` `Makefile` targets now contain `cuda-` prefix
- `rocm` + `cuda` `datascience` `Makefile` targets removed as we don't ship those variants
- with no chained builds - all datascience-dependent images contain their own required `Dockerfile` logic!
- `ENV` directive in `Dockerfile` now properly uses `=` (vs. whitespace)
- change in `buildinputs` to pull file paths from the terminal layer
- this change particularly needs refined - but its "crude yet effective" in its current form
Related-to: https://issues.redhat.com/browse/RHOAIENG-19048
andyatmiami
force-pushed
the
feat/no-chained-builds
branch
from
13e78b0 to
5007663
Compare
|
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/notebooks-ubi9-e2e-tests In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/notebooks-ubi9-e2e-tests |
|
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/notebooks-ubi9-e2e-tests In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/rocm-notebooks-e2e-tests |
|
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/rocm-notebooks-e2e-tests In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/ |
|
@jiridanek: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/rstudio-notebook-e2e-tests |
|
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/rstudio-notebook-e2e-tests In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/ |
|
@jiridanek: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/codeserver-notebook-e2e-tests |
|
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/codeserver-notebook-e2e-tests In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/rocm-runtimes-ubi9-e2e-tests |
|
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/rocm-runtimes-ubi9-e2e-tests In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override cilprow/rocm-runtimes-ubi9-e2e-tests |
|
@jiridanek: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/rocm-runtimes-ubi9-e2e-tests |
|
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/rocm-runtimes-ubi9-e2e-tests In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/runtimes-ubi9-e2e-tests |
|
@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/runtimes-ubi9-e2e-tests In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-bot
bot
merged commit 73f1c91
into
opendatahub-io:main
The recent change RHOAIENG-19048 that removed build chains made the option obsolete * opendatahub-io#924 This modifies the Makefile customization feature as implemented in RHOAIENG-9822 * opendatahub-io#657
…r rhel9 image builds This work is a continuation of changes originally implemented in `opendatahub-io/notebooks` under opendatahub-io#924. The `red-hat-data-services/notebooks` repo has some functional differences related to the `rstudio` images which necessitates additional changes: - `rhel9` builder image is used (instead of `c9s`) - `BuildConfig` manifest is used to build the image (due to reliance on `subscription-manager`) (instead of naive `Makefile` / container build) Commit contains following changes: - Refactors all `rhel9` `rstudio` `Dockerfile`s to to be "self-contained" - not rely on any "build chains" for internal images - please note in the final form of this PR - many intermediate stages will be combined... this current form makes it easier to understand how the `Dockerfile`s present in the "build chain" are being combined - long term - we'd expect the following stages: `base`, `base-<accelerator>` (if applicable), `<final>` - `Dockerfile` file now has a file extension/suffix that indicates CPU or accelerator - `LABEL` directives now should be consistent/accurate - this probably needs a little more attention - `base/rhel9-python-3.11` directory removed as its no longer relevant/required - Makefile updated to remove outdated `base-rhel9-python-3.9` target - `ENV` directive in `Dockerfile` now properly uses `=` (vs. whitespace) - `rstudio` `BuildConfig` manifests updated to accomodate changes outlined above - `Dockerfile.xxx` reference - No chained build - For `BuildConfig` manifest - this means removing the `from` attribute from `dockerStrategy`
…ript With the chained build removal changes in, we need not worry about build chains in GHA. * opendatahub-io#924 This means that there is no benefit from generating the image build workflow upfront and committing that. Previously the benefit was that ad-hoc matrix jobs could not have dependencies on one another, whereas these generated jobs could.
…r rhel9 image builds This work is a continuation of changes originally implemented in `opendatahub-io/notebooks` under opendatahub-io#924. The `red-hat-data-services/notebooks` repo has some functional differences related to the `rstudio` images which necessitates additional changes: - `rhel9` builder image is used (instead of `c9s`) - `BuildConfig` manifest is used to build the image (due to reliance on `subscription-manager`) (instead of naive `Makefile` / container build) Commit contains following changes: - Refactors all `rhel9` `rstudio` `Dockerfile`s to to be "self-contained" - not rely on any "build chains" for internal images - please note in the final form of this PR - many intermediate stages will be combined... this current form makes it easier to understand how the `Dockerfile`s present in the "build chain" are being combined - long term - we'd expect the following stages: `base`, `base-<accelerator>` (if applicable), `<final>` - `Dockerfile` file now has a file extension/suffix that indicates CPU or accelerator - `LABEL` directives now should be consistent/accurate - this probably needs a little more attention - `base/rhel9-python-3.11` directory removed as its no longer relevant/required - Makefile updated to remove outdated `base-rhel9-python-3.9` target - `ENV` directive in `Dockerfile` now properly uses `=` (vs. whitespace) - `rstudio` `BuildConfig` manifests updated to accomodate changes outlined above - `Dockerfile.xxx` reference - No chained build - For `BuildConfig` manifest - this means removing the `from` attribute from `dockerStrategy`
…r rhel9 image builds This work is a continuation of changes originally implemented in `opendatahub-io/notebooks` under opendatahub-io#924. The `red-hat-data-services/notebooks` repo has some functional differences related to the `rstudio` images which necessitates additional changes: - `rhel9` builder image is used (instead of `c9s`) - `BuildConfig` manifest is used to build the image (due to reliance on `subscription-manager`) (instead of naive `Makefile` / container build) Commit contains following changes: - Refactors all `rhel9` `rstudio` `Dockerfile`s to to be "self-contained" - not rely on any "build chains" for internal images - please note in the final form of this PR - many intermediate stages will be combined... this current form makes it easier to understand how the `Dockerfile`s present in the "build chain" are being combined - long term - we'd expect the following stages: `base`, `base-<accelerator>` (if applicable), `<final>` - `Dockerfile` file now has a file extension/suffix that indicates CPU or accelerator - `LABEL` directives now should be consistent/accurate - this probably needs a little more attention - `base/rhel9-python-3.11` directory removed as its no longer relevant/required - Makefile updated to remove outdated `base-rhel9-python-3.9` target - `ENV` directive in `Dockerfile` now properly uses `=` (vs. whitespace) - `rstudio` `BuildConfig` manifests updated to accomodate changes outlined above - `Dockerfile.xxx` reference - No chained build - For `BuildConfig` manifest - this means removing the `from` attribute from `dockerStrategy`
Related openshift/release PR
Description
Commit contains following changes:
Dockerfiles to to be "self-contained" - not rely on any "build chains" for internal imagesDockerfiles present in the "build chain" are being combined - long term - we'd expect the following stages:base,base-<accelerator>(if applicable),<final>Dockerfilefile now has a file extension/suffix that indicates CPU or acceleratorLABELdirectives now should be consistent/accuratebase/directory removed as its no longer relevant/requiredrocm/directory removed as its no longer relevant/requiredwheel+setuptoolsexplicitly added toruntime-imagesPipfilepytorchMakefiletargets now containcuda-prefixrocm+cudadatascienceMakefiletargets removed as we don't ship those variantsDockerfilelogic!ENVdirective inDockerfilenow properly uses=(vs. whitespace)buildinputsto pull file paths from the terminal layergen_gha_matrix_jobs.pynow uses anall-imagesMakefiletarget to identify image-related targets (while also supported theMakevariables that appear in ourtargetnames now)gha_pr_changed_files.pyadapted to:#\(MACHINE-PARSED LINE\)#\inmakeoutputDockerfile(with differing extensions) existing in a single directoryDockerfile.konfluxfilter_out_unchangedthat I would prefer to treat in a subsequent PRtest_make_building_only_specified_imagesfunction removed frommake_test.pyas its irrelevant now when not using chained buildshas_tests.pyupdated to:#\(MACHINE-PARSED LINE\)#\in its outputMakefiletargetwhen dealing withgha_pr_changed_filesMakefileis parsed now - we need to make sure we are passing a validMakefiletargetinto the test caseRelated-to: https://issues.redhat.com/browse/RHOAIENG-19048
How Has This Been Tested?
mainbranchmainbranch as seen here:Merge criteria: