Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8528049:
Introduce the constrained network server. (Closed)
|
Created:
9 years, 1 month ago by DaleCurtis Modified:
9 years, 1 month ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, acolwell+watch_chromium.org, annacc+watch_chromium.org, pam+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org, shadi Visibility:
Public. |
DescriptionIntroduce the constrained network server.
The CNS allows files to be served under constrained network conditions
per the design doc listed in the bug report.
Uses CherryPy to handle HTTP processing. See ./cns.py --help, to run:
./cns.py [--port <port>] [--port-range <port-range>]
Requests can then be made to:
http://<server ip>:<port>/ServeConstrained?f=<file>&latency=...&bandwidth=...&loss=...
The server will allocate a port from the preconfigured range and setup
constraints on that port. Subsequent requests for the same constraints
from the same source ip will result in the constrained port being
reused.
BUG=104242
TEST=Ran locally. Ran unittests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110458
Patch Set 1 #
Total comments: 32
Patch Set 2 : Fixed code review issues. Added unit tests. #
Total comments: 2
Patch Set 3 : Fixed nits. #
Total comments: 8
Patch Set 4 : Fixed code review issues. #
Messages
Total messages: 13 (0 generated)
Early review. No unit tests yet and Shadi is still finishing his scripts. Everything else is complete though, the only parts that will change are the Port.Setup() and Port.Delete() calls in Get() and Cleanup() respectively.
Mostly nits. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:28: _DEFAULT_SERVING_PORT = 80 Does this script require running as root? (IWBN not to) http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:115: expired = time.time() - status['last_update'] > self._expiry_time_secs Usually calling time.time() once per function (and assigning to a local "now" var) makes your life better in the long run as you can reason about the behavior of the function when looking at a dump of the current state, instead of having to dump state on each iteration. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:125: self._port_lock.release() In English this stanza looks wrong (if not locked, how can we release the lock?). There's a code smell in the current impl in that locking is a class-internal concept, so outside callers shouldn't need to know about locking, so they shouldn't be able to specify the locked param. Two ways to avoid this are: - having a public Cleanup & a private _Cleanup_Locked: def Cleanup(self, all_ports=False) with self._port_lock: self._Cleanup_Locked(all_ports) def _Cleanup_Locked(self, all_ports=False) ... and then have locked=true callers call self._Cleanup_Locked instead of Cleanup() - using threading.RLock (reentrant) and always acquire/release'ing (with) in Cleanup. Personally I think the first way is better (reentrancy is generally far more subtle than you care to think through). http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:154: latency: time to add to each request (integer in ms). Latency in my mind was always the ms to add to each TCP packet, not to the overall request. Have we been miscommunicating or is this just a poorly chosen phrasing? http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:163: sanitized_path = os.path.abspath(os.path.join(self._options.www_root, file)) Personally I'd overwrite |file| at this point, to make sure future lines only use the sanitized version. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:214: ' and are cleaned up. Default: %default')) s/' /'/ http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:219: ' a comma separated value pair. Default: %default')) s/' /'/ http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:220: parser.add_option('--local', action='store_true', default=False, Unused? http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:223: help='Number of threads in the thread pool.') doco omission semantics. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:225: help=('Port to serve the API on. Defaults to the current ' s/Port to serve the API on./Directory root of servable files./ http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:259: pa.Cleanup(all_ports=True) Does this run in case of ctrl-c? Or in what cases does it run?
http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:28: _DEFAULT_SERVING_PORT = 80 Technically this script doesn't. However shadi's tc scripts will need to be root. I can think of a few options: - Start the CNS as root. - Start the CNS as a user w/ password-less sudo. - Make a suid wrapper for the TC scripts. Any others? On 2011/11/15 17:54:07, Ami Fischman wrote: > Does this script require running as root? > (IWBN not to) http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:115: expired = time.time() - status['last_update'] > self._expiry_time_secs Done. You make a good case. On 2011/11/15 17:54:07, Ami Fischman wrote: > Usually calling time.time() once per function (and assigning to a local "now" > var) makes your life better in the long run as you can reason about the behavior > of the function when looking at a dump of the current state, instead of having > to dump state on each iteration. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:125: self._port_lock.release() Done. Agreed on the first solution. On 2011/11/15 17:54:07, Ami Fischman wrote: > In English this stanza looks wrong (if not locked, how can we release the > lock?). > There's a code smell in the current impl in that locking is a class-internal > concept, so outside callers shouldn't need to know about locking, so they > shouldn't be able to specify the locked param. > > Two ways to avoid this are: > - having a public Cleanup & a private _Cleanup_Locked: > def Cleanup(self, all_ports=False) > with self._port_lock: > self._Cleanup_Locked(all_ports) > def _Cleanup_Locked(self, all_ports=False) > ... > and then have locked=true callers call self._Cleanup_Locked instead of Cleanup() > > - using threading.RLock (reentrant) and always acquire/release'ing (with) in > Cleanup. > > Personally I think the first way is better (reentrancy is generally far more > subtle than you care to think through). http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:154: latency: time to add to each request (integer in ms). Poor phrasing on my part. I'll clean it up a bit. Latency is controlled via TC, so to be specific this is what it's doing: http://www.linuxfoundation.org/collaborate/workgroups/networking/netem#Emulat... On 2011/11/15 17:54:07, Ami Fischman wrote: > Latency in my mind was always the ms to add to each TCP packet, not to the > overall request. Have we been miscommunicating or is this just a poorly chosen > phrasing? http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:163: sanitized_path = os.path.abspath(os.path.join(self._options.www_root, file)) On 2011/11/15 17:54:07, Ami Fischman wrote: > Personally I'd overwrite |file| at this point, to make sure future lines only > use the sanitized version. Done. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:214: ' and are cleaned up. Default: %default')) On 2011/11/15 17:54:07, Ami Fischman wrote: > s/' /'/ Done. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:219: ' a comma separated value pair. Default: %default')) On 2011/11/15 17:54:07, Ami Fischman wrote: > s/' /'/ Done. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:220: parser.add_option('--local', action='store_true', default=False, Currently, yes. I need to talk to shadi to work this out properly. The tc commands for local vs eth are different. With a fresh mind this morning, it might actually be better to name this --interface and accept eth# or lo. I can then lookup the IP for each interface and set cherrypy to only bind there. On 2011/11/15 17:54:07, Ami Fischman wrote: > Unused? http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:223: help='Number of threads in the thread pool.') Done. I'll forward the default value from: cherrypy._cpserver.Server.thread_pool On 2011/11/15 17:54:07, Ami Fischman wrote: > doco omission semantics. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:225: help=('Port to serve the API on. Defaults to the current ' On 2011/11/15 17:54:07, Ami Fischman wrote: > s/Port to serve the API on./Directory root of servable files./ Done. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:259: pa.Cleanup(all_ports=True) Yes, it will always run. Although it doesn't disable Ctrl-C so if you were to hit it multiple times it will abort the cleanup call. It's a bit messy, but I can trap Ctrl-C during this phase if necessary. On 2011/11/15 17:54:07, Ami Fischman wrote: > Does this run in case of ctrl-c? Or in what cases does it run?
Taking myself off reviewers list since I don't know Python!
PTAL. All issues addressed. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:163: sanitized_path = os.path.abspath(os.path.join(self._options.www_root, file)) Actually can't do this because the relative/unsanitized path needs to be passed back to the redirect link. On 2011/11/15 18:46:00, DaleCurtis wrote: > On 2011/11/15 17:54:07, Ami Fischman wrote: > > Personally I'd overwrite |file| at this point, to make sure future lines only > > use the sanitized version. > > Done.
I am in no way a python guy so I have a few tiny nits aside from fischman's http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:67: **kwargs: Constraints to pass into ***TODO*** ConstraintGenerator... TODO(dalecurtis)? http://codereview.chromium.org/8528049/diff/6001/media/tools/constrained_netw... File media/tools/constrained_network_server/cns_test.py (right): http://codereview.chromium.org/8528049/diff/6001/media/tools/constrained_netw... media/tools/constrained_network_server/cns_test.py:103: return process.pid what's the worst case scenario if it's actually not ready by this time or the process quits early for whatever reason? (bad arg, etc...)
http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:67: **kwargs: Constraints to pass into ***TODO*** ConstraintGenerator... On 2011/11/16 02:25:42, scherkus wrote: > TODO(dalecurtis)? Done. http://codereview.chromium.org/8528049/diff/6001/media/tools/constrained_netw... File media/tools/constrained_network_server/cns_test.py (right): http://codereview.chromium.org/8528049/diff/6001/media/tools/constrained_netw... media/tools/constrained_network_server/cns_test.py:103: return process.pid Worst case is it fails to start up and tests fail, it'll still get killed in the tearDown() call though. On 2011/11/16 02:25:42, scherkus wrote: > what's the worst case scenario if it's actually not ready by this time or the > process quits early for whatever reason? (bad arg, etc...)
http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:28: _DEFAULT_SERVING_PORT = 80 On 2011/11/15 18:46:00, DaleCurtis wrote: > Technically this script doesn't. However shadi's tc scripts will need to be > root. I can think of a few options: > > - Start the CNS as root. > - Start the CNS as a user w/ password-less sudo. > - Make a suid wrapper for the TC scripts. > > Any others? > > On 2011/11/15 17:54:07, Ami Fischman wrote: > > Does this script require running as root? > > (IWBN not to) > I think suid for tc scripts makes most sense. So defaulting this port to something >1024 would be nice (AFAICT this is the only thing requiring root by default in this script). http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:214: ' and are cleaned up. Default: %default')) On 2011/11/15 18:46:00, DaleCurtis wrote: > On 2011/11/15 17:54:07, Ami Fischman wrote: > > s/' /'/ > > Done. Actually you did the opposite of what I suggested. IMO it's better to leave the inter-word space on the previous line because it makes indentation look better (rather than having a dangling apostrophe). (here and below) Matter of taste. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:259: pa.Cleanup(all_ports=True) On 2011/11/15 18:46:00, DaleCurtis wrote: > Yes, it will always run. Although it doesn't disable Ctrl-C so if you were to > hit it multiple times it will abort the cleanup call. It's a bit messy, but I > can trap Ctrl-C during this phase if necessary. > > On 2011/11/15 17:54:07, Ami Fischman wrote: > > Does this run in case of ctrl-c? Or in what cases does it run? > I don't think that's needed; a ctrl-c that aborted cleanup would also exit the process, at which point there's no need for more cleanup (unless cherrypy daemonizes a fork?) http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... media/tools/constrained_network_server/cns.py:227: parser.add_option('--eth', default='eth0', s/--eth/--interface/? http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... File media/tools/constrained_network_server/cns_test.py (right): http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... media/tools/constrained_network_server/cns_test.py:25: _EXPIRY_TIME = 2 Ouch to real time and sleeps. Inject a clock into PortAllocator instead? http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... media/tools/constrained_network_server/cns_test.py:102: time.sleep(self._SERVER_START_SLEEP_SECS) Ouch. Can you instead emit a stdout from cns saying server is up when it's ready and wait for that to show up in subprocess here? sleep==donotwant http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... media/tools/constrained_network_server/cns_test.py:137: # Verify the request took longer than the requested latency. Would be a more convincing test if the un-latency'd test asserted it took *less* than _L_T_S :)
http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:28: _DEFAULT_SERVING_PORT = 80 Done. Used 9000 since layout tests run on 8000. On 2011/11/16 03:11:22, Ami Fischman wrote: > On 2011/11/15 18:46:00, DaleCurtis wrote: > > Technically this script doesn't. However shadi's tc scripts will need to be > > root. I can think of a few options: > > > > - Start the CNS as root. > > - Start the CNS as a user w/ password-less sudo. > > - Make a suid wrapper for the TC scripts. > > > > Any others? > > > > On 2011/11/15 17:54:07, Ami Fischman wrote: > > > Does this script require running as root? > > > (IWBN not to) > > > > I think suid for tc scripts makes most sense. > So defaulting this port to something >1024 would be nice (AFAICT this is the > only thing requiring root by default in this script). http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:214: ' and are cleaned up. Default: %default')) Ah, I thought you were just pointing out my accidental double spacing, not a style nit. Done. On 2011/11/16 03:11:22, Ami Fischman wrote: > On 2011/11/15 18:46:00, DaleCurtis wrote: > > On 2011/11/15 17:54:07, Ami Fischman wrote: > > > s/' /'/ > > > > Done. > > Actually you did the opposite of what I suggested. > IMO it's better to leave the inter-word space on the previous line because it > makes indentation look better (rather than having a dangling apostrophe). (here > and below) > Matter of taste. http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:259: pa.Cleanup(all_ports=True) Well if cleanup gets aborted the interface will be left in an unknown state due to tc/iptables rules. If that's okay I can remove the code I added to trap Ctrl-C. Cherrypy doesn't fork or daemonize. On 2011/11/16 03:11:22, Ami Fischman wrote: > On 2011/11/15 18:46:00, DaleCurtis wrote: > > Yes, it will always run. Although it doesn't disable Ctrl-C so if you were to > > hit it multiple times it will abort the cleanup call. It's a bit messy, but I > > can trap Ctrl-C during this phase if necessary. > > > > On 2011/11/15 17:54:07, Ami Fischman wrote: > > > Does this run in case of ctrl-c? Or in what cases does it run? > > > > I don't think that's needed; a ctrl-c that aborted cleanup would also exit the > process, at which point there's no need for more cleanup (unless cherrypy > daemonizes a fork?) http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... media/tools/constrained_network_server/cns.py:227: parser.add_option('--eth', default='eth0', On 2011/11/16 03:11:22, Ami Fischman wrote: > s/--eth/--interface/? Done. http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... File media/tools/constrained_network_server/cns_test.py (right): http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... media/tools/constrained_network_server/cns_test.py:25: _EXPIRY_TIME = 2 On 2011/11/16 03:11:22, Ami Fischman wrote: > Ouch to real time and sleeps. > Inject a clock into PortAllocator instead? Done. http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... media/tools/constrained_network_server/cns_test.py:102: time.sleep(self._SERVER_START_SLEEP_SECS) On 2011/11/16 03:11:22, Ami Fischman wrote: > Ouch. Can you instead emit a stdout from cns saying server is up when it's > ready and wait for that to show up in subprocess here? > > sleep==donotwant Done. http://codereview.chromium.org/8528049/diff/3002/media/tools/constrained_netw... media/tools/constrained_network_server/cns_test.py:137: # Verify the request took longer than the requested latency. On 2011/11/16 03:11:22, Ami Fischman wrote: > Would be a more convincing test if the un-latency'd test asserted it took *less* > than _L_T_S :) Done.
LGTM++ http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... File media/tools/constrained_network_server/cns.py (right): http://codereview.chromium.org/8528049/diff/1/media/tools/constrained_network... media/tools/constrained_network_server/cns.py:259: pa.Cleanup(all_ports=True) > > I don't think that's needed; a ctrl-c that aborted cleanup would also exit the > > process, at which point there's no need for more cleanup (unless cherrypy > > daemonizes a fork?) That's a good point - we should not let tc/iptables config bleed out after this script exits.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/8528049/9001
Change committed as 110458 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
