search

opencadc / science-platform

Science Platform Infrastructure
GNU Affero General Public License v3.0
14 stars 29 forks source link

SP-4539 - Multiple mounts and types #681

Closed anujachaitanya closed 1 week ago

anujachaitanya commented 1 month ago

As part of this pull request,

  1. we have introduced templates for extra volume configs in the launch files. Also have refactored volumes and volume mounts. We are as of now, targeting four type of volumes.

    • Persistent Volume
    • HostPath
    • ConfigMap
    • Secrets

    We can introduce some more types, If needed.

CC : @abhishekghoshhh @rob-tw

anujachaitanya commented 1 month ago

Typically, the Helm definitions in the session-volumes.yaml and session-volumes-mounts.yaml would all go into the _helpers.tpl file. Were the new files created to separate them?

Yes. Going on we may introduce different volume types, in those cases separate files would be easier to maintain.

anujachaitanya commented 1 month ago

@brianmajor Regarding example:

We have added examples for extraVolumes config in the sample-local-values.yaml. We are assuming user has already created the volumes and volumeMounts.

brianmajor commented 1 month ago

Thanks for the updates @anujachaitanya, looking better.

We still need to figure out how to reconcile this with PR #645 After thinking about this some more, I feel that the CVMFS PR is an important feature and it's attractive to have it all enabled with the boolean in the helm values. So, others please chime in here, but I'm thinking that we merge #645 first and then see what the conflicts look like in this PR. Thoughts?

rptaylor commented 1 month ago

@brianmajor I would lean towards supporting fully generalized volumes and volumeMounts, so that the same chart functionality can be used to support mounting any arbitrary volumes in any possible way. Then it would support both CVMFS volumes (in particular, both the hostPath and CSI-based approaches) and anything else too.

Personally in my charts I lean towards the most flexible approach, supporting arbitrary YAML (like I suggested here - simple code and complex values) but the appropriate approach depends on the perspective of the chart's users and developers. For end users of the charts it can be nicer to have easy options like e.g. enableCVMFS: true instead of laying out all the explicit YAML details of the required CVMFS volume, but this means the chart code requires more logic, i.e. simple values, complex code.

brianmajor commented 2 weeks ago

Thanks @anujachaitanya . One last review by @at88mph please then we can merge and close this.

anujachaitanya commented 2 weeks ago

Hello @brianmajor, As discussed yesterday. Have added the changes for SP-4265. In this PR. Please have a look at it.

brianmajor commented 2 weeks ago

Hi @anujachaitanya - I think I had misheard what Abhishek had said during our meeting, or maybe I didn't make myself clear. I didn't expect these last 2 commits (related to redis and harbor) to be combined with this PR. I don't know how hard it would be to separate them again, but it would make reviewing, testing, and releasing much easier for us if they were separate.

abhishekghoshhh commented 2 weeks ago

Hi @anujachaitanya - I think I had misheard what Abhishek had said during our meeting, or maybe I didn't make myself clear. I didn't expect these last 2 commits (related to redis and harbor) to be combined with this PR. I don't know how hard it would be to separate them again, but it would make reviewing, testing, and releasing much easier for us if they were separate.

Hi @brianmajor we might have misunderstood you in the last call. Actually we were thinking to send 2 PR separately but only yesterday we merged the redis changes. We'll revert the last merge.

abhishekghoshhh commented 2 weeks ago

I've taken a look at the recent additions related to caching the harbor image list and have made some inline comments. However, and I could be mistaken, I had thought that the intent of that work was to remove support for proprietary images in harbor, in which case I would have expected to see the removal of imagePullSecrets in the launch*.yaml files and the removal of Authorization tokens from the calls to the harbor API.

Hi @brianmajor. After discussing the acceptance criteria with Sharon, she clarified that this story is primarily focused on addressing Harbor's scalability issues. The handling of proprietary images will be dealt with separately at a later stage, which is why we haven't touched that part in this story. However, if it's necessary to remove the code related to proprietary images now, we can do that and push the changes. Could you please confirm whether we should proceed with removing that code?

anujachaitanya commented 2 weeks ago

Hi @anujachaitanya - I think I had misheard what Abhishek had said during our meeting, or maybe I didn't make myself clear. I didn't expect these last 2 commits (related to redis and harbor) to be combined with this PR. I don't know how hard it would be to separate them again, but it would make reviewing, testing, and releasing much easier for us if they were separate.

Hi @brianmajor we might have misunderstood you in the last call. Actually we were thinking to send 2 PR separately but only yesterday we merged the redis changes. We'll revert the last merge.

We have reverted the changes from redis and harbor. Please have a look at it.

brianmajor commented 2 weeks ago

Hi @anujachaitanya - I think I had misheard what Abhishek had said during our meeting, or maybe I didn't make myself clear. I didn't expect these last 2 commits (related to redis and harbor) to be combined with this PR. I don't know how hard it would be to separate them again, but it would make reviewing, testing, and releasing much easier for us if they were separate.

Hi @brianmajor we might have misunderstood you in the last call. Actually we were thinking to send 2 PR separately but only yesterday we merged the redis changes. We'll revert the last merge.

We have reverted the changes from redis and harbor. Please have a look at it.

Great, thank you, we will take another look at this PR. And yes, the redis/caching should be in a separate PR.

brianmajor commented 2 weeks ago

I've taken a look at the recent additions related to caching the harbor image list and have made some inline comments. However, and I could be mistaken, I had thought that the intent of that work was to remove support for proprietary images in harbor, in which case I would have expected to see the removal of imagePullSecrets in the launch*.yaml files and the removal of Authorization tokens from the calls to the harbor API.

Hi @brianmajor. After discussing the acceptance criteria with Sharon, she clarified that this story is primarily focused on addressing Harbor's scalability issues. The handling of proprietary images will be dealt with separately at a later stage, which is why we haven't touched that part in this story. However, if it's necessary to remove the code related to proprietary images now, we can do that and push the changes. Could you please confirm whether we should proceed with removing that code?

Hi @abhishekghoshhh @SharonGoliath - Actually, I don't see how caching could work without removing the proprietary access to harbor. Since each users' view of the images available for launch is specific to them (they may have read or write access to images that others don't have), the cache would have to be indexed by user, so not very useful. The current PR has a shared cache for all users, which would be the view of the public images in harbor. So, removing the function of proprietary access to harbor goes hand-in-hand with adding caching. There should be two areas in which the users' harbor credentials are removed:

  1. On API calls to harbor--don't use (or obtain) the idToken on those calls.
  2. On job launch--don't use (or obtain) the imagePullSecret (the 'cliSecret' in harbor).