singularityhub / sregistry

server for storage and management of singularity images
https://singularityhub.github.io/sregistry
Mozilla Public License 2.0
103 stars 42 forks source link

API: add endpoint POST /v1/containers #349

Open pini-gh opened 3 years ago

pini-gh commented 3 years ago

Hi @vsoch,

Here is another PR to add POST /v1/containers. With this new endpoint we can fix GET /v1/containers/<entity>/<collection>/<container> so that it returns 404 when the container doesn't exist.

Tested against singularity client pushing to a non-existing container:

GET NamedEntityView
<QueryDict: {}>
pini
[pid: 17|app: 0|req: 12/54] 172.18.0.4 () {44 vars in 614 bytes} [Sun Feb 21 17:13:08 2021] GET /v1/entities/pini => generated 325 bytes in 68 msecs (HTTP/1.1 200) 5 headers in 142 bytes (1 switches on core 3)
GET GetNamedCollectionView
[pid: 13|app: 0|req: 7/55] 172.18.0.4 () {44 vars in 634 bytes} [Sun Feb 21 17:13:08 2021] GET /v1/collections/pini/macollection => generated 340 bytes in 100 msecs (HTTP/1.1 200) 5 headers in 142 bytes (1 switches on
 core 3)
GET GetNamedContainerView
Not Found: /v1/containers/pini/macollection/monconteneur1
[pid: 13|app: 0|req: 8/56] 172.18.0.4 () {44 vars in 660 bytes} [Sun Feb 21 17:13:08 2021] GET /v1/containers/pini/macollection/monconteneur1 => generated 72 bytes in 90 msecs (HTTP/1.1 404) 5 headers in 148 bytes (1 
switches on core 0)
POST ContainersView
[pid: 19|app: 0|req: 33/57] 172.18.0.4 () {46 vars in 638 bytes} [Sun Feb 21 17:13:08 2021] POST /v1/containers => generated 520 bytes in 153 msecs (HTTP/1.1 200) 5 headers in 148 bytes (1 switches on core 0)
GET PushNamedContainerView
[pid: 19|app: 0|req: 34/58] 172.18.0.4 () {44 vars in 817 bytes} [Sun Feb 21 17:13:08 2021] GET /v1/images/pini/macollection/monconteneur1:sha256.d97df3b0717cbace9add502b2634e0126d191037583444fc58b9855f9aa22d86?arch=a
md64 => generated 555 bytes in 198 msecs (HTTP/1.1 200) 5 headers in 142 bytes (1 switches on core 3)
[pid: 19|app: 0|req: 35/59] 172.18.0.4 () {44 vars in 596 bytes} [Sun Feb 21 17:13:09 2021] GET /version => generated 58 bytes in 11 msecs (HTTP/1.1 200) 5 headers in 141 bytes (1 switches on core 1)
POST RequestPushImageFileView
[pid: 19|app: 0|req: 36/60] 172.18.0.4 () {46 vars in 640 bytes} [Sun Feb 21 17:13:09 2021] POST /v2/imagefile/12 => generated 408 bytes in 104 msecs (HTTP/1.1 200) 5 headers in 137 bytes (1 switches on core 2)
PUT CompletePushImageFileView
DUMMY-7ee42129-258b-4c0f-996c-ff2a8d287570

We see above that the client first checks for the container existence, then creates it if need be. While our new endpoint just mimics the container creation, this workflow matches the Sylabs library's one better, and avoids wrong answers on GET /v1/containers/<entity>/<collection>/<container>.

vsoch commented 3 years ago

Thanks @pini-gh ! I again won't be able to review this during work hours, but in the meantime, could you point me at the Sylabs client code where this is done? And explain why the existing endpoint is not sufficient?

pini-gh commented 3 years ago

could you point me at the Sylabs client code where this is done?

https://github.com/sylabs/scs-library-client/blob/82d22e604229c73a27d339106ebcb2c6e137e03f/client/push.go#L159-L172

And explain why the existing endpoint is not sufficient?

We have a bunch of scripts (bash + curl) which use this endpoint already with the Sylabs library. I'd prefer not to have to adapt them when switching to Singularity Registry Server. Why do we need this endpoint with Sylabs library? Because we have sometimes to push private images, and if the container doesn't exist, it is created as public by default by the singularity client. So we need to create it first hand using this endpoint and adding "private": true to the json payload (this would be ignored by our endpoint; but it's OK because privacy is set at the collection level for Singularity Registry Server).

pini-gh commented 3 years ago

This is a use case for the Sylabs library actually. And I need this endpoint to be enabled on sregistry as well so I can use the very same scripts whatever my registry is (Sylabs library or sregistry).

vsoch commented 3 years ago

Let's step back here for a second. You're asking to add an endpoint to Singularity Registry Server that doesn't actually work as expected - and only so you don't have to modify your current scripts. I'm not sure this is a reasonable request, and I'd suggest that we step back and look at the information that you need, which seems to be data for a collection. My suggestion is to add a try/except to your script, that on a 404 (or other) response to create the container, you instead do a request to get metadata about the collection (which seems to be what you are looking for). I'm happy to help you think through this.

pini-gh commented 3 years ago

Well. This is not that simple. This endpoint:

As for my use case, let me reiterate: When I have to push a private image to a non-existing container I go through these steps with Sylabs library:

  1. Create the container via the API endpoint with private set to true.
  2. Push the image

If I don't create the image first, it would be created during the push with private set to false. Whatever the collection's private flag is.

If I want my scripts to work the same way with sregistry I need this endpoint. Or I'll end up having different snippets depending on which implementation the registry uses, which is not a good thing imho (reminds me web pages using different code blocks depending on the browser).

pini-gh commented 3 years ago

The whole point is either way the sregistry data model is not compatible with the Sylabs API regarding container objects.

You workaround this returning a false container object on calls to GET /v1/containers/<entity>/<collection>/<container> when the container doesn't exist. I workaround it returning a fake new container object on calls to POST /v1/containers.

In my opinion the latter is more consistent than the former, because it involves the very same workflow as with the Sylabs library when pushing a new container.

vsoch commented 3 years ago

My concern is that by adding this view, we are requiring an additional ping of a completely non-essential endpoint to perform the same action - one more request to the registry that serves no purpose. If we get a 200 response here we can skip over that entire extra request. It's different than what the Sylabs library does, but it's logical in the context of Singularity Registry server because the idea of creating a container does not exist. I see your point and I empathize with (and think it's reasonable) that you want your curl calls to be consistent, but I'm not convinced that adding this extra endpoint adds any value to the registry (but might actually hurt it).

pini-gh commented 3 years ago

but might actually hurt it

o_O

It brings a fix as well: the current GET /v1/containers/<entity>/<collection>/<container> should return 404 when looking for a non existing container.

vsoch commented 3 years ago

Yes, but it's not technically required - we can return 200 off the bat and then skip the extra request. To require the extra request and (still) return a 200 response (for a container that does not exist) is not a significant improvement.

pini-gh commented 3 years ago

Because you limit your use case to the interactions with the singularity client only.

When using the API from any other use case, answering 200 instead of 404 is an error.

vsoch commented 3 years ago

Ok, that's definitely fair - If you can get support from others in the community and show me other usage of the Sylabs API outside of the Singularity client, then that would be enough to convince me.

pini-gh commented 3 years ago

Oh well... Anyone?