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

Documentation on using OCI images for model storage (modelcars) #415

Merged

Conversation

israel-hdez
Copy link

What this PR does / why we need it:

Documentation on using OCI images for model storage (modelcars)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

https://issues.redhat.com/browse/RHOAIENG-13446

Checklist:

  • [N/A] Have you added unit/e2e tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

@openshift-ci openshift-ci bot requested review from Jooho and spolti October 3, 2024 16:21
@openshift-ci openshift-ci bot added the approved label Oct 3, 2024
Finally, notice that ownership of the copied model files is changed to the `root`
user and group, and also read permissions are granted to all users. This is
important, because OpenShift runs containers with a random user ID and with the
`root` group ID. The adjustment of the group and the privileges on the model files
Copy link
Member

@spolti spolti Oct 3, 2024

Choose a reason for hiding this comment

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

I am, not sure about it, avoiding using root is a good practice.
Did you try to add a dummy user and use this uid instead 0?

e.g.: COPY --chown=1001:0 models /models

Copy link
Author

Choose a reason for hiding this comment

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

My understanding is that the advice against using root is applicable for running processes. In that case, you definitely want to avoid running as root because it has security implications.

In contrast, here the models are simple data files and cannot be run. The chmod command in the Containerfile only gives read permissions and won't give write nor execute permissions. The model server (which is another container) would load the model and run it, and would be running as non-root user (provided the user configured the ServingRuntime correctly).

So, given the models are simple data files, I'm not seeing security implications with giving root ownership. It has the added benefit that the model files cannot be modified once mounted in the pod. So, I preferred to keep the Containerfile simple and not creating users, nor using dummy UIDs.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, my concern is that this image still has bash and it is up to the user to create it, so, avoiding the usage of root might be a good thing.

Copy link
Author

Choose a reason for hiding this comment

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

I think I'm not understanding the relationship between bash and the data files... And the base image has a lot of files owned by root. Probably, I'm not seeing something as I don't understand the problem.

Copy link
Member

Choose a reason for hiding this comment

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

It is more about good practices than a real problem.

Copy link

Choose a reason for hiding this comment

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

I agreed with @spolti

> [!IMPORTANT]
> Remember that, by default, models are exposed outside the cluster and not
> protected with authorization. Read the [authorization guide](authorization.md#deploying-a-protected-inferenceservice)
> and the [private services guide (TODO)](#TODO) to learn how to privately deploy
Copy link
Member

Choose a reason for hiding this comment

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

what is missing in this TODO?

Copy link
Author

Choose a reason for hiding this comment

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

We don't have ODH-specific docs for private InferenceServices that are focused on the API. I'm not planning to write those docs in this PR. Thus, I left this as a placeholder.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe is good to also create the jiras so we don't forgot about it.

docs/odh/oci-model-storage.md Outdated Show resolved Hide resolved
docs/odh/oci-model-storage.md Outdated Show resolved Hide resolved
docs/odh/oci-model-storage.md Show resolved Hide resolved
docs/odh/oci-model-storage.md Outdated Show resolved Hide resolved
docs/odh/oci-model-storage.md Show resolved Hide resolved
docs/odh/oci-model-storage.md Show resolved Hide resolved
docs/odh/oci-model-storage.md Show resolved Hide resolved
docs/odh/oci-model-storage.md Show resolved Hide resolved
@israel-hdez
Copy link
Author

@spolti @Jooho I fixed your feedback. Please, give it another look.

Copy link

openshift-ci bot commented Oct 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: israel-hdez, spolti

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Jooho
Copy link

Jooho commented Oct 18, 2024

I am ok with the change.
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 18, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit ac7ae34 into opendatahub-io:master Oct 18, 2024
19 checks passed
@israel-hdez israel-hdez deleted the j13446-modelcar-docs branch October 18, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants