API: add endpoint POST /v1/containers #349

pini-gh · 2021-02-22T08:28:53Z

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 · 2021-02-22T11:27:46Z

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 · 2021-02-22T11:57:39Z

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).

vsoch

I'm a little confused about the use case for this view, but the code and documentation looks good! Could you better explain to me why we would want an endpoint to create a container that doesn't actually create it?

vsoch · 2021-02-22T16:06:23Z

shub/apps/library/views/images.py

+    def post(self, request):
+        """Mimic the creation a new container.
+
+        POST /v1/coontainers


Suggested change

POST /v1/coontainers

POST /v1/containers

vsoch · 2021-02-22T16:07:28Z

shub/apps/library/views/images.py

+            return Response(message, status=400)
+
+        try:
+            collection_id = int(body["collection"])


For the other function we convert everything to string (the ids) so we should probably be consistent and use one or the other, but not both.

Here we need an integer to reuse later in:

collection = Collection.objects.get(id=collection_id)

In the other function, comparing strings instead of ints saves a try: / except: block.

yes, but if the collection doesn't exist, it will raise an error here, which you'd need to catch. .get assumes an existing collection.

vsoch · 2021-02-22T16:08:00Z

shub/apps/library/views/images.py

+
+        # check permissions
+        # return 403 when collection does not exist or user is not an owner
+        collection = Collection.objects.get(id=collection_id)


This is going to raise an error if it doesn't exist.

vsoch · 2021-02-22T16:09:13Z

shub/apps/library/views/images.py

+        ignore here because containers' privacy is inherited from the
+        collection they belong to.
+
+        Return the newly created container.


I don't quite understand this endpoint - you mention that we return the newly created container, but then you mention that we "mimic the Sylabs library without actually creating the container object." What's the point?

You're right. The comment should be fixed.

shub/apps/library/views/helpers.py

pini-gh · 2021-02-22T16:28:02Z

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 · 2021-02-22T16:44:30Z

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 · 2021-02-22T17:23:50Z

Well. This is not that simple. This endpoint:

fixes a broken sregistry endpoint: GET /v1/containers/<entity>/<collection>/<container> should return 404 when the container doesn't exist.
exists on the Sylabs library side; so implementing it should improves the compatibility with other singularity clients.
doesn't break anything.

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:

Create the container via the API endpoint with private set to true.
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 · 2021-02-22T18:17:06Z

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 · 2021-02-23T16:16:57Z

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 · 2021-02-23T17:26:50Z

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 · 2021-02-23T17:38:26Z

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 · 2021-02-23T17:49:58Z

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 · 2021-02-23T17:59:52Z

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 · 2021-02-23T20:16:25Z

Oh well... Anyone?

API: add endpoint POST /v1/containers

ebcc9cf

vsoch reviewed Feb 22, 2021

View reviewed changes

Fixes from PR conversations

e43ecf2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: add endpoint POST /v1/containers #349

API: add endpoint POST /v1/containers #349

pini-gh commented Feb 22, 2021 •

edited

Loading

vsoch commented Feb 22, 2021

pini-gh commented Feb 22, 2021 •

edited

Loading

vsoch left a comment

vsoch Feb 22, 2021

vsoch Feb 22, 2021

pini-gh Feb 22, 2021

vsoch Feb 22, 2021

pini-gh Feb 22, 2021

vsoch Feb 22, 2021

pini-gh Feb 22, 2021

vsoch Feb 22, 2021

pini-gh Feb 22, 2021

pini-gh Feb 22, 2021

pini-gh commented Feb 22, 2021

vsoch commented Feb 22, 2021

pini-gh commented Feb 22, 2021 •

edited

Loading

pini-gh commented Feb 22, 2021 •

edited

Loading

vsoch commented Feb 23, 2021

pini-gh commented Feb 23, 2021

vsoch commented Feb 23, 2021

pini-gh commented Feb 23, 2021

vsoch commented Feb 23, 2021

pini-gh commented Feb 23, 2021

API: add endpoint POST /v1/containers #349

Are you sure you want to change the base?

API: add endpoint POST /v1/containers #349

Conversation

pini-gh commented Feb 22, 2021 • edited Loading

vsoch commented Feb 22, 2021

pini-gh commented Feb 22, 2021 • edited Loading

vsoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pini-gh commented Feb 22, 2021

vsoch commented Feb 22, 2021

pini-gh commented Feb 22, 2021 • edited Loading

pini-gh commented Feb 22, 2021 • edited Loading

vsoch commented Feb 23, 2021

pini-gh commented Feb 23, 2021

vsoch commented Feb 23, 2021

pini-gh commented Feb 23, 2021

vsoch commented Feb 23, 2021

pini-gh commented Feb 23, 2021

pini-gh commented Feb 22, 2021 •

edited

Loading

pini-gh commented Feb 22, 2021 •

edited

Loading

pini-gh commented Feb 22, 2021 •

edited

Loading

pini-gh commented Feb 22, 2021 •

edited

Loading