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

Remove system-owned runtime image configurations #3130

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ptitzler
Copy link
Member

@ptitzler ptitzler commented Mar 16, 2023

Elyra currently pre-installs a set of runtime image configurations. There are several issues:

  • these configurations cannot be removed by users
  • the referenced container images don't work on Red Hat Openshift
  • the referenced container images are maintained and hosted by third-parties and might be impacted by the recent Docker decision to sunset free team organizations

This PR removes these runtime configurations from the Elyra installation. Migration instructions will be included in the Elyra release notes. A draft version can be found here https://github.com/elyra-ai/elyra/discussions/3120. Note that documentation links in this document will yield a 404 until Elyra 3.15.0 was published.

Considerations:

  • No impact on existing pipeline files because the image name is persisted, not the runtime configuration name.
  • Existing installations of Elyra continue to include these runtime image configurations, even after Elyra was upgraded using pip install ... --upgrade. The removal only impacts new installations in a clean environment or upgrades that are performed by running pip uninstall ... followed by pip install ....
  • Container-based Elyra deployments can be extended to use the elyra-metadata install command to create the desired default runtime image configuration entries.
  • Custom builds of Elyra can still take advantage of system-owned runtime image configurations because this PR does not remove the relevant code. For example, a custom build might add one or more metadata files to the etc/config/metadata/runtime_images directory.

What changes were proposed in this pull request?

  • Remove runtime configurations
  • Add runtime configurations as test resources
  • Update tests to use test resources

How was this pull request tested?

  • Launch JupyterLab/Elyra and verify that nothing is broken
  • test-server and test-integration Makefile targets complete successfully

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Signed-off-by: Patrick Titzler <ptitzler@us.ibm.com>
@ptitzler ptitzler added this to the 3.15.0 milestone Mar 16, 2023
@ptitzler ptitzler added component:runtime-image Issues related to container images used to run generic components status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. labels Mar 16, 2023
@kevin-bates
Copy link
Member

  • Existing installations of Elyra continue to include these runtime image configurations, even after Elyra was upgraded. The removal only impacts new installations.

This is really a function of how the user chooses to "upgrade" the application. If they treat the upgrade as "uninstall/install", then the files in {sys.prefix}/share/jupyter/metdata/runtime-images will be removed during the uninstall. Using pip install --upgrade elyra will "preserve" the location.

  • Container-based Elyra deployments can be extended to use the elyra-metadata install command

We should probably document equivalent elyra-metadata install runtime-images commands for users to produce this. Also, with this approach, users would be able to modify these instances since they will not be considered "system-owned" - which is a good thing. 😄

Signed-off-by: Patrick Titzler <ptitzler@us.ibm.com>
@ptitzler
Copy link
Member Author

  • Existing installations of Elyra continue to include these runtime image configurations, even after Elyra was upgraded. The removal only impacts new installations.

This is really a function of how the user chooses to "upgrade" the application. If they treat the upgrade as "uninstall/install", then the files in {sys.prefix}/share/jupyter/metdata/runtime-images will be removed during the uninstall. Using pip install --upgrade elyra will "preserve" the location.

Great point! I've updated the PR description accordingly.

Signed-off-by: Patrick Titzler <ptitzler@us.ibm.com>
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

These changes look good. I think we should also remove the references to etc/metadata/runtime-images that are in pyproject.toml for completeness:

"etc/config/metadata/runtime-images" = "share/jupyter/metadata/runtime-images"

"etc/config/metadata/runtime-images/*.json",

Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

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

I don't think we should remove these runtime images as these will make the pipelines unusable out of the box.

What is the issue behind here?

  • Do we need to make these images based on UBI?
  • Do we need to create a elyra-base image for ISV's to use for embedding them in their system?

@ptitzler
Copy link
Member Author

I don't think we should remove these runtime images as these will make the pipelines unusable out of the box.

Pipelines are already unusable out of the box for KFP and Airflow because users need to create a runtime configuration first, which is a non-trivial task. That local executions currently require a container image to be as associated even though it is never used is a design issue, which should be fixable.

What is the issue behind here?

The issues are listed in the PR description. Mitigation options for users of container based deployments are documented in the linked release notes.

  • Do we need to make these images based on UBI?

As far as I know it was never our intention to be the provider and maintainer of runtime images. Users are of course free to continue using any image of their choice, even the ones we are referencing today. The currently referenced images may or may not be maintained (e.g. kept current) by their owners, which requires manual intervention by the users. It is therefore my belief that by not including runtime image configurations we are not introducing a new problem.

  • Do we need to create a elyra-base image for ISV's to use for embedding them in their system?

The existing base/example images we have (e.g. elyra and kf-notebook) are already basic blueprints.

Open Data Hub, which will become a major consumer of Elyra, is already creating their own set of standardized container images for OCP: https://github.com/opendatahub-io/notebooks. I don't think there is a need for the Elyra maintainers to provide additional images beyond what is already available.

@lresende
Copy link
Member

I don't think we should remove these runtime images as these will make the pipelines unusable out of the box.

Pipelines are already unusable out of the box for KFP and Airflow because users need to create a runtime configuration first, which is a non-trivial task. That local executions currently require a container image to be as associated even though it is never used is a design issue, which should be fixable.

Yes, we make this on purpose to make sure validation was considering the pipeline ready for real business which is run in a runtime which requires associated runtime-images.

As the elyra maintainers, we cannot guess what the configuration required to connect to a runtime deployed on the user environment, so unfortunately we cannot provide a sample runtime configuration. But we can guess some widely used runtime-images that can be useful for users and allow them to use elyra out of the box, even if they only want to run local pipelines.

The thing here is probably that we do this every day, so we know what and where to do it with our eyes closed. New users will be overwhelmed with the required steps to run a pipeline, our sample pipelines will not work as there is not going to be any runtime-image to use, etc and they will just give up.

@kevin-bates
Copy link
Member

I find the requirement of a docker image for all generic nodes knowing I want to run the pipeline locally to be my number one pet peeve about Elyra! I really wish this requirement could be relaxed until the time of submission when the actual runtime is determined. Or perhaps we could relax the requirement if Generic Pipeline is selected since those can only use generic components. We really need a runtime configuration meta-property like "require-images-on-generic-components", then, by promoting Local to its own runtime (here we are again!) we could make the proper enforcement. (This would also argue for some client-side validation to improve UX.)

If we could relax the runtime-image requirement on generic nodes so that its enforcement is a function of runtime selection, then I think this would allow a generic pipeline to be run out-of-the-box.


Regarding the removal of the system-owned runtime-image metadata instances, I think providing a table of parameters for CLI elyra-metadata install runtime-image commands for each of the previous system-owned images would be sufficient - especially given users will typically want to use their own images and many of these will likely cease to be available due to the Docker policy changes.

I think we're going to run into issues with these anyway once jupyter_core changes the default to prefer the platform directories (i.e., {sys.prefix}/share/jupyter/metadata/runtime-images) - which will occur in its 6.0 release but can be used now. When platform directories are preferred, I believe users will be able to update "system-owned" metadata instances in place since we rely on the ordering of directories - so these "system-owned" instances are a hack at best (and always have been since directory preference orders are already configurable).

@lresende
Copy link
Member

I find the requirement of a docker image for all generic nodes knowing I want to run the pipeline locally to be my number one pet peeve about Elyra! I really wish this requirement could be relaxed until the time of submission when the actual runtime is determined.

I am ok with this, the issue is that we have the "as you build" checks that mark nodes as red when they are not ready for submission, more like a compilation check before you run, to avoid only seeing 10 issues when you run and we will lose that.

If we treat the absence of the etc/config/metadata/runtime-images directory
as a valid condition it could be confusing to log a file not found
"error"

Signed-off-by: Patrick Titzler <ptitzler@us.ibm.com>
@ptitzler ptitzler removed this from the 3.15.0 milestone Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:runtime-image Issues related to container images used to run generic components status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants