Skip to content

Commit

Permalink
Prevent live view viewers from making openQA unresponsive
Browse files Browse the repository at this point in the history
* Avoid live view viewers from occupying too many Mojolicious workers by
  moving the image streaming of the live mode to the live handler daemon
* Make sure the frontend code can still connect to the streaming route in a
  development setup by taking the different port into account and by
  setting CORS headers
* Tested by running the main web UI service allowing only a limited number
  of connections via `script/openqa prefork -c 1 -w 1`. This causes it to
  be unresponsive if there is at least one live view tab open. With this
  change this is no longer a problem and multiple live tabs can be opened
  and are updated simultaneously.
* Tested without and with reverse proxy.
  • Loading branch information
Martchus committed Jul 10, 2024
1 parent 6db0ab5 commit 247a6ed
Show file tree
Hide file tree
Showing 7 changed files with 66 additions and 38 deletions.
30 changes: 20 additions & 10 deletions assets/javascripts/openqa.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,24 +121,34 @@ function reloadPage() {
location.reload();
}

// returns an absolute "ws://" URL for the specified URL which might be relative
function makeWsUrlAbsolute(url, servicePortDelta) {
// don't adjust URLs which are already absolute
if (url.indexOf('ws:') === 0) {
return url;
}

// read port from the page's current URL
var location = window.location;
var port = Number.parseInt(location.port);
function makeUrlPort(servicePortDelta) {
// read port from the location of the current page
let port = Number.parseInt(window.location.port);
if (Number.isNaN(port)) {
// don't put a port in the URL if there's no explicit port
port = '';
} else {
if (port !== 80 || port !== 443) port += servicePortDelta;
port = ':' + port;
}
return port;
}

function makeUrlAbsolute(url, servicePortDelta) {
const location = window.location;
const port = makeUrlPort(servicePortDelta);
return location.protocol + '//' + location.hostname + port + (url.indexOf('/') !== 0 ? '/' : '') + url;
}

// returns an absolute "ws://" URL for the specified URL which might be relative
function makeWsUrlAbsolute(url, servicePortDelta) {
// don't adjust URLs which are already absolute
if (url.indexOf('ws:') === 0) {
return url;
}

const location = window.location;
const port = makeUrlPort(servicePortDelta);
return (
(location.protocol == 'https:' ? 'wss://' : 'ws:/') +
location.hostname +
Expand Down
22 changes: 13 additions & 9 deletions assets/javascripts/running.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ function addDataListener(elem, callback) {
// ensure any previously added event source is removed
removeDataListener(elem);

// define callback function for response of OpenQA::WebAPI::Controller::Running::streamtext
// define callback function for response of OpenQA::Shared::Controller::Running::streamtext
if (!elem.eventCallback) {
elem.eventCallback = function (event) {
// define max size of the live log
Expand All @@ -241,7 +241,7 @@ function addDataListener(elem, callback) {
var catData = currentData + newData;
var newStartIndex = newLength - maxLiveLogLength;

// discard one (probably) partial line (in accordance with OpenQA::WebAPI::Controller::Running::streamtext)
// discard one (probably) partial line (in accordance with OpenQA::Shared::Controller::Running::streamtext)
for (; newStartIndex < catData.length && catData[newStartIndex] !== '\n'; ++newStartIndex);

firstElement.innerHTML = catData.substr(newStartIndex);
Expand Down Expand Up @@ -309,13 +309,13 @@ var last_event;

// loads a data-url img into a canvas
function loadCanvas(canvas, dataURL) {
var context = canvas[0].getContext('2d');
var context = canvas.getContext('2d');

// load image from data url
var scrn = new Image();
scrn.onload = function () {
canvas[0].width = this.width;
canvas[0].height = this.height;
canvas.width = this.width;
canvas.height = this.height;
context.clearRect(0, 0, this.width, this.width);
context.drawImage(this, 0, 0);
};
Expand All @@ -324,12 +324,16 @@ function loadCanvas(canvas, dataURL) {

function initLivestream() {
// setup callback for livestream
var livestream = $('#livestream');
livestream.eventCallback = function (event) {
const livestream = document.getElementById('livestream');
const servicePortDelta = Number.parseInt(document.getElementById('developer-panel').dataset.servicePortDelta);
const url = makeUrlAbsolute(livestream.dataset.url, servicePortDelta);
livestream.dataset.url = url;
const elements = $(livestream);
elements.eventCallback = function (event) {
loadCanvas(livestream, event.data);
last_event = event;
};
liveViewElements.push({log: livestream});
liveViewElements.push({log: elements});
}

function disableLivestream() {
Expand Down Expand Up @@ -1125,7 +1129,7 @@ function processWsCommand(obj) {
case 'error':
// handle errors

// ignore connection errors if there's no running module according to OpenQA::WebAPI::Controller::Running::status
// ignore connection errors if there's no running module according to OpenQA::Shared::Controller::Running::status
// or the test execution is stopped
if ((!testStatus.running || developerMode.stoppingTestExecution) && category === 'cmdsrv-connection') {
console.log('ignoring error from ws proxy: ' + what);
Expand Down
4 changes: 4 additions & 0 deletions lib/OpenQA/LiveHandler.pm
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use Mojo::Base 'Mojolicious', -signatures;
use OpenQA::Schema;
use OpenQA::Log 'setup_log';
use OpenQA::Setup;
use OpenQA::Utils qw(service_port);

has secrets => sub { shift->schema->read_application_secrets };

Expand Down Expand Up @@ -56,6 +57,9 @@ sub startup ($self) {
$job_r->post('/upload_progress')->name('developer_post_upload_progress')
->to('live_view_handler#post_upload_progress');

# register route for live streaming of image
$test_r->get('/streaming')->name('streaming')->to('running#streaming');

OpenQA::Setup::setup_plain_exception_handler($self);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Copyright 2014-2021 SUSE LLC
# SPDX-License-Identifier: GPL-2.0-or-later

package OpenQA::WebAPI::Controller::Running;
package OpenQA::Shared::Controller::Running;
use Mojo::Base 'Mojolicious::Controller', -signatures;

use Mojo::Util 'b64_encode';
Expand Down Expand Up @@ -178,8 +178,16 @@ sub streaming ($self) {
$self->render_later;
Mojo::IOLoop->stream($self->tx->connection)->timeout(900);
my $res = $self->res;
my $headers = $res->headers;
$res->code(200);
$res->headers->content_type('text/event-stream');
$headers->content_type('text/event-stream');

# set CORS headers required when not using a reverse proxy (so the port differs)
if ($self->is_local_request) {
my $req_origin = $self->req->url->base->clone->port(undef);
$headers->append(Vary => 'Origin');
$headers->access_control_allow_origin("$req_origin:" . service_port('webui'));
}

# setup a function to stop streaming again
my $timer_id;
Expand Down
1 change: 0 additions & 1 deletion lib/OpenQA/WebAPI.pm
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ sub startup ($self) {
$test_r->get('/status')->name('status')->to('running#status');
$test_r->get('/livelog')->name('livelog')->to('running#livelog');
$test_r->get('/liveterminal')->name('liveterminal')->to('running#liveterminal');
$test_r->get('/streaming')->name('streaming')->to('running#streaming');
$test_r->get('/edit')->name('edit_test')->to('running#edit');
$test_r->get('/badge')->name('test_result_badge')->to('test#badge');

Expand Down
33 changes: 18 additions & 15 deletions t/26-controllerrunning.t
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use lib "$FindBin::Bin/lib", "$FindBin::Bin/../external/os-autoinst-common/lib";
use DateTime;
use Test::Warnings ':report_warnings';
use OpenQA::Test::TimeLimit '6';
use OpenQA::WebAPI::Controller::Running;
use OpenQA::Shared::Controller::Running;
use OpenQA::Jobs::Constants;
use Test::MockModule;
use Test::Output qw(combined_is combined_like);
Expand Down Expand Up @@ -59,7 +59,9 @@ subtest streaming => sub {
$id = Mojo::IOLoop->stream($stream);
my $log = Mojo::Log->new;
my $contapp = Mojolicious->new(log => $log);
my $controller = OpenQA::WebAPI::Controller::Running->new(app => $contapp);
push @{$contapp->plugins->namespaces}, 'OpenQA::Shared::Plugin';
$contapp->plugin('SharedHelpers');
my $controller = OpenQA::Shared::Controller::Running->new(app => $contapp);
my $faketx = Mojo::Transaction::Fake->new(fakestream => $id);
$log->unsubscribe('message');
$log->on(message => sub { my ($log, $level, @lines) = @_; $log_messages .= join "\n", @lines, '' });
Expand Down Expand Up @@ -89,7 +91,7 @@ subtest streaming => sub {

# test image streaming
$contapp->attr(schema => sub { FakeSchema->new() });
$controller = OpenQA::WebAPI::Controller::Running->new(app => $contapp);
$controller = OpenQA::Shared::Controller::Running->new(app => $contapp);
$faketx = Mojo::Transaction::Fake->new(fakestream => $id);
$controller->tx($faketx);
monkey_patch 'FakeSchema::Find', find => sub { Job->new };
Expand Down Expand Up @@ -122,13 +124,13 @@ subtest init => sub {
my $not_found;
my $render_specific_not_found;
my $render;
monkey_patch 'OpenQA::WebAPI::Controller::Running', not_found => sub { $not_found = 1 };
monkey_patch 'OpenQA::WebAPI::Controller::Running',
monkey_patch 'OpenQA::Shared::Controller::Running', not_found => sub { $not_found = 1 };
monkey_patch 'OpenQA::Shared::Controller::Running',
render_specific_not_found => sub { $render_specific_not_found = 1 };
monkey_patch 'OpenQA::WebAPI::Controller::Running', reply => sub { shift };
monkey_patch 'OpenQA::WebAPI::Controller::Running', render => sub { shift; $render = [@_] };
monkey_patch 'OpenQA::Shared::Controller::Running', reply => sub { shift };
monkey_patch 'OpenQA::Shared::Controller::Running', render => sub { shift; $render = [@_] };

my $c = OpenQA::WebAPI::Controller::Running->new(app => $app);
my $c = OpenQA::Shared::Controller::Running->new(app => $app);
$c->param(testid => 'foobar');

# No job could be found
Expand Down Expand Up @@ -170,15 +172,15 @@ subtest edit => sub {
my $not_found;
my $render_specific_not_found;
my $found;
monkey_patch 'OpenQA::WebAPI::Controller::Running', init => sub { 1 };
monkey_patch 'OpenQA::Shared::Controller::Running', init => sub { 1 };
monkey_patch 'FakeSchema::Find', find => sub { undef };
monkey_patch 'OpenQA::WebAPI::Controller::Running', not_found => sub { $not_found = 1 };
monkey_patch 'OpenQA::WebAPI::Controller::Running',
monkey_patch 'OpenQA::Shared::Controller::Running', not_found => sub { $not_found = 1 };
monkey_patch 'OpenQA::Shared::Controller::Running',
render_specific_not_found => sub { $render_specific_not_found = 1 };
monkey_patch 'OpenQA::WebAPI::Controller::Running', reply => sub { shift };
monkey_patch 'OpenQA::Shared::Controller::Running', reply => sub { shift };

# No results
my $c = OpenQA::WebAPI::Controller::Running->new(app => $app);
my $c = OpenQA::Shared::Controller::Running->new(app => $app);
$c->param('testid', "foobar");
$c->stash("job", Job->new);
$c->edit();
Expand All @@ -187,8 +189,8 @@ subtest edit => sub {
# Check if we can get the fake results
my $details_count;
monkey_patch 'FakeSchema::Find', find => sub { Job->new };
monkey_patch 'OpenQA::WebAPI::Controller::Running', redirect_to => sub { $found = 1; $details_count = $_[5]; };
$c = OpenQA::WebAPI::Controller::Running->new(app => $app);
monkey_patch 'OpenQA::Shared::Controller::Running', redirect_to => sub { $found = 1; $details_count = $_[5]; };
$c = OpenQA::Shared::Controller::Running->new(app => $app);
$c->param('testid', "foobar");
$c->stash("job", Job->new);
$c->edit();
Expand Down Expand Up @@ -229,6 +231,7 @@ package Mojo::Transaction::Fake;
use Mojo::Base 'Mojo::Transaction';
sub resume { ++$_[0]{writing} and return $_[0]->emit('resume') }
sub connection { shift->{fakestream} }
sub remote_address { '::1' }

package FakeSchema;
sub new {
Expand Down
2 changes: 1 addition & 1 deletion templates/webapi/test/live.html.ep
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@
</div>

<div id="canholder">
<canvas id="livestream" width="1024" height="768" data-url='<%= url_for("streaming", testid => $testid) %>'>
<canvas id="livestream" width="1024" height="768" data-url="/liveviewhandler/tests/<%= $testid %>/streaming">
</canvas>
</div>

Expand Down

0 comments on commit 247a6ed

Please sign in to comment.