Bug #3038

Clean up recommended/default port numbers for API server and Workbench

Added by Tom Clegg about 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
-
Target version:
Start date:
09/23/2014
Due date:
% Done:

100%

Estimated time:
(Total: 3.00 h)
Story points:
1.0

Description

Currently there are various conflicting conventions in use.

  • Default apiserver port number in run_test_server.py is 3001.
  • Default apiserver port number to connect to in workbench default config is 3001.
  • Apiserver port number to connect to in workbench example config file is 3000.
  • Default workbench address to redirect to in apiserver default config is workbench.local:3001 -- but that's where API server itself wants to run!

Proposed improvement:

  • Hacking Workbench page currently suggests running Workbench in dev mode on port 3031.
  • Hacking API server page currently suggests running API server in dev mode on port 3030.
  • Let's choose port 3001 for "Workbench in test mode".
  • Let's choose port 3000 for "API server in test mode".
  • Update run_test_server.py and various other bits to reflect this 3000/3001 convention.
  • */*/config/application.default.yml should use localhost:3000 / localhost:3001 in the "test" section.
  • */*/config/application.yml.example should have encouraging comments in the "dev" and "production" section. Perhaps the "dev" section can use localhost:3030 / localhost:3031 (uncommented) as appropriate.
  • None of the yaml files should have any of these settings in the "common" section.

Subtasks

Task #3958: Update all the port numbers as specifiedResolvedRadhika Chippada

Task #3959: Review branch: 3038-default-port-numbersResolvedRadhika Chippada

Task #3995: Review 3991-docker-workbench-addressResolvedWard Vandewege

Task #3991: Update docker scripts to set workbench_address, reset it to nil in default configResolvedTom Clegg

Associated revisions

Revision 2a8d349e
Added by Radhika Chippada almost 5 years ago

closes #3038
Merge branch '3038-default-port-numbers'

Revision 8f3b2fc8 (diff)
Added by Ward Vandewege almost 5 years ago

Protect the one place in the api server code where we assume the
workbench_address parameter is always set, it is in fact optional.

refs #3038

Revision 9700a3a6 (diff)
Added by Ward Vandewege almost 5 years ago

API server: setting workbench_address is not required. This unbreaks the
docker build for the api server image.

refs #3038

History

#1 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg almost 5 years ago

  • Target version set to Arvados Future Sprints

#3 Updated by Tom Clegg almost 5 years ago

  • Target version changed from Arvados Future Sprints to 2014-10-08 sprint

#4 Updated by Radhika Chippada almost 5 years ago

  • Assigned To set to Radhika Chippada

#5 Updated by Radhika Chippada almost 5 years ago

  • Status changed from New to In Progress

#6 Updated by Radhika Chippada almost 5 years ago

  • Used localhost:nnnn in API and workbench application.default.yml and used "arvados.local" / "workbench.local" in application.yml.example files. This is in sync with previous setup + the tests are also using localhost:nnnn and hence it seemed better to stick with this usage pattern.
  • Removed port number configuration in common sections of the yaml files and added those in the development / test / production modes separately.

#7 Updated by Brett Smith almost 5 years ago

Reviewing eb8dc9d

  • In the Rails YAML files:
    • I think omitting configuration options from the test sections of application.yml.example could help avoid misleading our users. The default values provided should always work for tests, and strange failures might occur if users change those. Hinting that they can be changed by including them in the example might confuse a few users.
    • And then sort of the mirror of that, I think it would be a little safer to omit URLs from the development section of application.default.yml. My main concern is avoiding situations where our code talks to other servers on the host (which might not be another Arvados server, especially since 3000 is the default Rails development port) before the user realizes what's going on. Suggesting the defaults in application.yml.example should be sufficient.
    • Unsetting variables with ~ is usually only needed when you're clearing a previous setting. It shouldn't be necessary for any of these settings in application.default.yml, since we're being careful not to define a value in the common section.
  • A few more files need to be changed to match the new convention (I found these with git grep -E '\b30[0-3][0-3]\b'):
    • services/api/README
    • services/api/script/rails
    • services/keep/tools/traffic_test.py
  • It's not a big deal, but just to make sure I understand, why does this branch change button text from "Stop" to "Pause" in apps/workbench/test/diagnostics/pipeline_test.rb?

Thanks.

#8 Updated by Tom Clegg almost 5 years ago

Brett Smith wrote:

  • Unsetting variables with ~ is usually only needed when you're clearing a previous setting. It shouldn't be necessary for any of these settings in application.default.yml, since we're being careful not to define a value in the common section.

IMO the ~ is worth including for clarity here, even when it doesn't change behavior.

If our recommendations change, or have changed (e.g., put the whole useful test setup in default.yml instead of expecting the test suite to copy yml.example to yml), we should update Hacking API server.

#9 Updated by Radhika Chippada almost 5 years ago

Brett: addressed your feedback.

  • Regarding "In the Rails YAML files" :
    • I removed the config from test section in example yml
    • Updated the default yml files to use "~" instead. Please see Tom's comment regarding this.
  • Regarding "A few more files need to be changed to match the new convention"
    • services/api/README : This file seems to be rails generic README with instructions as to how to generate a rails application. Hence, changing the port number 3000 did not seem correct and left it as is.
    • services/api/script/rails - I wonder how I lost my update to this! Anyways, updated this and the other file. Thanks for catching them.
  • Regarding "apps/workbench/test/diagnostics/pipeline_test.rb": I retested this as part of my manual testing of port number updates and noticed that it is failing due to Peter's recent dashboard updates, which I had not noticed during testing the dashboard branch review.

#10 Updated by Brett Smith almost 5 years ago

f76e5864 is good to merge. Thanks.

#11 Updated by Radhika Chippada almost 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:2a8d349eaf2b1ac2254056ea32859f72a226d63f.

#12 Updated by Ward Vandewege almost 5 years ago

  • Status changed from Resolved to In Progress

Reviewing 3991-docker-workbench-address:

  • The change to docker/api/Dockerfile removes a warning. Cool.
  • The change to docker/passenger/Dockerfile seems fine. I note that it (greatly) increases the image build time because now Passenger needs to be rebuilt every time, but it's probably the right thing to do.
  • The change to docker/build_tools/build.rb which introduces

+ config['API_WORKBENCH_ADDRESS'] = 'http://localhost:9899'

is a problem, because this puts a run-time characteristic (the port the workbench docker container's web server is exposed at on the system running the container) hardcoded inside the image. That port is deliberately configurable in arvdock.

This is why workbench_address should not be configured in the docker containers.

arvdock is very explicit to point the user to workbench when the containers are started up. It shouldn't be necessary to have that redirect on the API server container in place. Sending e-mail from the docker containers is not supported (they are not set up for it), so that is also no reason to set workbench_address.

In other words, I think you should revert the changes to docker/api/application.yml.in, docker/build_tools/build.rb and docker/config.yml.example.

  • services/api/app/controllers/static_controller.rb

When workbench_address is not set, this patch removes a helpful warning message and replaces it with a more mysterious 'Path not found'. I guess you've decided to just give up on the redirect feature without trying to help the admin to configure it. I suspect people will get hung up on this (rather than immediately understanding what is happening). Why not tell people something like "This is an Arvados API server. You probably want to visit the Workbench application for this API server." if you really don't want to point out the actual problem (i.e. workbench_address is unset)?

  • The equivalent change for the e-mail templates is great.
  • All the other changes make things shorter and seem fine.

Thanks,
Ward.

#13 Updated by Tom Clegg almost 5 years ago

Ward Vandewege wrote:

Reviewing 3991-docker-workbench-address:

  • The change to docker/api/Dockerfile removes a warning. Cool.

Yeah, I thought it was failing there, so I fixed it, but it turned out to be a warning just before a different failure. Accidental improvement!

  • The change to docker/passenger/Dockerfile seems fine. I note that it (greatly) increases the image build time because now Passenger needs to be rebuilt every time, but it's probably the right thing to do.

I'm not sure why this was a fatal problem on my setup but not others. I'm guessing it means some part of the (ruby/rvm/??) environment was leaking through from the host to the container (and in my case there was none on the host to leak) but this does seem more correct in any case.

  • The change to docker/build_tools/build.rb which introduces

+ config['API_WORKBENCH_ADDRESS'] = 'http://localhost:9899'

is a problem, because this puts a run-time characteristic (the port the workbench docker container's web server is exposed at on the system running the container) hardcoded inside the image. That port is deliberately configurable in arvdock.
[...]
In other words, I think you should revert the changes to docker/api/application.yml.in, docker/build_tools/build.rb and docker/config.yml.example.

OK, I've changed the auto-configured setting to "false". If I remove it entirely, api server will rightly complain "you forgot to configure this, and you told me you're running in production mode". (And it seems like we might as well let people configure this via config.yml if they want to instead of forking application.yml.in, even though I suppose there are a hundred other things they can't configure this way...)

  • services/api/app/controllers/static_controller.rb

When workbench_address is not set, this patch removes a helpful warning message and replaces it with a more mysterious 'Path not found'. I guess you've decided to just give up on the redirect feature without trying to help the admin to configure it. I suspect people will get hung up on this (rather than immediately understanding what is happening). Why not tell people something like "This is an Arvados API server. You probably want to visit the Workbench application for this API server." if you really don't want to point out the actual problem (i.e. workbench_address is unset)?

OK, I've changed it to "Oops, this is an API endpoint. You probably want to point your browser to an Arvados Workbench site instead."

(I don't like the "tell the admin to configure the thing" message. The config-checking code already forced the admin to make a choice about this feature during installation, and the admin decided to disable it. I want the application to respect that as a legitimate choice instead of suggesting to the users that the application isn't configured properly.)

Thanks. Update at 48d0472 + a805395

#14 Updated by Ward Vandewege almost 5 years ago

Excellent, LGTM. Thanks.

#15 Updated by Tom Clegg almost 5 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF