Detach actioncable from the WebsocketServer and run it with the UI #18296

skateman · 2018-12-15T22:22:44Z

As the remote console implementation is growing, I think it is the right time to start using the worker for the remote consoles only. I looked up the documentation of ActionCable and realized that we were already running it under the default /cable endpoint. The reverse proxy configuration on the appliances masked this, so it is not really exposed in production. However, it consumes up to 5 worker threads just because.

So I was thinking why don't we use this embedded solution only? This way I can reduce the complexity of the WebsocketServer a little and maybe even rename it in the future.

@djberg96 this will solve your issue with websockets when running your development setup with evm:start.
@kbrock maybe we will have to adjust the number of threads for the workers.
~~@skateman you have to test this on an appliance~~

Merge with ManageIQ/manageiq-appliance#219
Parent issue: #18300

miq-bot · 2018-12-16T08:40:50Z

Checked commit skateman@d7d2824 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

kbrock

Sorry. Took me a little while.

As long as you can verify my logic, then I'm 👍

kbrock · 2018-12-17T16:28:56Z

config/application.rb

@@ -84,8 +84,10 @@ class Application < Rails::Application

    # Disable ActionCable's request forgery protection
    # This is basically matching a set of allowed origins which is not good for us
-    # Our own origin-host forgery protection is implemented in lib/websocket_server.rb
-    Rails.application.config.action_cable.disable_request_forgery_protection = true
+    config.action_cable.disable_request_forgery_protection = false


kbrock · 2018-12-17T16:29:43Z

config/application.rb

+    config.action_cable.disable_request_forgery_protection = false
+    # Matching the origin against the HOST header is much more convenient
+    config.action_cable.allow_same_origin_as_host = true
+    config.action_cable.mount_path = '/ws/notifications'


verify:

This says that we have action cable mounting at /ws/notifications.
This will create the route necessary to handle this.
Now action cable works like it does for other rails apps. (It just happens to use a slightly different route)

kbrock · 2018-12-17T16:30:33Z

config/routes.rb

@@ -2,6 +2,6 @@
  if Rails.env.development? && defined?(Rails::Server)
    logger = Logger.new(STDOUT)
    logger.level = Logger.const_get(::Settings.log.level_websocket.upcase)
-    mount WebsocketServer.new(:logger => logger) => '/ws'
+    mount WebsocketServer.new(:logger => logger) => '/ws/console'


Please verify:

We didn't mount ActionCable at /ws before.
We only called it manually in WebsocketServer?

future: we are mounting websocket server at /ws/console and rails is auto mounting action cable at /ws/notifications

kbrock · 2018-12-17T16:41:27Z

lib/websocket_server.rb

@@ -52,9 +52,6 @@ def initialize(options = {})
  end

  def call(env)
-    # Pass the request to ActionCable if it is for notifications
-    return ActionCable.server.call(env) if env['REQUEST_URI'].start_with?('/ws/notifications') && ::Settings.server.asynchronous_notifications


verfiy:

This code no longer has notification (action cable) traffic going through here. so we no longer need to handle this case.

Dumb question, are we still using ::Settings.server.asynchronous_notifications? At this point, is it even possible to disable async notifications?

Yes, because that prevents the frontend to connect...

Should we remove that option and assume it's on? It doesn't have to be here but it seems wasteful to check if you can't operate the UI without async notifications.

Well, you can ... if you turn it off, it would not complain, but you won't get realtime notifications.

@jrafanie with other words, if notifications aren't available, the frontend will keep trying to connect every minute and throw ugly errors to the user. However, if asynchronous_notifications is set to false, the frontend will not try to connect.

Great, if it makes sense, maybe we drop the ::Settings.server.asynchronous_notifications completely in a followup PR. I don't see how there's a way to deprecate it. There is no alternative.

Actually I'm still using that feature, it is really good for debugging if someone thinks that websockets break stuff in their code. It saved me multiple times

kbrock · 2018-12-17T16:41:37Z

spec/lib/websocket_server_spec.rb

@@ -17,15 +17,6 @@
  let(:env) { {'REQUEST_URI' => "/ws/#{url}", 'rack.hijack' => hijack} }

  describe '#call' do
-    context 'notifications' do


Fryguy · 2018-12-17T18:43:19Z

cc @jrafanie

Fryguy · 2018-12-17T18:48:08Z

@skateman Does this mean the WebsocketWorker goes away? From the way it's described in ManageIQ/manageiq-appliance#219 it sounds like all the traffic is handled by the UiWorker. Is that right? (I admit I don't understand the solution's architecture here)

skateman · 2018-12-17T19:05:26Z

@Fryguy no, WebsocketWorker will be still used for remote consoles, I would like to rename it to RemoteConsoleWorker or something similar. I already have a PoC of cockpit running through a very similar proxy (which is 99%) compatible with the one in WSWorker and I'm also close to finishing purr, i.e. native TCP connections through HTTP.

djberg96 · 2019-01-04T13:52:02Z

👍

jrafanie · 2019-01-04T14:05:32Z

@skateman this looks great to me. From a high level, do we need to change anything else in regards to how we boot puma/configure apache/load balancers besides what you've already done in ManageIQ/manageiq-appliance#219. In other words, are there followup PRs you know we need to do?

skateman · 2019-01-04T14:08:01Z

@jrafanie no, this will work well after the 2PRs are merged and I can continue with adding cockpit support to the proxy.

Fryguy · 2019-01-04T15:08:18Z

@skateman Can you open a placeholder issue for yourself to rename the WebSocketWorker to RemoteConsoleWorker (as per #18296 (comment) )

skateman · 2019-01-04T15:10:05Z

@Fryguy already have it #18300, I'm collecting all the changes here.

skateman mentioned this pull request Dec 15, 2018

Detach actioncable from the WebsocketServer and run it with the UI ManageIQ/manageiq-appliance#219

Merged

Detach actioncable from the WebsocketServer and run it with the UI

d7d2824

skateman force-pushed the actioncable-ui-worker branch from c127ca9 to d7d2824 Compare December 16, 2018 08:36

kbrock approved these changes Dec 17, 2018

View reviewed changes

jrafanie approved these changes Jan 4, 2019

View reviewed changes

jrafanie merged commit 602b434 into ManageIQ:master Jan 4, 2019

jrafanie self-assigned this Jan 4, 2019

jrafanie added enhancement providers/console labels Jan 4, 2019

jrafanie added this to the Sprint 102 Ending Jan 7, 2019 milestone Jan 4, 2019

skateman deleted the actioncable-ui-worker branch January 4, 2019 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detach actioncable from the WebsocketServer and run it with the UI #18296

Detach actioncable from the WebsocketServer and run it with the UI #18296

skateman commented Dec 15, 2018 •

edited

Loading

miq-bot commented Dec 16, 2018

kbrock left a comment

kbrock Dec 17, 2018

kbrock Dec 17, 2018

kbrock Dec 17, 2018

skateman Dec 17, 2018

kbrock Dec 17, 2018

skateman Dec 17, 2018

jrafanie Dec 17, 2018

skateman Dec 17, 2018

jrafanie Dec 18, 2018

skateman Dec 18, 2018

skateman Dec 18, 2018

jrafanie Jan 4, 2019

skateman Jan 4, 2019

kbrock Dec 17, 2018

Fryguy commented Dec 17, 2018

Fryguy commented Dec 17, 2018

skateman commented Dec 17, 2018

djberg96 commented Jan 4, 2019

jrafanie commented Jan 4, 2019

skateman commented Jan 4, 2019

Fryguy commented Jan 4, 2019

skateman commented Jan 4, 2019

Detach actioncable from the WebsocketServer and run it with the UI #18296

Detach actioncable from the WebsocketServer and run it with the UI #18296

Conversation

skateman commented Dec 15, 2018 • edited Loading

miq-bot commented Dec 16, 2018

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Dec 17, 2018

Fryguy commented Dec 17, 2018

skateman commented Dec 17, 2018

djberg96 commented Jan 4, 2019

jrafanie commented Jan 4, 2019

skateman commented Jan 4, 2019

Fryguy commented Jan 4, 2019

skateman commented Jan 4, 2019

skateman commented Dec 15, 2018 •

edited

Loading