On Fri, Jul 14, 2017 at 12:02 PM, Justin Cinkelj <[email protected]> wrote:
> Based on @myechuri work, > https://github.com/myechuri/osv-apps/commits/nginx-osv Thanks (to both of you)! I want to commit this patch (we can always improve it later), but have one question first - does this patch depend on any of your recent (yet uncomitted) patches, like the sigsuspend stuff? I also have some more minor comments and questions below: > A single nginx worker thread is run, so only one CPU core is utilized. > What would it involve to have multiple worker threads? > > Example configuration includes very long keepalive_timeout/requests > I think you don't actually need a very long keepalive *timeout* - even 1 second should be more than enough (and anything more will obviously be fine) - if you only start a new connection once ever 1 second, in 2 minutes (the default segment life, if I remember correctly) you only pass through 120 connections. What you did need is a long keepalive *request limit* - say, 10,000. I think that in any case, regardless of any problem in OSv, it is a good idea to have a very large limit there. I still remember why this limit was added in Apache HTTPd: The thinking is that the server surely has bugs, and after you process a large number of requests in the same server process (it was processes, not threads, originally) is likely to have accrued bugs - especially memory leaks - so it's a good idea to stop the process and start a new one, once in a while, e.g., every 1000 requests. This rationale is no longer relevant in nginx (where we have just one thread anyway, which never stops even if we break the client's connection), and there is no rationale, as far as I can see, why it ever makes sense to break a connection with a client who is still constantly sending requests. And there's especially no reason to break the connection after as little as 100 requests. Is this "100" an nginx default or some random limit which myechuri used in his original config file? > setting to avoid possible problems with OSV and TCP client port reuse. > With example configuration will server offer two URLs: > http://$IP/index.html and > http://$IP/basic_status > > Signed-off-by: Justin Cinkelj <[email protected]> > --- > nginx/.gitignore | 2 + > nginx/Makefile | 45 +++++++ > nginx/module.py | 3 + > .../0001-nginx-OSv-fix-process-spawning.patch | 59 +++++++++ > nginx/patches/nginx.conf | 143 > +++++++++++++++++++++ > 5 files changed, 252 insertions(+) > create mode 100644 nginx/.gitignore > create mode 100644 nginx/Makefile > create mode 100644 nginx/module.py > create mode 100644 nginx/patches/0001-nginx-OSv- > fix-process-spawning.patch > create mode 100644 nginx/patches/nginx.conf > > diff --git a/nginx/.gitignore b/nginx/.gitignore > new file mode 100644 > index 0000000..cb8005e > --- /dev/null > +++ b/nginx/.gitignore > @@ -0,0 +1,2 @@ > +usr.manifest > +upstream.tar > diff --git a/nginx/Makefile b/nginx/Makefile > new file mode 100644 > index 0000000..be5fa9a > --- /dev/null > +++ b/nginx/Makefile > @@ -0,0 +1,45 @@ > +VERSION=1.12.1 > +SOURCE=http://nginx.org/download/nginx-${VERSION}.tar.gz > +CONFIGURE_MODULES=--prefix=/nginx/ --with-debug > --without-http_rewrite_module --with-threads --with-http_stub_status_module > + > +.PHONY: module clean > + > +SRC=upstream/nginx > + > +module: usr.manifest > + > +usr.manifest: $(SRC)/nginx.so > + echo '[manifest]' > usr.manifest > + echo '/nginx.so: $${MODULE_DIR}/$(SRC)/nginx.so' >> usr.manifest > + echo '/nginx/html/**: $${MODULE_DIR}/upstream/nginx/html/**' >> > usr.manifest > + echo '/nginx/logs/**: $${MODULE_DIR}/upstream/nginx/logs/**' >> > usr.manifest > + echo '/nginx/conf/**: $${MODULE_DIR}/upstream/nginx/conf/**' >> > usr.manifest > + echo '/nginx/conf/nginx.conf: $${MODULE_DIR}/patches/nginx.conf' > >> usr.manifest > + > +clean: > + rm -fr upstream > + rm -f upstream.tar usr.manifest > + > +# Note: the touch commands below are needed after commands which create > files > +# "in the past", like wget and tar, and can confuse Make to rebuild a > target > +# which is new, just pretends to be old. > +upstream.tar: > + wget -O $@ $(SOURCE) > + touch $@ > + > +$(SRC)/configure: upstream.tar > + mkdir upstream > + tar -C upstream -xf upstream.tar > + cd upstream; ln -sf nginx-${VERSION} nginx > + cd $(SRC); patch -p1 < ../../patches/0001-nginx-OSv- > fix-process-spawning.patch > + mkdir $(SRC)/logs; touch $(SRC)/logs/dummy-file > + touch $(SRC)/configure > + > +$(SRC)/Makefile: $(SRC)/configure > + cd $(SRC); ./configure $(CONFIGURE_MODULES) --with-cc-opt='-O2 > -D_FORTIFY_SOURCE=2 -fPIC' --with-ld-opt='-pie' > + > +$(SRC)/nginx.so: $(SRC)/Makefile > + make -j4 -C $(SRC) > I think (but haven't tried this in a while) that if you use $(MAKE) instake of "make" you won't need to use "-j4" because it will inherit the parent's paralellism? > + mv $(SRC)/objs/nginx $(SRC)/nginx.so > + > +.DELETE_ON_ERROR: > diff --git a/nginx/module.py b/nginx/module.py > new file mode 100644 > index 0000000..3a126f6 > --- /dev/null > +++ b/nginx/module.py > @@ -0,0 +1,3 @@ > +from osv.modules import api > + > +default = api.run('/nginx.so -c /nginx/conf/nginx.conf') > diff --git a/nginx/patches/0001-nginx-OSv-fix-process-spawning.patch > b/nginx/patches/0001-nginx-OSv-fix-process-spawning.patch > new file mode 100644 > index 0000000..7ef301a > --- /dev/null > +++ b/nginx/patches/0001-nginx-OSv-fix-process-spawning.patch > @@ -0,0 +1,59 @@ > +From 0b19a4133fb3afa580a9a8a822b13abfdc37b0e4 Mon Sep 17 00:00:00 2001 > +From: Justin Cinkelj <[email protected]> > +Date: Fri, 14 Jul 2017 10:07:53 +0200 > +Subject: [PATCH] nginx: OSv fix process spawning > + > +Original author @myechuri, > +https://github.com/myechuri/osv-apps/tree/nginx-osv > + > +Justin only commented out FIOASYNC part - it is not supported on OSv. > + > +Signed-off-by: Justin Cinkelj <[email protected]> > +--- > + src/os/unix/ngx_process.c | 14 ++++++++++---- > + 1 file changed, 10 insertions(+), 4 deletions(-) > + > +diff --git a/src/os/unix/ngx_process.c b/src/os/unix/ngx_process.c > +index 24a63fb..1128045 100644 > +--- a/src/os/unix/ngx_process.c > ++++ b/src/os/unix/ngx_process.c > +@@ -144,18 +144,22 @@ ngx_spawn_process(ngx_cycle_t *cycle, > ngx_spawn_proc_pt proc, void *data, > + > + on = 1; > + if (ioctl(ngx_processes[s].channel[0], FIOASYNC, &on) == -1) { > ++ // OSv: ignore error > + ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno, > +- "ioctl(FIOASYNC) failed while spawning > \"%s\"", name); > +- ngx_close_channel(ngx_processes[s].channel, cycle->log); > +- return NGX_INVALID_PID; > ++ "ioctl(FIOASYNC) failed while spawning \"%s\", > ignore on OSv", name); > ++ // ngx_close_channel(ngx_processes[s].channel, cycle->log); > ++ // return NGX_INVALID_PID; > I am not familiar with this code, but if I remember correctly FIOASYNC is about sending a *signal* when I/O is possible on this socket - some sort of archaic replacement for epoll. So I wonder how this stuff actually works *without* the right signals... Maybe the fact that this is in "ngx_process.c" means this is not part of the socket handling, but some sort of process support which we don't care about anyway? Or something? Would be nice to have a longer comment on why this patch is ok, and what we're giving up on (e.g., that it won't be fine if we had more than one process - or whatever). > + } > + > ++ // OSv: No need to set owner for the socket since pid 0 will be > running worker. > ++ /* > + if (fcntl(ngx_processes[s].channel[0], F_SETOWN, ngx_pid) == > -1) { > + ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno, > + "fcntl(F_SETOWN) failed while spawning > \"%s\"", name); > + ngx_close_channel(ngx_processes[s].channel, cycle->log); > + return NGX_INVALID_PID; > + } > ++ */ > Maybe we should implement F_SETOWN, ignoring a parameter of 0 (what we return for getpid()) and warning about any other parameter. However, I just noticed we have in sohasoutofband() the code which sends the SIGURG signal commented out, so F_SETOWN will not really work as expected, not for FIOASYNC's SIGIO, and not for SIGURG. Hmm.... Maybe worth printing a warning message but still succeeding, so you wouldn't need this patch? > + > + if (fcntl(ngx_processes[s].channel[0], F_SETFD, FD_CLOEXEC) == > -1) { > + ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno, > +@@ -183,7 +187,9 @@ ngx_spawn_process(ngx_cycle_t *cycle, > ngx_spawn_proc_pt proc, void *data, > + ngx_process_slot = s; > + > + > +- pid = fork(); > ++ // OSv: Fake master process as worker. > ++ // pid = fork(); > ++ pid = 0; > + > + switch (pid) { > + > +-- > +2.9.4 > + > diff --git a/nginx/patches/nginx.conf b/nginx/patches/nginx.conf > new file mode 100644 > index 0000000..44a984c > --- /dev/null > +++ b/nginx/patches/nginx.conf > @@ -0,0 +1,143 @@ > + > +#user nobody; > +worker_processes 1; > + > +# Set error_log to stderr so that log messages are displayed on > +# OSv console that started "scripts/run.py -nvd". > +# Although this is less ideal when compared to redirecting error > +# and access logs to syslog for example, it is a workable first > +# solution that is comparable to redirection used while starting > +# Nginx in a container > +# (reference: http://serverfault.com/questions/657863/nginx-how-to- > use-docker-log-collector-when-nginx-is-running-under-supervisord). > +#error_log stderr debug; > I think that this comment is outdated, because the line is commented out, and you have logging to files below? > + > +#pid logs/nginx.pid; > + > +# Run in foreground, primarily because fork() is stubbed in OSv. > +# This setting is consistent with official Nginx Dockerfile configuration: > +# https://github.com/nginxinc/docker-nginx/blob/ > 41aa13f7d2c24407e483c40fb1e8b33e73462ff1/mainline/jessie/Dockerfile#L27 > +daemon off; > + > +events { > + worker_connections 1024; > Good enough for tests (in your latency test, there is just one connection...) but isn't this much too small for actual deployments? Back in year 2000, we were already talking about C10K, i.e., 10,000 connections... What is nginx's normal default, if we don't set this? > +} > + > + > +http { > + include mime.types; > + default_type application/octet-stream; > + > + #log_format main '$remote_addr - $remote_user [$time_local] > "$request" ' > + # '$status $body_bytes_sent "$http_referer" ' > + # '"$http_user_agent" "$http_x_forwarded_for"'; > + > + # Default logging > + access_log logs/access.log combined; > + error_log logs/error.log error; > + # Send access and error logs to stderr for the time being. > + #access_log stderr; > + #error_log stderr debug; > + > + sendfile on; > + tcp_nopush on; > + > + # Default keepalive param values > + #keepalive_timeout 75; > + #keepalive_requests 100; > + # Long keepalive to avoid/reduce preblems with TCP port resue > + # See https://github.com/cloudius-systems/osv/issues/889 > + keepalive_timeout 75000; > As I commented above, I don't think such a high timeout is necessary - if 75 was nginx's default, I think it should be perfectly fine for us too. The main purpose of this timeout is to get rid of client connections which have been idle for a very long time and just wasting memory on our server. It doesn't affect your benchmarks, where a connection is never idle. + keepalive_requests 1000000000; > Well, there is something between 100 and 1000000000 :-) But I actually think that 1000000000 (or even infinity) is fine, as I explained above. So ok. > + > + #gzip on; > + > + server { > + listen 80; > + server_name localhost; > + > + # server_name 192.168.122.1; > + > + #charset koi8-r; > + > + #access_log logs/host.access.log main; > + > + location / { > + root html; > + index index.html index.htm; > + #aio threads; > + } > + > + location /basic_status { > + stub_status; > + } > + > + #error_page 404 /404.html; > + > + # redirect server error pages to the static page /50x.html > + # > + error_page 500 502 503 504 /50x.html; > + location = /50x.html { > + root html; > + } > + > + # proxy the PHP scripts to Apache listening on 127.0.0.1:80 > + # > + #location ~ \.php$ { > + # proxy_pass http://127.0.0.1; > + #} > + > + # pass the PHP scripts to FastCGI server listening on > 127.0.0.1:9000 > + # > + #location ~ \.php$ { > + # root html; > + # fastcgi_pass 127.0.0.1:9000; > + # fastcgi_index index.php; > + # fastcgi_param SCRIPT_FILENAME /scripts$fastcgi_script_name; > + # include fastcgi_params; > + #} > + > + # deny access to .htaccess files, if Apache's document root > + # concurs with nginx's one > + # > + #location ~ /\.ht { > + # deny all; > + #} > + } > + > + > + # another virtual host using mix of IP-, name-, and port-based > configuration > + # > + #server { > + # listen 8000; > + # listen somename:8080; > + # server_name somename alias another.alias; > + > + # location / { > + # root html; > + # index index.html index.htm; > + # } > + #} > + > + > + # HTTPS server > + # > + #server { > + # listen 443 ssl; > + # server_name localhost; > + > + # ssl_certificate cert.pem; > + # ssl_certificate_key cert.key; > + > + # ssl_session_cache shared:SSL:1m; > + # ssl_session_timeout 5m; > + > + # ssl_ciphers HIGH:!aNULL:!MD5; > + # ssl_prefer_server_ciphers on; > + > + # location / { > + # root html; > + # index index.html index.htm; > + # } > + #} > + > +} > -- > 2.9.4 > > -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
