learningequality / kolibri-server

A performance-boosting access layer for Kolibri with multi-core support and improved caching
5 stars 8 forks source link

Have nginx listen to default port 8080 #6

Closed benjaoming closed 5 years ago

benjaoming commented 5 years ago

For the sake of keeping user facing documentation etc. simple, we should stick to the normal port 8080. But this port is already occupied by CherryPy. So we would need a way to switch off CherryPy.

Secondly, we would want to pick up a user-defined port. It's not exactly known how we can read the user-defined port, but let's get a good interface for the kolibri environment to launch and detect its own port.

benjaoming commented 5 years ago

For instance: Running service kolibri port could be an interface to avoid kolibri-server using internals of the kolibri debian package or kolibri upstream sources? @jredrejo what do you think?

I imagine that kolibri service port would source /etc/default/kolibri and then invoke some yet-to-be-created kolibri get-config port command... or we just decide that it always explicitly define KOLIBRI_HTTP_PORT in /etc/default/kolibri and thus we don't care any further for it.

jredrejo commented 5 years ago

@benjaoming I see several problems here:

  1. If we use an nginx config file, this config must be the port written there, so it can't be a file provided by the package (well, postinst could hardcode it but then the user can't change it later).
  2. I think, according the previous conversations we have had, that get-config port should look only in kolibri options.ini file. Clearly, in that case kolibri-server must stop any previous running instance of kolibri that CherryPy might have started.

Summarizing: I think we should add this code to the kolibri-server init script:

  1. read the port from options.ini or set 8080 if there's nothing there
  2. write that port in the nginx configuration
  3. stop any running instance of kolibri
  4. reload nginx so it takes the new configuration

I don't like points 2 & 4 but I can't think of a better solution. Do you have any alternative?

benjaoming commented 5 years ago

If we use an nginx config file, this config must be the port written there, so it can't be a file provided by the package (well, postinst could hardcode it but then the user can't change it later).

Well... hmm... I think this pattern is already getting us there:

https://github.com/learningequality/kolibri-server/pull/7/files#diff-07e2cd25f7da0550504d107fb2cae5beR16

stop any running instance of kolibri

I agree with the steps, except that kolibri still has to run as a daemon and somehow be configured to run without CherryPy.

I'm arriving more and more at a point where I think that the package should control certain mandatory aspects of Kolibri through ENV_VARS set in /etc/default/kolibri and /etc/kolibri/conf.d. This way, the server should dictate Kolibri to 1) assume the user facing port used in the Nginx config and 2) tell Kolibri to run without CherryPy.

jredrejo commented 5 years ago

@benjaoming why does kolibri has to run as a daemon?, what is the problem in stopping it?

On the other hand, there's an alternative we have not touched yet as it could be odd as we are both upstream and the package maintainer , but the possibility is there: we can patch Kolibri code when building the package if the end result is not contradictory with the Kolibri user experience and documentation. I.E. if we need to patch Kolibri to read some envs that don't conflict with options.ini, that's fine. What we should avoid is having two different sources of truth, one for the people using the package and another for the rest of the users.

benjaoming commented 5 years ago

Why does kolibri has to run as a daemon?, what is the problem in stopping it?

Some process has to run to download those 100 GB of channel data :slightly_smiling_face: I can't see it being a UWSGI worker... we harakiri them and scale the number of workers up and down... this won't work if suddenly a worker is busy for 2 days, or another worker starts doing the same, because they run w/o shared states etc.

I never heard of anyone doing background tasks in their UWSGI workers... async work has always been handled outside of UWSGI.. celery, channels etc. In our case, we use our own daemon w/ Iceqube. That's why I want to make sure we don't consider UWSGI workers w/ each their Iceqube. At all.

benjaoming commented 5 years ago

Side note: I don't think we can patch the code from upstream kolibri from the kolibri-server package.. it doesn't package that code :)

jredrejo commented 5 years ago

Ok, this is my proposal

  1. For Kolibri < 0.12 (to be done in master branch)
  1. For kolibri >= 0.12 (to be done in develop branch)
    • kolibri-server will set nginx to listen on 8080 . The debconf template will allow the user to choose between 8080, 80 & 8008 (These are the only ports p2p discovery tests). After a better discovery is used it will allow the user to select any port.
    • Selected port will be written by debconf on options.ini when installing the package. After it, kolibri-server will read the port from options.ini everytime it starts and will update nginx configuration if needed.
    • The Kolibri application will run in the background (started using KOLIBRI_CHERRYPY_START=False kolibri start ) providing background services but not starting CherryPy, thus not opening any other port
indirectlylit commented 5 years ago

my understanding is that this issue has been addressed.

If not, please open new individual issues and target any critical short-term ones to the v0.1.0 milestone

benjaoming commented 5 years ago

Bad side effect: Device info page will show port 9999, any idea to workaround it?

@jredrejo is there an issue open for this?

indirectlylit commented 5 years ago

if it only affects versions prior to 0.12.x, my sense is that would be a 'wontfix' because we'd almost certainly want people to simply upgrade

jredrejo commented 5 years ago

@benjaoming @indirectlylit right, the wrong port in Device info page was solved in kolibri 0.12.0. Previously we had to keep two TCP ports open and Device info showed the wrong one. I'd say this is a fixed issue more than wonfix ;)