Patch5 for docker two stage

[mgage]

I believe this is in good enough shape to be looked at. I have not yet done all the tests
that I want to do to make sure it's solid. I'm open to suggestions for improvements.

htdocs/tmp is now owned by www-data
if either the admin directory or the admin course tables are missing then they are replaced.

for the time being I've put off including install instructions -- we can discuss that later.

[dlglin]

Doing a find/replace on the config files is fragile, as evidenced by the changes to the password quoting. For changes to localOverrides.conf, would it make more sense to either have the docker build generate that file from scratch (which should be safe since the .dist version is entirely commented out), or simply add new config options at the end of the file (before the closing line)? This way if the text changes in the .dist version it wouldn't break the find/replace.

[taniwallach]

In my testing there are several bugs.

• The admin_user table is not being recreated due to an error in the bash script condition.
• The wwsh command is not executing what is intended when it would be called.
• Dropping the global statistics table leads to it not be recreated, as the relevant line was commented out.
  Corrections are proposed in the "line comments".

[taniwallach]

I would recommend leaving the ADD_APT_PACKAGES line commented out, and explaining how to use it in the documentation.

Adding packages here causes a significant start-up delay for them to be installed. Users who need to access the shell inside a running docker container can either install things when needed, or can make the decision to pay the penalty in start-up time.

Installing extra support related packages to the base image is not considered to be appropriate due to DevOps concerns about keeping the image as small/clean as possible. (There are old posts from @xcompass about that, and the consensus seemed to be to do so.)

[mgage]

I see their point, but not having less and an editor available inside the container is a real disadvantage for a brand new user who might want to fix some minor glitch in the local container file or just wants to have a look around. @xcompass suggested using environment variables to differentiate between production and development (and in this case newbie) builds. I notice that the installation of vim is not very clean (lots of warnings), and in any case perhaps nano would be a better editor to include. more is included automatically so perhaps we could make do without "less". (pun intended) I'll think some more about this.

[taniwallach]

Even with the patches discussed in my review, there is still an issue with the "local statistics" being missing, which triggers an error:

Warning messages

    Couldn't find the OPL local statistics table. Did you download the latest OPL and run update-OPL-statistics.pl? at /opt/webwork/webwork2/lib/WeBWorK/Utils/LibraryStats.pm line 69. 

Error messages

Table 'webwork.OPL_local_statistics' doesn't exist at /opt/webwork/webwork2/lib/WeBWorK/Utils/LibraryStats.pm line 71.
Call stack

The following information can help locate the source of the problem.

    in WeBWorK::Utils::LibraryStats::getLocalStats called at line 1057 of /opt/webwork/webwork2/lib/WeBWorK/ContentGenerator/Instructor/SetMaker.pm
    in WeBWorK::ContentGenerator::Instructor::SetMaker::make_data_row called at line 1671 of /opt/webwork/webwork2/lib/WeBWorK/ContentGenerator/Instructor/SetMaker.pm
    in WeBWorK::ContentGenerator::Instructor::SetMaker::body called at line 155 of /opt/webwork/webwork2/lib/WeBWorK/Template.pm
    in WeBWorK::Template::template called at line 577 of /opt/webwork/webwork2/lib/WeBWorK/ContentGenerator.pm
    in WeBWorK::ContentGenerator::content called at line 232 of /opt/webwork/webwork2/lib/WeBWorK/ContentGenerator.pm
    in WeBWorK::ContentGenerator::go called at line 386 of /opt/webwork/webwork2/lib/WeBWorK.pm

I think that for "developers" and "simple" Docker use, the best solution is probably to disable local statistics by adding

$problemLibrary{showLibraryLocalStats} = 0;

into conf/localOverrides.conf.

[taniwallach]

Doing a find/replace on the config files is fragile, as evidenced by the changes to the password quoting. For changes to localOverrides.conf, would it make more sense to either have the docker build generate that file from scratch (which should be safe since the .dist version is entirely commented out), or simply add new config options at the end of the file (before the closing line)? This way if the text changes in the .dist version it wouldn't break the find/replace.

About how to efficiently and safely modify conf/localOverrides.conf it might be simplest to try using a patch file to modify the end of the file, or just to put a minimal file (without all the commented out lines) under docker-config and copy it into the image with a COPY line.

[mgage]

I think that for "developers" and "simple" Docker use, the best solution is probably to disable local statistics by adding
$problemLibrary{showLibraryLocalStats} = 0;
into conf/localOverrides.conf.

I think you are right, the developers and simple Docker use is unlikely to be used heavily for real students

[pstaabp]

@mgage I've decided to get off my anti-docker horse and dig in. Can you give me some basics to try this out? I ran docker build . with all the appropriate arguments however I think that built the whole thing.

[taniwallach]

With the changes in this PR, you should not need to first edit docker-compose.yml as was necessary when #1404 was first being used.

So roughly you want to do:

# cd into webwork2 directory (as usual)
docker build --tag webwork-base:forWW216 -f DockerfileStage1 .
docker-compose build

Another docker-compose build would only rebuild the "webwork" layers.

---

If you want to force a rebuild of the base OS image (the one which installs lots of Ubuntu packages, etc.) you would run:

# cd into webwork2 directory (as usual)
docker build --no-cache --tag webwork-base:forWW216 -f DockerfileStage1 .

using the --no-cache setting to force Docker not to use cached layers from the prior builds.

[taniwallach]

@mgage - I still think the condition to trigger the recreation of the admin course databases is broken. The change I suggested above fixes it for me.

Here is my test protocol:

Update the branch and rebuild the (stage2) image.

docker-compose build
docker-compose up -d

Enter the container and delete the admin_user table to trigger the condition to rebuild the admin course tables.

docker exec -it webwork2_app_1 bash

# Inside container
mysql -u webworkWrite -p -h db webwork
# give password (from .env)

mysql> drop admin_user;
mysql> exit

exit
docker-compose down
docker-compose up

I see in the middle of the output:

app_1  | check admin course and admin tables
app_1  | Waiting for database to become available...
db_1   | 2021-07-07 16:11:52 8 [Warning] Aborted connection 8 to db: 'unconnected' user: 'unauthenticated' host: '172.20.0.4' (This connection closed normally without authentication)
app_1  | using pre-existing admin course and admin tables

namely that the proposed version of docker-compose.yml is not triggering the admin course table recreation.

docker-compose down

Change the line in docker-config/docker-entrypoint.sh (diff below):

-    elif [ ! $ADMIN_TABLE_EXISTS ]; then
+    elif [ $ADMIN_TABLE_EXISTS == 0 ]; then

Rebuild the container (to get the modified version of docker-entrypoint.sh into the image). (Externally mounting via docker-compose.yml it would also work.)

docker-compose build
docker-compose up

This time I see:

app_1  | check admin course and admin tables
app_1  | Waiting for database to become available...
db_1   | 2021-07-07 16:16:28 8 [Warning] Aborted connection 8 to db: 'unconnected' user: 'unauthenticated' host: '172.21.0.4' (This connection closed normally without authentication)
app_1  | admin course db tables need updating
app_1  | Table user created as admin_user in database.
app_1  | 
app_1  | wwsh - The WeBWorK Shell
app_1  | Available objects: $ce (WeBWorK::CourseEnvironment)
app_1  |                    $db (WeBWorK::DB)
app_1  | Available modules: Data::Dumper
app_1  | 
app_1  | courseID: admin and scriptFile:  /opt/webwork/webwork2/bin/addadmin
app_1  | --------------------------------
app_1  | errors addPassword: password exists (perhaps you meant to use putPassword?) at /opt/webwork/webwork2/bin/addadmin line 4.
app_1  | 
app_1  | admin course tables created with one user: admin   whose password is admin

The error about addPassword vs. putPassword is expected, as the admin_password table was not removed.

Open http://localhost:8080/webwork2/admin and login as admin - it works.

[mgage]

I fixed the line to
elif [ $ADMIN_TABLE_EXISTS == 0 ]; then

-- just missed that item when I pushed your other suggestions.
My tests read the same as yours.

As to an earlier comment to Peter Staab. docker-compose.yml still needs to be edited to set the root password for mariaDB. Is there an alternative method that doesn't modify docker-compose.yml? I keep catching myself just before I commit changes in docker-compose.yml and have to go back and fix the password so that it is not revealed.

[mgage]

I'm still getting the error
Table 'webwork.OPL_local_statistics' doesn't exist at /opt/webwork/webwork2/lib/WeBWorK/Utils/LibraryStats.pm line 71.

The line editing the showLibraryLocalStatistics flag failed to include the fact that the original line in localOverrides.conf began with a # since it was commented out.

[taniwallach]

docker-compose.yml still needs to be edited to set the root password for mariaDB. Is there an alternative method that doesn't modify docker-compose.yml? I keep catching myself just before I commit changes in docker-compose.yml and have to go back and fix the password so that it is not revealed.

I cannot think of any reason why it could not be set in the .env file and the loaded from the environment in a similar manner to what we already for for the regular DB password.

    MYSQL_PASSWORD: ${WEBWORK_DB_PASSWORD}

Trying this out requires totally removing the storage volume for the database, as AFAIK only when a new database is first created is the MYSQL_ROOT_PASSWORD environment variable used to create the root database user account.

[taniwallach]

I think we should probably also add .env to .gitignore and the only allow forcing changes to be put into that file.

[taniwallach]

I just quickly tested a change to docker-compose.yml to load the mysql root password from .env using the same method as used for other things. After a docker volume rm webwork2_mysql and a docker-compose up the DB volume is recreated.

The new root SQL password was tested as follows:

docker exec -it webwork2_db_1 bash
mysql -u root -p

---

I tested the fix to showLibraryLocalStatistics from the last commit. It now works as expected. Sorry - I you added code to change it, and did not test that it was OK.

[taniwallach]

Other than not liking the startup delay due to ADD_APT_PACKAGES all the key changes in this PR now works in my tests.

The startup delay is something I and others can avoid by commenting out the ADD_APT_PACKAGES line.

1. The admin_user table is recreated when missing, and a full table schema check is done then.
2. The admin user account get created.
3. The SQL root password is set in .env and gets processed from there on the first creation of the database.
4. The library browser works properly "out of the box".
   I did not test using R, but the setting change makes sense.