On 07/16/2017 03:14 PM, Nadav Har'El wrote:
On Fri, Jul 14, 2017 at 12:02 PM, Justin Cinkelj
<[email protected] <mailto:[email protected]>> wrote:
Based on @myechuri work,
https://github.com/myechuri/osv-apps/commits/nginx-osv
<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?
Yes, the sigsuspend patch is required.
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?
The linux fork is commented out, and then the main thread is used to run
the only one worker thread. I think I can replace execve with osv_execve
(or something like that), but I haven't tried yet.
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?
100 is nginx default. I used very high limit for both keepalive_timeout
(to avoid problem with test longer than 75 sec - the default value is
75s) and keepalive_requests (as some test are also longer that 100 req
:)). I agree that restart process due to bugs cannot be applied on OSv
(even after nginx stops, leaked memory is not freed - we need to restart
whole VM to achieve that).
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]
<mailto:[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
<http://nginx.org/download/nginx-$%7BVERSION%7D.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?
I will try this! Hope it works, it would be much nicer.
+ 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]
<mailto:[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
<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]
<mailto:[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).
I will look into this. I got error, and tried first to just ignore error
due to ioctl(FIOASYNC).
+ }
+
++ // 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?
So a patch for OSv, to stub fcntl(fd, F_SETOWN, pid==0), with
descriptive WARN_ONCE message.
+
+ 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
<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?
I left myechuri logging to stderr in the conf file, but commented it
out. When nginx doesn't start (possible due to attempt to open log file
in nonexistent directory), it is useful to write errors to stderr. And
that is easier to achieve if default config has example lines.
+
+#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
<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://nginx.org/en/docs/ngx_core_module.html#worker_connections - it's
only 512.
Otherwise, that line is unmodified from nginx.tar.gz. As is most of
nginx.conf. Hm, I could submit nginx.conf as patch to file from .tar.gz,
but I guess that would be only more difficult to read. So I stayed with
nginx.conf as whole file.
+}
+
+
+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
<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.
I thought the keepalive_timeout is about connection "age". Have to look
at
https://tools.ietf.org/id/draft-thomson-hybi-http-timeout-01.html#rfc.section.2.1
('Keep-Alive: timeout" section) - and yes, it's about connection idle time.
So yes, default 75sec is more than enough.
+ 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 <http://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 <http://127.0.0.1:9000>
+ #
+ #location ~ \.php$ {
+ # root html;
+ # fastcgi_pass 127.0.0.1:9000 <http://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]
<mailto:osv-dev%[email protected]>.
For more options, visit https://groups.google.com/d/optout
<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.
