Ensure all size limits are inclusive, and are internally consistent. #74

rfk · 2017-09-21T03:35:40Z

Fixes #73, adds explicit functional tests to assert https://bugzilla.mozilla.org/show_bug.cgi?id=1401707, and includes "max_request_bytes" in the configuration report even when batches are enabled.

@mhammond r? The changes here are mostly to test cases so if you could check whether they make sense from your perspective that'd be awesome.

rfk · 2017-09-21T04:54:28Z

syncstorage/tests/functional/test_old_storage.py

-        res = self.app.post_json(self.root + '/storage/col2', wbos)
-        res = res.json
-        self.assertEquals(len(res['success']), 4)
-        self.assertEquals(len(res['failed']), 1)


These tests just assert compatibility with the old sync1.1 API, which we no longer care about, so I opted to delete this test rather than fix it.

rfk · 2017-09-21T04:54:48Z

syncstorage/tests/functional/test_storage.py

@@ -780,15 +780,20 @@ def test_get_collection_ttl(self):
        self.assertEquals(len(res.json), 0)

    def test_batch(self):
-        # This can't be run against a live server


It now can be run against a live server, if it returns the proper config info \o/

rfk · 2017-09-21T04:56:25Z

syncstorage/views/__init__.py

        )
    limits = {}
    for name in LIMIT_NAMES:
        limits[name] = get_limit_config(request, name)
-    # This limit is hard-coded for now.
-    limits["max_record_payload_bytes"] = MAX_PAYLOAD_SIZE


I moved this under the control of get_limit_config so that it can be adjusted via config file, which is important for the memcached tests.

rfk · 2017-09-21T04:59:26Z

syncstorage/views/validators.py

@@ -303,12 +304,13 @@ def parse_multiple_bsos(request):
            logger.info(logmsg, userid, collection, id, msg, bso)
            continue

-        if count >= BATCH_MAX_COUNT:


This was accidentally an inclusive range, because enumerate was starting the count at 0.

To be clear, it's still an inclusive range now, it's just more explicitly inclusive no?

Correct, it's now more obvious that it's an inclusive range on purpose.

mhammond

👍

mhammond · 2017-09-21T05:15:08Z

syncstorage/tests/functional/test_storage.py

-        self.assertEquals(max_bytes, 1024 * 1024)
-        bsos = [{'id': str(i), 'payload': "X" * (210 * 1024)}
-                for i in range(5)]
+        # Uploading N+1 210kB items should produce one failure.


This test seems less valuable now the max_bytes constraint isn't a constant (and had me confused for a while). IIUC we are now starting with an arbitrary item_size, then calculating how many items we believe will fit given our max_bytes constraint and uploading one more than that limit? I'd be inclined to adjust the comment to make that clearer as it's not clear what "N" is, or why "210kB" is relevant given the existing comment.

(indeed, there seems other item_sizes which would be interesting - 1, max_bytes, max_bytes/2. max_bytes/2+-1 :) But yeah, I'm not sure that would actually add value...

You understand correctly; I'll adjust the comment to make it more obvious what this is about.

mhammond · 2017-09-21T08:35:52Z

syncstorage/tests/functional/test_storage.py

+        # Uploading N+1 210kB items should produce one failure.
+        item_size = (210 * 1024)
+        max_items = max_bytes / item_size
+        self.assertTrue(max_items * item_size < max_bytes)


this seems to just be checking |(max_bytes / item_size) * item_size < max_bytes| (and similarly below)

Yes, the idea was to check that item_size is not an exact multiple of max_bytes but I don't think that's actually necessary, I'll remove it.

mhammond · 2017-09-21T08:52:40Z

syncstorage/views/util.py

 DEFAULT_LIMITS["max_total_records"] = 100 * DEFAULT_LIMITS["max_post_records"]
 DEFAULT_LIMITS["max_total_bytes"] = 100 * DEFAULT_LIMITS["max_post_bytes"]

+# In production, the request-size limit is actually controlled by nginx.


I got confused in IRC about this and still am :) Is nginx's size only talking about payloads too? (Note I've no reason to believe this is wrong, I'm just confused :)

I'm pretty sure nginx's size is concerned with total request size, and not payloads.

Correct, nginx will basically reject any request with content-length over its configured limit.

thomcc

Looks fine. A couple nits/concerns but the code itself is fine.

thomcc · 2017-09-21T14:02:20Z

syncstorage/views/validators.py

@@ -303,12 +304,13 @@ def parse_multiple_bsos(request):
            logger.info(logmsg, userid, collection, id, msg, bso)
            continue

-        if count >= BATCH_MAX_COUNT:


To be clear, it's still an inclusive range now, it's just more explicitly inclusive no?

thomcc · 2017-09-21T14:03:04Z

syncstorage/views/util.py

 DEFAULT_LIMITS["max_total_records"] = 100 * DEFAULT_LIMITS["max_post_records"]
 DEFAULT_LIMITS["max_total_bytes"] = 100 * DEFAULT_LIMITS["max_post_bytes"]

+# In production, the request-size limit is actually controlled by nginx.


I'm pretty sure nginx's size is concerned with total request size, and not payloads.

thomcc · 2017-09-21T14:08:33Z

syncstorage/tests/functional/test_storage.py

+        res = self.app.post_json(endpoint, bsos)
+        self.assertEquals(res.json['failed']['toomany'], 'retry bso')
+
+        # `max_total_records` is an (inclusive) limit on the


I don't think desktop will say this. Maybe depend is the wrong word then? (Or we need a client fix...). Same for X-Weave-Total-Bytes below.

"We can only enforce it if..." is probably a more accurate phrasing. Of course iif desktop can send this it would be helpful :-)

thomcc · 2017-09-21T14:10:59Z

syncstorage/tests/functional/test_storage.py

        res = self.app.post_json(self.root + '/storage/col2', bsos)
        res = res.json
-        self.assertEquals(len(res['success']), 4)
+        self.assertEquals(len(res['success']), max_items)
        self.assertEquals(len(res['failed']), 1)


Arguably, everything should fail inside a batch if a single record fails... (Normally the client could just not commit the batch, but that's not an option for batch=1234&commit=true where the body has records, which for desktop – and probably the other clients – it almost always will). But that's certainly a discussion for a different time...

I guess if we respect all the config limits, there should be no failures, so it probably doesn't matter.

Yeah, I agree, and in fact I feel like it would be simpler all round if any bad item in a request just failed the whole request. But I think we kept it this way just so we didn't have to mess with existing client code, and particularly so we didn't have different behaviour between batch and non-batch cases.

rfk force-pushed the inclusive-max branch 2 times, most recently from de8b836 to aa7ec1b Compare September 21, 2017 04:50

rfk commented Sep 21, 2017

View reviewed changes

mhammond approved these changes Sep 21, 2017

View reviewed changes

thomcc approved these changes Sep 21, 2017

View reviewed changes

Ensure all size limits are inclusive, and are internally consistent.

13dfa8b

rfk force-pushed the inclusive-max branch from aa7ec1b to 13dfa8b Compare September 22, 2017 00:09

rfk merged commit a6d709d into master Sep 22, 2017

thomcc mentioned this pull request Sep 25, 2018

Enforce the limits returned by /info/configuration mozilla-services/syncstorage-rs#40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure all size limits are inclusive, and are internally consistent. #74

Ensure all size limits are inclusive, and are internally consistent. #74

rfk commented Sep 21, 2017

rfk Sep 21, 2017

rfk Sep 21, 2017

rfk Sep 21, 2017

rfk Sep 21, 2017

thomcc Sep 21, 2017

rfk Sep 21, 2017

mhammond left a comment

mhammond Sep 21, 2017

rfk Sep 21, 2017

mhammond Sep 21, 2017

rfk Sep 21, 2017

mhammond Sep 21, 2017

thomcc Sep 21, 2017

rfk Sep 21, 2017

thomcc left a comment

thomcc Sep 21, 2017

thomcc Sep 21, 2017

thomcc Sep 21, 2017

rfk Sep 21, 2017

thomcc Sep 21, 2017

rfk Sep 21, 2017

Ensure all size limits are inclusive, and are internally consistent. #74

Ensure all size limits are inclusive, and are internally consistent. #74

Conversation

rfk commented Sep 21, 2017

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

mhammond 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

thomcc 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