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

Add support for COS public endpoint to Kubeflow Pipelines runtime config #2887

Merged
merged 6 commits into from
Aug 18, 2022

Conversation

portellaa
Copy link
Contributor

What changes were proposed in this pull request?

This aims to introduce a new property to configure the public endpoint for the Cloud Objecte Storage if necessary.

In some cases (ours for example) we run separate services with separate policies for access, so we use a private endpoint for the API (which normally writes or it's meant to be used by automated components) and a public endpoint only for read that is meant to be provided to the user.

This is the result of this change

Screenshot 2022-08-12 at 13 14 15

How was this pull request tested?

This change is very easy, no "new logic", well apart of creating a variable that tries to get the configured value from the runtime configuration an if doesn't exists uses the API endpoint that is mandatory.

Make sure that the pre change works well:

  • create a runtime configuration without public endpoint configured
  • run pipeline
  • click on the object storage link and open the private one

Make sure that the new public endpoint works

  • create a runtime configuration with public endpoint
    • run pipeline
  • click on the object storage link and open the public one

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.

This commit aims to introduce support for a public endpoint to a Cloud
Object Storage.

It adds a new property to the schema JSON to be shown in the Runtime
configuration section

During the pipeline creation, it try to gets that variable if defined
otherwise uses the api one.
@elyra-bot
Copy link

elyra-bot bot commented Aug 16, 2022

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link: Binder

@ptitzler ptitzler added kind:enhancement New feature or request component:pipeline-editor pipeline editor platform: pipeline-Kubeflow Related to usage of Kubeflow Pipelines as pipeline runtime status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. labels Aug 16, 2022
@ptitzler ptitzler self-requested a review August 16, 2022 11:41
@ptitzler
Copy link
Member

Conceptually this is looking good! I haven't tried the code yet but wanted to bring up a few things we need to add to close the loop:

  • Update the user documentation to list the new property (source which surfaces here)
  • Update the Runtimes metadata display to render the public object storage endpoint URL as a link instead of the non-public endpoint - see changes to the RuntimesWidget in this PR, which delivered similar functionality for the Kubeflow Pipelines endpoint.

I tagged this as a Kubeflow Pipelines feature for now. We can always extend this to Apache Airflow in a follow-up PR (or have somebody with access to an Airflow deployment augment your PR since the new functionality is not runtime specific).

docs/source/user_guide/runtime-conf.md Outdated Show resolved Hide resolved
docs/source/user_guide/runtime-conf.md Outdated Show resolved Hide resolved
elyra/pipeline/kfp/processor_kfp.py Outdated Show resolved Hide resolved
@ptitzler
Copy link
Member

Code changes look good and work as expected:

  • KFP RTC configured without public COS endpoint
    • tested editor and RTC list view
    • tested run pipeline and export pipeline
  • KFP RTC configured with public COS endpoint
    • tested editor and RTC list view
    • tested run pipeline and export pipeline
  • no impact on Airflow RTC

Last remaining item is to fix the broken integration test Pipeline Editor tests -> should save runtime configuration. This call fails because there are now two elements in the DOM with titles containing the string object storage endpoint.

@portellaa
Copy link
Contributor Author

Code changes look good and work as expected:

  • KFP RTC configured without public COS endpoint

    • tested editor and RTC list view
    • tested run pipeline and export pipeline
  • KFP RTC configured with public COS endpoint

    • tested editor and RTC list view
    • tested run pipeline and export pipeline
  • no impact on Airflow RTC

Last remaining item is to fix the broken integration test Pipeline Editor tests -> should save runtime configuration. This call fails because there are now two elements in the DOM with titles containing the string object storage endpoint.

Oh thanks. I was looking for that. fixing

@ptitzler ptitzler added this to the 3.11.0 milestone Aug 17, 2022
tests/support/commands.ts Outdated Show resolved Hide resolved
@ptitzler ptitzler changed the title Add support for COS public endpoint Add support for COS public endpoint to Kubeflow Pipelines runtime config Aug 18, 2022
@ptitzler ptitzler added component:metadata-editor Metadata Editor Extension and removed component:pipeline-editor pipeline editor labels Aug 18, 2022
@ptitzler
Copy link
Member

If you can make the final change by tomorrow we'll include the PR in the upcoming release, otherwise we might have to defer.

Co-authored-by: Patrick Titzler <ptitzler@us.ibm.com>
@ptitzler ptitzler removed the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Aug 18, 2022
Copy link
Member

@ptitzler ptitzler left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you for your contribution. Much appreciated!

@portellaa
Copy link
Contributor Author

@ptitzler thank you us for the project.

sorry busy morning, i tried to run the tests, with what you said, but they failed with the error:

Screenshot 2022-08-18 at 14 22 26

@akchinSTC
Copy link
Member

sorry busy morning, i tried to run the tests, with what you said, but they failed with the error:

@portellaa - Were there additional non committed changes you were testing against locally? The CI tests here seems to have passed without issue.

@ptitzler
Copy link
Member

When I run the cypress tests in my environment I occasionally see 'random' errors that are totally unrelated to the code changes. The same behavior can be observed in some of the official CI runs that fail only to succeed the next time without any changes in between. We've also confirmed through manual testing that the code works as expected. Unless there is a clear pattern I wouldn't worry about it.

@akchinSTC akchinSTC merged commit 06365cb into elyra-ai:main Aug 18, 2022
@portellaa
Copy link
Contributor Author

sorry busy morning, i tried to run the tests, with what you said, but they failed with the error:

@portellaa - Were there additional non committed changes you were testing against locally? The CI tests here seems to have passed without issue.

no no i didn't.

they fail at different steps, but probably some configuration or bad package on my conda env 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:metadata-editor Metadata Editor Extension kind:enhancement New feature or request platform: pipeline-Kubeflow Related to usage of Kubeflow Pipelines as pipeline runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants