RHOAIENG-437: Add instructions to fetch models from different s3 regions

Sara4994 commented 6 months ago

Description

This PR adds instructions to the credentials-s3.secret.template about the optional endpoint_url and region params. JIRA

To fetch models from S3 in a different region from the IAM credentials, user can consider endpoint_url param as an optional one and pass the correct region to the credentials secret. This way region will take precedence over endpoint_url and allows the task to download model from the specified region.

How Has This Been Tested?

Merge criteria:

[x] The commits are squashed in a cohesive manner and have meaningful messages.
[x] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
[x] The developer has manually tested the changes and verified that the changes work

openshift-ci-robot commented 6 months ago

@Sara4994: This pull request references RHOAIENG-437 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/216): > > >## Description > > >## How Has This Been Tested? > > > > >## Merge criteria: > > > >- [ ] The commits are squashed in a cohesive manner and have meaningful messages. >- [ ] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious). >- [ ] The developer has manually tested the changes and verified that the changes work > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.

MarianMacik commented 6 months ago

/retest

openshift-ci-robot commented 6 months ago

@Sara4994: This pull request references RHOAIENG-437 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/216): > > >## Description > >This PR adds instructions to the [credentials-s3.secret.template](https://github.com/opendatahub-io/ai-edge/blob/5960d4fc90c6f5aeb7c6a4eab5af5b1c2e972158/pipelines/tekton/aiedge-e2e/templates/credentials-s3.secret.yaml.template) about the optional endpoint_url and region params. [JIRA](https://issues.redhat.com/browse/RHOAIENG-437) > >To fetch models from S3 in a different region from the IAM credentials, user can consider endpoint_url param as an optional one and pass the correct region to the credentials secret. This way region will take precedence over endpoint_url and allows the task to download model from the specified region. > >## How Has This Been Tested? > > > > >## Merge criteria: > > > >- [ ] The commits are squashed in a cohesive manner and have meaningful messages. >- [ ] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious). >- [ ] The developer has manually tested the changes and verified that the changes work > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.

LaVLaS commented 6 months ago

Adding this for historical reference, the use of the s3.amazonaws.com global endpoint is now classified a Legacy Global Endpoint.

grdryn commented 6 months ago

Adding this for historical reference, the use of the s3.amazonaws.com global endpoint is now classified a Legacy Global Endpoint.

That's an interesting page, I hadn't seen it, thanks. I don't think we should mention that endpoint in our docs, since it will get confusing. If I understand that page correctly, that legacy endpoint will only work for some regions (those that existed before March 20th 2019).

Sara4994 commented 6 months ago

Adding this for historical reference, the use of the s3.amazonaws.com global endpoint is now classified a Legacy Global Endpoint.

That's an interesting page, I hadn't seen it, thanks. I don't think we should mention that endpoint in our docs, since it will get confusing. If I understand that page correctly, that legacy endpoint will only work for some regions (those that existed before March 20th 2019).

in that case this entire note wouldn't make sense? as we could just remove endpoint_url property from the template itself and just keep region alone?

grdryn commented 6 months ago

Adding this for historical reference, the use of the s3.amazonaws.com global endpoint is now classified a Legacy Global Endpoint.

That's an interesting page, I hadn't seen it, thanks. I don't think we should mention that endpoint in our docs, since it will get confusing. If I understand that page correctly, that legacy endpoint will only work for some regions (those that existed before March 20th 2019).

in that case this entire note wouldn't make sense? as we could just remove endpoint_url property from the template itself and just keep region alone?

For Amazon AWS S3, yes, we could remove the endpoint_url; but if someone wants to use some alternative service providing the S3 API (of which there are many, such as if a user has a Minio instance deployed to the cluster, and that's what they're using for model storage), then the endpoint_url needs to be specified.

openshift-ci-robot commented 6 months ago

@Sara4994: This pull request references RHOAIENG-437 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/216): > > >## Description > >This PR adds instructions to the [credentials-s3.secret.template](https://github.com/opendatahub-io/ai-edge/blob/5960d4fc90c6f5aeb7c6a4eab5af5b1c2e972158/pipelines/tekton/aiedge-e2e/templates/credentials-s3.secret.yaml.template) about the optional endpoint_url and region params. [JIRA](https://issues.redhat.com/browse/RHOAIENG-437) > >To fetch models from S3 in a different region from the IAM credentials, user can consider endpoint_url param as an optional one and pass the correct region to the credentials secret. This way region will take precedence over endpoint_url and allows the task to download model from the specified region. > >## How Has This Been Tested? > > > > >## Merge criteria: > > > >- [ ] The commits are squashed in a cohesive manner and have meaningful messages. >- [ ] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious). >- [ ] The developer has manually tested the changes and verified that the changes work > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.

Sara4994 commented 6 months ago

Adding this for historical reference, the use of the s3.amazonaws.com global endpoint is now classified a Legacy Global Endpoint.

That's an interesting page, I hadn't seen it, thanks. I don't think we should mention that endpoint in our docs, since it will get confusing. If I understand that page correctly, that legacy endpoint will only work for some regions (those that existed before March 20th 2019).

in that case this entire note wouldn't make sense? as we could just remove endpoint_url property from the template itself and just keep region alone?

For Amazon AWS S3, yes, we could remove the endpoint_url; but if someone wants to use some alternative service providing the S3 API (of which there are many, such as if a user has a Minio instance deployed to the cluster, and that's what they're using for model storage), then the endpoint_url needs to be specified.

@grdryn cool, i have just tweaked the note a bit.

openshift-ci-robot commented 6 months ago

@Sara4994: This pull request references RHOAIENG-437 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/216): > > >## Description > >This PR adds instructions to the [credentials-s3.secret.template](https://github.com/opendatahub-io/ai-edge/blob/5960d4fc90c6f5aeb7c6a4eab5af5b1c2e972158/pipelines/tekton/aiedge-e2e/templates/credentials-s3.secret.yaml.template) about the optional endpoint_url and region params. [JIRA](https://issues.redhat.com/browse/RHOAIENG-437) > >To fetch models from S3 in a different region from the IAM credentials, user can consider endpoint_url param as an optional one and pass the correct region to the credentials secret. This way region will take precedence over endpoint_url and allows the task to download model from the specified region. > >## How Has This Been Tested? > > > > >## Merge criteria: > > > >- [x] The commits are squashed in a cohesive manner and have meaningful messages. >- [ ] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious). >- [ ] The developer has manually tested the changes and verified that the changes work > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.

openshift-ci-robot commented 6 months ago

@Sara4994: This pull request references RHOAIENG-437 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/216): > > >## Description > >This PR adds instructions to the [credentials-s3.secret.template](https://github.com/opendatahub-io/ai-edge/blob/5960d4fc90c6f5aeb7c6a4eab5af5b1c2e972158/pipelines/tekton/aiedge-e2e/templates/credentials-s3.secret.yaml.template) about the optional endpoint_url and region params. [JIRA](https://issues.redhat.com/browse/RHOAIENG-437) > >To fetch models from S3 in a different region from the IAM credentials, user can consider endpoint_url param as an optional one and pass the correct region to the credentials secret. This way region will take precedence over endpoint_url and allows the task to download model from the specified region. > >## How Has This Been Tested? > > > > >## Merge criteria: > > > >- [x] The commits are squashed in a cohesive manner and have meaningful messages. >- [x] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious). >- [x] The developer has manually tested the changes and verified that the changes work > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.

grdryn commented 6 months ago

Thanks @Sara4994, I'm fine with this now. I'll leave it for a while to see if anyone else wants to review. Otherwise I'll approve later.

LaVLaS commented 6 months ago

/lgtm

Adding the 3rd and final LGTM

LaVLaS commented 6 months ago

/approve

Adding approval also based on the additional LGTMs above

openshift-ci[bot] commented 6 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LaVLaS, Sara4994

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/opendatahub-io/ai-edge/blob/main/OWNERS)~~ [LaVLaS] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

opendatahub-io / ai-edge