Make the demo server be a single docker image #450

bwillard · 2018-06-30T03:25:42Z

This should:
a) Make local development a little easier
b) Make it possible to publish a base docker image publicly that folks can easily try out. (We still need to solve #449 to make this possible).

This also makes a small change to use the datatransferproject repo name, which we own, so we can start publishing this image.

…teract with

rtannenbaum

Nice, just some comments for clarity & cleaning up.

I also wonder if there's any reason to keep the old version around, and have this be a new distro? i.e. Would integrators ever still want to run frontend and backend in different places?

rtannenbaum · 2018-07-01T13:47:07Z

distributions/demo-server/build.gradle

 group = 'docker'
- dependsOn buildDemoServerImage, buildClientImage
+ dependsOn buildDemoImage
 }

 task installToolsAndDockerize {
 description = 'Installs required build tools and builds the Client and API Server Docker images'


should this desc be updated? since client and backend are now in the same image

rtannenbaum · 2018-07-01T13:51:02Z

distributions/demo-server/build.gradle

- defaultCommand("java", "-Xdebug", "-Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005", "-jar", "/app/demo-server-all.jar")
+ // nginx configuration
+ runCommand("apt-get update")
+ runCommand("apt-get -y install nginx")


can nginx be installed from an image, similar to how we do the java image? then you can define version number too

No I don't think so. A docker image can only have one base, so we have to either start from the nginx base or the java base, and install the other. I think we might be able to use docker compose to solve this problem, but that adds another dependency, so while maybe we will switch to that latter I didn't want to do that now.

rtannenbaum · 2018-07-01T13:52:00Z

distributions/demo-server/build.gradle

+ copyFile("build/webapp/test-keys/server.key", "/etc/ssl")
+
+ // Install supervisor to allow mutliple processes to run
+ runCommand("apt-get install -y supervisor")


same here, can supervisor come from an image?

I don't think so, for the same reasons.

rtannenbaum · 2018-07-01T13:52:34Z

distributions/demo-server/build.gradle

+ //defaultCommand("java", "-Xdebug", "-Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005", "-jar", "/app/demo-server-all.jar")
+ copyFile("src/config/demo/supervisord.conf", "/etc/supervisor/conf.d/supervisord.conf")
+
+ defaultCommand("/usr/bin/supervisord", "-c", "/etc/supervisor/conf.d/supervisord.conf")
 // Read in secrets and set them as environment variables.


TODO for issue #449 ?

rtannenbaum · 2018-07-01T13:54:09Z

distributions/demo-server/build.gradle

+ // create the user nginx runs as, matches user in nginx.config
+ runCommand("adduser --disabled-password nginx")
+
+ copyFile("src/config/client/nginx.conf", "/etc/nginx")


can nginx work be moved to a separate task this dependsOn?

I don't think so no, this is to compose the docker file, so I think it all needs to be together.

ah, misread this, copyFile is a Dockerfile directive right? forgot how that worked and read it as a scripted copy

maybe for clarity, document that these commands are all generating Dockerfile directives

I think that might be overly commenting the file.

I started to try it out, but then it seemed wierd we didn't explain that runCommand was just a docker command as well.

rtannenbaum · 2018-07-01T13:55:58Z

distributions/demo-server/build.gradle


+ //defaultCommand("java", "-Xdebug", "-Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005", "-jar", "/app/demo-server-all.jar")


nit: remove

rtannenbaum · 2018-07-01T14:00:44Z

distributions/demo-server/build.gradle

 }

-task dockerize {
- description = 'Builds the Client and API Server Docker images'
+task demoserver {


can demoserver and buildDemoImage be combined? if so I would keep the convention of naming the task dockerize, since we're in the demo distro, and the task we are doing is creating a docker image of the whole thing

Ya good point, done.

rtannenbaum · 2018-07-01T14:04:34Z

distributions/demo-server/build.gradle

 group = 'docker'
- destFile project.file("${buildDir}/demoserver/Dockerfile")
+ destFile project.file("${buildDir}/demo/Dockerfile")


looks like you started to rename demoserver -> demo in some places like above, which makes sense to me as we're now including the client in the image as well.

Can you do that everywhere else it's still called demo-server?

done, I left the folder name, but everywhere else its just demo.

(Really demoserver is still a fine name, I just changed the docker image so there was no weirdness with old docker images.)

rtannenbaum · 2018-07-01T14:05:23Z

distributions/demo-server/build.gradle

@@ -134,19 +134,37 @@ shadowJar {

 shadowJar.dependsOn(createSecretsFile, createApiFile)

-task createDemoServerDockerfile(type: Dockerfile) {
- description = 'Builds the Demo Server Dockerfile'
+task createDemoDockerfile(type: Dockerfile) {


nit: how about just createDockerfile?

before we were doing both client and server stuff, hence the different names

…ansfer-project into willard-oneserver

rtannenbaum

Nice, just some more documentation suggestions, otherwise looks good

Main suggestion is to use the term "demo-backend" (or something like that) to refer to the combined API + transfer worker jar we use for the demo.

Similarly, use consistent term to refer to the demo frontend which is nginx serving Angular.

rtannenbaum · 2018-07-02T15:50:06Z

Documentation/Developer.md

@@ -63,39 +63,30 @@ limitations under the License.
 * The copyright should be added to all new files from now on (note: it might be collapsed so not immediately obvious)

 ## Creating the Docker Network


Should this header be renamed "Starting docker?"

The network stuff is all within the demo image now, right?

did you see this one?

Oh nope, sorry, fixed.

rtannenbaum · 2018-07-02T15:55:41Z

Documentation/Developer.md

-The following builds and optionally runs the demo server (containing the API and Transfer Worker)
-on `port:8080`
+## Building/Running locally
+The following builds and optionally runs the demo server locally


"Server" is still confusing to me because it makes me think backend.. maybe it's just me

I'm fine with keeping the name but can you add a comment here that the demo image contains both the frontend (served by nginx) and "demo backend" (API and worker combined in one process), which is (obviously) different than what you'd have in production? (there, API and transfer worker are separate images, and the frontend is Angular and deployed separately)

I just came up with the term "demo backend", we can call that something else, but I think we should have a name for it that we can use in documentation

I called out it is all three parts.

rtannenbaum · 2018-07-02T15:57:17Z

Documentation/Developer.md

@@ -118,9 +109,8 @@ The following runs the client-rest api
 * `ng serve --ssl --port 3000 --proxy-config proxy.conf.json`

 The following builds and runs the demo-server (which contains the worker and the api) with the jettyrest transport to be


this documentation is out of date, right?

It was mostly right, I added the extra port that was needed.

rtannenbaum · 2018-07-02T15:58:29Z

distributions/demo-server/build.gradle

-task createDemoServerDockerfile(type: Dockerfile) {
- description = 'Builds the Demo Server Dockerfile'
+task createDockerfile(type: Dockerfile) {
+ description = 'Builds the Demo Dockerfile, which supports both the API and nginx'


how about:

...which includes both the frontend served by nginx, and a combined API + transfer worker backend

rtannenbaum · 2018-07-02T15:59:28Z

distributions/demo-server/build.gradle

 // Port to open up for the debugger
 exposePort 5005
+
+ // API server binary


It's not just API, it's the "demo backend" or whatever we decide to call it (API + transfer worker combined in one process)

can the jar be renamed to that too, i.e. demo-backend-all.jar?

I think demo-server is better than demo-backend, as it includes the API and the worker, so it really the middle layer and the backend.

rtannenbaum · 2018-07-02T16:01:46Z

distributions/demo-server/build.gradle

- defaultCommand("java", "-Xdebug", "-Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005", "-jar", "/app/demo-server-all.jar")
+ // nginx configuration
+ runCommand("apt-get update")
+ runCommand("apt-get -y install nginx")


rtannenbaum · 2018-07-02T16:05:01Z

distributions/demo-server/build.gradle

+ // create the user nginx runs as, matches user in nginx.config
+ runCommand("adduser --disabled-password nginx")
+
+ copyFile("src/config/client/nginx.conf", "/etc/nginx")


ah, misread this, copyFile is a Dockerfile directive right? forgot how that worked and read it as a scripted copy

maybe for clarity, document that these commands are all generating Dockerfile directives

rtannenbaum · 2018-07-02T16:05:13Z

distributions/demo-server/build.gradle

+ copyFile("build/webapp/test-keys/server.key", "/etc/ssl")
+
+ // Install supervisor to allow mutliple processes to run
+ runCommand("apt-get install -y supervisor")


rtannenbaum · 2018-07-02T16:05:42Z

distributions/demo-server/build.gradle

- dependsOn shadowJar, createClientDockerfile, copyWebApp
- dockerFile = project.file("${buildDir}/client/Dockerfile")
+task dockerize(type: DockerBuildImage) {
+ description = 'Builds the Demo Docker image including API server and nginx'


s/API/Demo backend

rtannenbaum · 2018-07-02T16:07:11Z

distributions/demo-server/build.gradle

 }

 task installToolsAndDockerize {
- description = 'Installs required build tools and builds the Client and API Server Docker images'
+ description = 'Installs required build tools and builds the Client and API Server Docker image'


sometimes we say client, sometimes we say nginx, to refer to the same thing. I would use the same term consistently to be clear, or come up with a new one like "demo-frontend" - nginx serving Angular

rtannenbaum · 2018-07-03T13:01:08Z

Documentation/Developer.md

@@ -63,39 +63,30 @@ limitations under the License.
 * The copyright should be added to all new files from now on (note: it might be collapsed so not immediately obvious)

 ## Creating the Docker Network


did you see this one?

Make the demo server be a single docker image to make it easier to in…

87f505f

…teract with

bwillard requested review from jimmarino and rtannenbaum June 30, 2018 03:25

Merge branch 'master' into willard-oneserver

4162af2

rtannenbaum reviewed Jul 1, 2018

View reviewed changes

bwillard added 2 commits July 1, 2018 20:23

Address review comments

8a0e083

Merge branch 'willard-oneserver' of https://github.com/google/data-tr…

080de28

…ansfer-project into willard-oneserver

rtannenbaum reviewed Jul 2, 2018

View reviewed changes

bwillard added 2 commits July 3, 2018 07:05

Address some review comments

f1c9803

Address some review comments

63f8aa5

rtannenbaum approved these changes Jul 3, 2018

View reviewed changes

bwillard and others added 2 commits July 3, 2018 08:18

Address some review comments

39fc469

Merge branch 'master' into willard-oneserver

489fc5f

bwillard merged commit 7d16169 into master Jul 3, 2018

bwillard deleted the willard-oneserver branch July 3, 2018 14:13


		//defaultCommand("java", "-Xdebug", "-Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005", "-jar", "/app/demo-server-all.jar")

		@@ -63,39 +63,30 @@ limitations under the License.
		* The copyright should be added to all new files from now on (note: it might be collapsed so not immediately obvious)

		## Creating the Docker Network

		@@ -118,9 +109,8 @@ The following runs the client-rest api
		* `ng serve --ssl --port 3000 --proxy-config proxy.conf.json`

		The following builds and runs the demo-server (which contains the worker and the api) with the jettyrest transport to be

Make the demo server be a single docker image #450

Make the demo server be a single docker image #450

Conversation

bwillard commented Jun 30, 2018

rtannenbaum 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

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

rtannenbaum 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

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