Fixes #25293 - Puma support for smart proxy #613

ik5 · 2018-10-14T11:47:28Z

Work in progress (still did not test it or wrote any tests for it), loading puma (or) webrick as an HTTP Server.

Started earlier today to work on it, most of it done, but not all :)

ohadlevy · 2018-10-14T11:56:30Z

lib/launcher.rb


+    RACK_SERVERS = {
+      :webrick => Rack::Handler::WEBrick,
+      :puma => Rack::Handler::Puma


won't this will require both gems to load? e.g. if you dont have puma gem, this line will break?

Does it hurt to load them both? If it don't let's ignore it? If it does, then let's definitely take care of this.

So it looks like webrick is always present, dependency of sinatra or something. But we should be able to install foreman-proxy without puma bundler group, so this needs to be taken care of.

ik5 · 2018-10-14T16:48:15Z

It does not work yet, and that's one of the reasons, also tests will be broken at this time, due to refactoring of the code. Also there are places still hard coded for webrick. That's why I placed it as wip 😁

…

On Sun, Oct 14, 2018, 14:56 Ohad Levy ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/launcher.rb <#613 (comment)> : > @@ -10,6 +10,15 @@ class Launcher attr_reader :settings + RACK_SERVERS = { + :webrick => Rack::Handler::WEBrick, + :puma => Rack::Handler::Puma won't this will require both gems to load? e.g. if you dont have puma gem, this line will break? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#613 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEdg0liZZ1W9dmek2OrocJwneTrrHYAks5ukyaIgaJpZM4XbFxk> .

lzap · 2018-10-15T10:08:14Z

lib/launcher.rb

      {
        :app => app,
-        :server => :webrick,
+        :server => Proxy::SETTINGS.http_server_type || :webrick,


This does not correspondend with the line above :server => Proxy::SETTINGS.http_server_type.to_sym, so either one or another. The settings stack handles the defaults, so I think the version from 58 is better.

lzap · 2018-10-15T10:10:40Z

bundler.d/puma.rb

@@ -0,0 +1 @@
+gem 'puma', '3.11.4'


This should be in group :puma so users can turn it on or off via bundler. In RPM deployments, we need to do packaging change and do subpackage so installing foreman-proxy-puma will actually grab the dependency. Also I'd probably add '~> 3.11' requirement, not sure about the gemspec syntax tho.

lzap · 2018-10-15T10:13:03Z

Nicely done, if it boots up and can be properly disabled without the puma dependency, let's ship it to hear from our users after 1.20 is branched. We can test it many times, but only our community will really tell us.

ik5 · 2018-10-15T12:47:04Z

there are basics puma, but it does not handle the actual execution for the route properly, and still on it, and when that works, I will have to do two things:

A. Fix tests
B. Add tests for Puma
C. Group Puma, and only if it exists load things that belongs to it.

config/settings.yml.example

iNecas · 2018-10-16T12:00:28Z

Also, ssl doesn't work at all yet

ik5 · 2018-10-16T12:03:34Z

Also, ssl doesn't work at all yet

I haven't tested it yet, because the actual routing crashes, and now working on that part.

lzap · 2018-10-16T12:24:57Z

bundler.d/puma.rb

+    gem 'puma', '3.11.4', :require => 'puma'
+  else
+    gem 'puma', :require => 'puma'
+  end


Holy Moly, we are lucky that 3.11 was the last version supporting Ruby 2.0. Latest is 3.12 which only supports Ruby 2.2+. Unfortunately, SCLing proxy is imminent.

Can you avoid open-ended version requirement on line 5? Packagers will require us to have a specific version, something like '~> 3.12' or something like that.

lzap · 2018-10-16T12:25:09Z

bundler.d/development.rb

  gem 'pry'
+  gem 'pry-rescue'
+  gem 'pry-stack_explorer'
+  gem 'pry-byebug'


These belong to bundler.d/Gemfile.local.rb

ack, I thought it can help debug on development, but np :)

It can, we tend to be little more strict in core, I'd probably follow this rule. Everyone use her own tools.

lzap · 2018-10-16T12:28:10Z

lib/puma_patch.rb

+end
+
+require 'puma'
+Puma::Server.class_eval { include PumaPatch }


I don't like monkey patching just because of one or two simple calls. In the method where you call mount you already have case server_name anyway, I suggest to do the mapping there, or in a launcher helper.

lzap · 2018-10-17T15:08:52Z

lib/launcher.rb

 require 'webrick'
 begin
-  require 'puma'
+  Bundler.require(:puma)


Please test this on RPM installation, I think this will not work. We actually turn bundler off for RPM installations and use special stub called bundler_ext so it works seamlessly with Bundler for development.

Will check it out at the end of my work (soon, very soon :) )

timogoebel · 2018-10-18T09:13:16Z

lib/launcher.rb

+      https_server_name = @settings.https_server_type
+      if https_app
+        t1 = case https_server_name
+             when :webrick


I would extract this to a method add_server() then then calls add_webrick_server or add_puma_server to reduce the duplication.

executing the server is blocking, so at the moment actually playing around with my threading design.

the need for it, is to support multiple hosts, that the current way of working as a server (puma that is) is blocking.

ik5 · 2018-10-21T11:57:26Z

Okay, almost there, now working on making SSL/TLS to work.

Important thing, after investing a lot of time testing it out, I can't bind to many hosts per port, only for a single one.
The reason is that I will require to manage multiple threads - one per host and port/connection type. The reason for it, is that each server running entry is block instead of non-block.
In order to manage multiple hosts/connection type, there is a need to manage a threadpool and other resources management system.

I think that as another task, such management can be written,

ik5 · 2018-10-23T12:42:19Z

Several notes:

A. Last working version of Puma with Ruby v2.0.0 is 3.10.0 (puma/puma#1662).
B. Puma's version of 3.10.0 does not support of filtering Ciphers in any way (so no code for that).
C. Currently fixing some tests, Ruby 2.0.0 have bug with Kerberos that makes it crash with the tests, so handpicking the tests and testing them.

Once the tests are ready I'll remove the WIP part.
@lzap now it's more or less ready for PR review regarding the sources of the logic without much of rewrites :)

iNecas · 2018-10-23T13:08:29Z

lib/puma_patch.rb

@@ -0,0 +1,11 @@
+
+class PumaMapperMiddleware
+  def initialize(app)


Is this middleware needed for anything right now?

Yes, it is the only way for Puma to enter the route and deal with it.

there is another way of doing it, but then we will ignore rack, and that by using add_xxx_listener (for example) of Puma, but we can then support multiple Ethernet devices, and that's the balance I was playing with.

Sorry, I don't get it. The only place this middleware is being used is in here https://github.com/theforeman/smart-proxy/pull/613/files#diff-b8fbcbffabe3ae5c9935bb21e3e94852R178, and I don't see the server variable being used anywhere.

iNecas · 2018-10-23T13:09:58Z

lib/launcher.rb

      @settings = settings
+      @settings.http_server_type = Proxy::SETTINGS.http_server_type.to_sym
+      @settings.https_server_type = Proxy::SETTINGS.https_server_type.to_sym
+      if @settings.http_server_type == :puma && !$HAS_PUMA


I don't think we need an extra $GLOBAL_VARIABLE, we can just check on defined? Puma

my issue with define puma is that if something else regarding puma was not loaded we do not know of it, and then looking for every point that is needed is a bit of an issue in my opinion.

You will end up either writing a function that does this, or have duplicate places that check that code.

So knowing that everything was loaded properly, even on other locations at the project when needed is a bit easier in my opinion and easier for the long run.

iNecas · 2018-10-23T13:22:53Z

lib/launcher.rb

 require 'thread'
+require 'rack'
+require 'webrick'
+require 'pry'


still a leftover from requiring pry for development

that's my bad, I added it when debug puma crashes, removing it

iNecas · 2018-10-23T13:23:15Z

lib/launcher.rb

 require 'proxy/signal_handler'
 require 'thread'
+require 'rack'
+require 'webrick'


Do we need to require webrick in case we will not use it?

if the loading of puma failed it is the fallback, and at the moment the SETTINGS constant is not available for me to know if to load it or not.

Do you have a better idea?

What about loading webrick or puma just after we know what server we're going to use? We don't have to load everything at the very beginning, if we have a reason for that. That would also allow us not to use the $HAS_PUMA global variable

iNecas · 2018-10-23T13:37:00Z

config/settings.yml.example

+# https server type configuration
+# A string that indicates the server type (possible values: 'webrick', 'puma').
+# Default value is 'webrick'
+#:https_server_type: 'puma'


I don't think we really need separate server_type setting for http and https and I think it's just misleading. One option should rule them all

okay unifying them

iNecas · 2018-10-23T14:19:22Z

When running via bundle exec bin/smart-proxy, I can't quit the process with CTRL+C, when running without bundle exec it fails with LoadError: cannot load such file -- puma. I think at least one of the variants should work

ik5 · 2018-10-24T06:27:33Z

@iNecas the pressing of ^C works for me, the only time it didn't was when i was inside a breakpoint.

ik5 · 2018-10-24T08:05:44Z

fixed all known test issues (ruby 2.0.0 crashes on a bug of kerberos), so using the following test mechanism to test my code :)

theforeman-bot · 2018-10-24T08:10:22Z

There were the following issues with the commit message:

e1e7373 must be in the format fixes #redmine_number - brief description
commit message for e1e7373 is not wrapped at 72nd column
commit message for e1e7373 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

iNecas · 2018-10-24T10:17:25Z

Interesting regarding the ^C behavior: can you check with ruby 2.4 and 2.5 (2.4.1 and 2.5.1 were misbehaving with ^C for me)

lzap · 2018-10-24T12:49:02Z

That sounds more like a race condition than a problem with a particular Ruby version.

Anyway, what is the status of the PR? Is this still WIP? I want to do formal review and testing after it's finished.

lzap · 2018-10-24T12:51:34Z

lib/launcher.rb

+      when :puma
+        Thread.new do
+          add_puma_server(app, addresses, port, conn_type)
+        end


Any particular reason why the two blocks don't have the same syntax (do vs {)?

originally there was a longer line and wanted to make it readable, use moved to do instead of proc.

lzap · 2018-10-24T13:01:05Z

I think the way we handle UNIX signals is incorrect in the current version, we kinda do this (signal_handler.rb):

trap(:INT) { exit(0) }
trap(:TERM) { exit(0) }

We should change that and do graceful shutdown. Puma server has methods for graceful stop for both Single and Cluster classes called stop_blocked. We should also correct this for webrick which offers method called shutdown. There is no need of checking a timelimit, systemd will kill the process if it takes too long (45 secs by default) to gracefully terminate.

lzap · 2018-10-24T13:06:13Z

To be able to do that, you'd probably need to use ::Puma::Launcher instead of ::Rack::Handler::Puma wrapper.

ik5 · 2018-11-04T11:28:19Z

@lzap Puma has it's own handler for the signals: https://github.com/puma/puma/blob/v3.10.0/lib/rack/handler/puma.rb#L70

And as you can see at that file, it is using launcher.

lzap · 2018-11-09T16:39:01Z

Interrupt Ruby exception is only for Ctrl-C interrupt for interactive mode. In production you run this as a daemon but it still needs to be able to respond to SIGTERM or SIGHUP and SIGUSR1. We use these to rotate logs and initiate graceful restart. So this is different thing, that's why I suggest to change how things are booted up.

Anyway, what is the status? This is entitled WIP, so wondering what are the plans and timeframe. I like your work, would totally love to see this happening.

ik5 · 2018-11-12T05:50:15Z

@lzap, the status is that I waited for your comments.
I'm not sure that it is WIP anymore, but I will add what you have commented here.

lzap · 2018-11-19T10:15:38Z

Sorry I haven't noticed you already removed the WIP tag, I would like to proceed with testing but it appears you closed the PR and deleted the branch?

lzap · 2018-11-19T10:33:34Z

I opened new PR at #623 with your commit, I will push it through. @iNecas want to take a look?

ik5 · 2018-11-19T10:35:18Z

I can explain it not over this thread.

…

On Mon, Nov 19, 2018 at 12:15 PM Lukáš Zapletal ***@***.***> wrote: Sorry I haven't noticed you already removed the WIP tag, I would like to proceed with testing but it appears you closed the PR and deleted the branch? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#613 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEdg_FIfCiTfv0dJrwH-9hNaJcrAdkRks5uwoTLgaJpZM4XbFxk> .

theforeman-bot added Needs testing Not yet reviewed labels Oct 14, 2018

ohadlevy reviewed Oct 14, 2018

View reviewed changes

lzap reviewed Oct 15, 2018

View reviewed changes

iNecas reviewed Oct 16, 2018

View reviewed changes

config/settings.yml.example Show resolved Hide resolved

lzap reviewed Oct 16, 2018

View reviewed changes

lzap reviewed Oct 17, 2018

View reviewed changes

timogoebel reviewed Oct 18, 2018

View reviewed changes

iNecas reviewed Oct 23, 2018

View reviewed changes

ik5 force-pushed the devel/puma branch from 9b6a5e6 to ea64d94 Compare October 24, 2018 07:52

ik5 changed the title ~~[WIP] Puma support for smart proxy~~ Fixes #25293 - Puma support for smart proxy Oct 24, 2018

ik5 force-pushed the devel/puma branch from 35f5c27 to e1e7373 Compare October 24, 2018 08:10

theforeman-bot added the Waiting on contributor label Oct 24, 2018

ik5 force-pushed the devel/puma branch from e1e7373 to 5e73818 Compare October 24, 2018 08:11

theforeman-bot removed the Waiting on contributor label Oct 24, 2018

lzap reviewed Oct 24, 2018

View reviewed changes

ik5 force-pushed the devel/puma branch from 5e73818 to a125bf2 Compare November 4, 2018 06:27

ik5 force-pushed the devel/puma branch from a125bf2 to 6f0d316 Compare November 5, 2018 07:23

Fixes #25293 - Puma support for smart proxy

d23307c

ik5 force-pushed the devel/puma branch from 6f0d316 to d23307c Compare November 5, 2018 14:40

ik5 closed this Nov 15, 2018

ik5 deleted the devel/puma branch November 15, 2018 06:41

lzap mentioned this pull request Nov 19, 2018

Fixes #25293 - Puma support for smart proxy #623

Closed

		@@ -0,0 +1,11 @@

		class PumaMapperMiddleware
		def initialize(app)

Fixes #25293 - Puma support for smart proxy #613

Fixes #25293 - Puma support for smart proxy #613

Uh oh!

Conversation

ik5 commented Oct 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ik5 commented Oct 14, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzap commented Oct 15, 2018

Uh oh!

ik5 commented Oct 15, 2018

Uh oh!

Uh oh!

iNecas commented Oct 16, 2018

Uh oh!

ik5 commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ik5 commented Oct 21, 2018

Uh oh!

ik5 commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iNecas commented Oct 23, 2018

Uh oh!

ik5 commented Oct 24, 2018