Redesign Build Deployment Process (External)

[MukuFlash03]

This PR serves as the implementation for this issue for redesigning our build and deployment processes across e-mission-server, join, admin-dash, public-dash primarily.

Collaborating on this along with @nataliejschultz .

All four redesign PRs for the external repos:
E-mission-server: #961
Join: e-mission/nrel-openpath-join-page#29
Admin-dash: e-mission/op-admin-dashboard#113
Public-dash: e-mission/em-public-dashboard#125

---

A rough plan can be:

1st phase

• Consolidating Differences
  • Minimize the differences in internal and external versions, cleanup unnecessary files, so on.
• Image_Build_Push.yml:
  • CI / CD via GitHub actions to build and push images to Dockerhub for all four repos based on the current process for e-mission-server.
  • Image build push only builds for that specific repo when merge is made to a specific branch.
  • Additionally, for e-mission-server, image-build-push will now also need to trigger the GitHub action in join, admin-dash, public-dash.

2nd phase:

• Multi-tier uniform repo structure in nrelopenpath
  • Need to change the internal repos to pull from the externally built images for each repo without duplicating the entire external code repository internally.

[MukuFlash03]

Checking in the first initial set of changes for consolidating the differences in the external and internal versions.
These are mostly related to config files based on the differences identified here

Currently included these changes:

1. secret_list.json

• Modified code here ./emission/net/auth/secret.py
• Added try-catch block to load data from secret_list non sample file present, else load from sample file.
• Now, can be removed from internal repo: nrelopenpath/webapp/conf/net/auth/secret_list.json as well.
  • Changes made in internal repo.

---

2. webapp/conf/analysis/debug.conf

• Removed the entire analysis directory from internal repo: nrelopenpath/webapp/analysis.
  • Changes made in internal repo.

---

3. Logging level changed for these files:
   Webapp/conf/log/webserver.conf <-> log/webserver.conf.sample
   analysis/conf/log/intake.conf <-> log/intake.conf.sample

• Can aim to match both versions by making external also have the same logging level as internal.
• Internal given priority as detailed logging is better for detecting errors, though it does make logs verbose.
• Also, DEBUG() statements [~1500] are much more than WARNING statements [~50].
  • Got this count by searching in the code base for logging.debug() and logging.warning(), respectively.
• Will lose out on all these when external repo is used / run if we still keep level as WARNING.
• Made both webserver.conf and intake.conf use logging level debug and formatter detailed.
  • File can now be removed internally as it matches external ones; removed log/ directory in both webapp and analysis.

[nataliejschultz]

From our testing, it looks like these values are not formatted correctly to pull into get_database.py.

[MukuFlash03]

From our testing, it looks like these values are not formatted correctly to pull into get_database.py.

More details on errors encountered are mentioned here in the public-dash PR.

To summarize:

1. Faced this error so far with docker-compose.dev with public-dash. Works fine with the prod version - docker-compose.yml.

2. Also, inside the dev containers spun up using docker-compose.dev, I was able to acess values for these environment variables directly via python.

Need further investigation.

[nataliejschultz]

Server testing:

First testing the external code as it currently is by building on the current master image. After creating the db:

$ docker run --name db -d -p 27017:27017 --network emission mongo:4.4.0

I built the container as such:

$ docker run --name em-server-1 -d -e DB_HOST=db -e WEB_SERVER_HOST=0.0.0.0 -e STUDY_CONFIG=stage_program --network emission shankari/e-mission-server:master_2024-04-15--53-23

The container spun up and I ran:

$ docker exec -it em-server-1 /bin/bash

Started the e-mission environment:

Source setup/activate.sh

And ran all the tests:

./runAllTests.sh

Every test was passing but one:

After downloading vim into the container and modifying the file with some prints, I found that the issue was a GET returning a 404. Many trials later, I figured out that the issue was from the docker run command setting STUDY_CONFIG=stage_program, instead of stage-program. After running:
export STUDY_CONFIG=stage-program

All tests passed!

This is in alignment with the success of test-with-docker and ubuntu-only-test-with-manual-install, which essentially do the same thing.

To test the changes that we’ve made, I ran the exact same process as before (without the incorrect environment variable) using the most recently created image from our consolidate-differences image_build_push.yml:

$ docker run --name em-server-1 -d -e DB_HOST=db -e WEB_SERVER_HOST=0.0.0.0 -e STUDY_CONFIG=stage-program --network emission mukuflash03/e-mission-server:image-push-merge_2024-04-24--44-54

./runAllTests worked for this as well.

It is reassuring that the test-with-docker and ubuntu-only-test-with-manual-install runs in our consolidate-differences branch also passed.

[shankari]

@nataliejschultz this only tests previously created images. I don't see you building the images.
By default, image_build_push.yml runs on merges to master, so I am not sure how any previously created image will have any of the changes

[nataliejschultz]

By default, image_build_push.yml runs on merges to master, so I am not sure how any previously created image will have any of the changes

On the image-push-merge branch, we changed the workflows mentioned above to temporarily run on push to image-push-merge branch for testing. Since the actions are running in @MukuFlash03 's forked repository, shouldn't the changes be checked out?

This is also how the images with the changes have been pushed; running image_build_push.yml on push to image-push merge on the fork.

[nataliejschultz]

Internal repo testing for Analysis:

Started mongo container from scratch:
$ docker run --name db -d -p 27017:27017 --network emission mongo:4.4.0

Cd into copy of nrelopenpath multi-tier branch and build + run:

$ docker build -t int-analysis ./analysis/
$ docker run --name int-analysis-1 -d -e DB_HOST=db -e WEB_SERVER_HOST=0.0.0.0 -e STUDY_CONFIG=stage-program -e PUSH_PROVIDER="firebase" -e PUSH_SERVER_AUTH_TOKEN="Get from firebase console" -e PUSH_APP_PACKAGE_NAME="full package name from config.xml. e.g. edu.berkeley.eecs.emission or edu.berkeley.eecs.embase. Defaults to edu.berkeley.eecs.embase" -e PUSH_IOS_TOKEN_FORMAT="fcm" --network emission int-analysis

Checked to make sure that all of the cmd shell scripts made it into the correct directory, based on the new internal configuration:

Exec into container, start emission environment, and run ./runAllTests.sh:

Tests passed.

[nataliejschultz]

Of note: these are currently the only differences between consolidate-differences branch and image-push-merge:

None of the differences are significant to the functionality of the images.

[nataliejschultz]

Just merged the changes from Mukul's tag automation work. Trying to get all of these PRs ready (removing conflicts, changing tags etc.) so that they're 100% ready for merge.

[nataliejschultz]

Okay, I think it is ready for review. All of the branches are prepped. However, I think we need to create a token to get the image build test to work, per this run I triggered saying:
"message": "Resource not accessible by personal access token",

[nataliejschultz]

@shankari Here is the token

[shankari]

I see this, but there is no instruction on what the token should be called or where it should be configured. Fortunately, I can read code 😄

It is also not clear at what level this token needs to be set (does it need to be at the org level or can it be at the repo level)?

[nataliejschultz]

Mukul outlined the tokens he used in the issue here:

• • Used classic Personal access tokens for downloading artifacts.
• • Used fine-grained tokens for GitHub REST APIs.

and in more detail in this comment

I believe they are organization-level tokens, since they're working across repositories rather than just within the server.

[shankari]

Mukul just said "This required usage of a fine-grained token with the required access scope was actions: write". Not, "This required usage of a fine-grained token called XXX with the required access scope was actions: write on repo YYYY defined at XXX level"

That is what this comment is about. Instructions should be precise and we should not give permissions that we don't need. Concretely, the answer is not "org level for everything".

Regardless, as you can see from the issue, I figured it out and created the tokens. This is feedback for future PRs and instructions.

[shankari]

@nataliejschultz almost there. Let's get this right before we move on to the repos that depend upon it.

[shankari]

@nataliejschultz why do we have the consolidate-differences branch in here?

[shankari]

This has not been fixed yet.

[nataliejschultz]

I wanted to leave it and continue to test in GH actions as the cleanup changes are made, but I removed it now!

[shankari]

I think we can actually remove this environment variable and hardcode it to 0.0.0.0
Especially now that we are focused on docker containers, and runs behind a reverse proxy, I have never seen this be anything other than 0.0.0.0

[nataliejschultz]

Was trying to remove this and have the environment variable hardcoded inside the start_script.sh and/or dockerfile, while modifying the functionality of emission/net/api/config.py to match this change.

I built a local image after removing the use of webserver.conf. I thought the functionality would be preserved by the get_config_data_from_env function in config.py, but it actually completely broke the code.
After running the container with:

docker run --name em-server-noWH -d -e DB_HOST=db  -e STUDY_CONFIG=stage-program --network emission webserver-changes-2

I ran the tests, and they were going verrrrrrrry slowly (it took an hour and a half in total). Almost every test failed, with the resultant error message being:

pymongo.errors.ServerSelectionTimeoutError: 1234:27017: [Errno 65] No route to host, Timeout: 30s, Topology Description: <TopologyDescription id: 66466bf340a670086a00b405, topology_type: Unknown, servers: [<ServerDescription ('1234', 27017) server_type: Unknown, rtt: None, error=AutoReconnect('1234:27017: [Errno 65] No route to host')>]>

Going to read through my changes and figure out what happened.

[nataliejschultz]

Found out that webserver.conf.sample is currently necessary, and definitely wasn't replaced by the environment variables. Trying again!

[nataliejschultz]

Re-built locally and ran tests. It worked with the changes in my most recent push.

[shankari]

This is just a duplicate of lines 7-10. I am not sure why these need to be copied again.

[nataliejschultz]

Removed lines 7-10

[shankari]

I don't see how this is supposed to work - the local_host variable is set if DB_HOST is not set but then in the else, you do export DB_HOST=$local_host. How is that supposed to work? $local_host is not the same as localhost

[nataliejschultz]

see here

[shankari]

Ditto. @nataliejschultz have you tried testing this with DB_HOST not set? I bet right now that it doesn't work

[nataliejschultz]

@shankari You're right, it does not work without DB_HOST set. I ran the server using my locally built image:

docker run --name em-server-no-host -d -e WEB_SERVER_HOST=0.0.0.0 -e STUDY_CONFIG=stage-program --network emission consolidate-diff-server-localbuild

and before I could exec into the container, it gave me this error:

2024-05-13 15:28:05 pymongo.errors.ServerSelectionTimeoutError: 172.26.0.3:27017: [Errno 111] Connection refused, Timeout: 30s, Topology Description: <TopologyDescription id: 664285c63b13fd554aa58ca9, topology_type: Unknown, servers: [<ServerDescription ('172.26.0.3', 27017) server_type: Unknown, rtt: None, error=AutoReconnect('172.26.0.3:27017: [Errno 111] Connection refused')>]>

[nataliejschultz]

@shankari I've tried several different ways to export the DB_HOST environment variable, but kept getting the connection refused error. I decided to try running the server container as it currently is in production by building a local image with an added echo.

docker build -t localhost-5 .
docker run --name em-server-no-host -d -e WEB_SERVER_HOST=0.0.0.0 -e STUDY_CONFIG=stage-program --network emission localhost-5

The echo I added is in docker_start_script.sh, after it's supposedly set with the sample file:

echo ${DB_HOST}
if [ -z ${DB_HOST} ] ; then
    local_host=`hostname -i`
    jq --arg db_host "$local_host" '.timeseries.url = $db_host' conf/storage/db.conf.sample > conf/storage/db.conf
else
    jq --arg db_host "$DB_HOST" '.timeseries.url = $db_host' conf/storage/db.conf.sample > conf/storage/db.conf
fi
cat conf/storage/db.conf

echo "LOCALHOST!!:" ${DB_HOST}

Container log:

2024-05-14 12:55:08 {
2024-05-14 12:55:08   "timeseries": {
2024-05-14 12:55:08     "url": "172.26.0.3",
2024-05-14 12:55:08     "result_limit": 250000
2024-05-14 12:55:08   }
2024-05-14 12:55:08 }
2024-05-14 12:55:08 LOCALHOST!!:

I got the connection refused error again. Thoughts:

• It isn't working in the first place
• I'm misunderstanding what's happening
• My db container is the problem

Going to try a new db container first and see if that's the issue.

[nataliejschultz]

Tried new DB container:
docker run --name db -d -p 27017:27017 --network emission mongo:4.4.0

Ran the exact same image as before, and got the same connection refused error.

[shankari]

I don't understand the challenge outlined in
#961 (comment)

The previous codebase assumed that if the DB_HOST was not set, the DB and the webapp were running on the same host. Obviously, if you try to test it with containers, it won't work.

However, with the new codebase, even if you run the DB and the webapp on the same container, it won't work because of #961 (comment)

[nataliejschultz]

Per the group meeting with @shankari, we clarified that:

• The else was removed, so this code should work as expected
• This method of setting the variable doesn't work when using containers, explaining the failures in my tests above

[nataliejschultz]

If this doesn't work with containers, then do we even need this functionality in a file that only runs when the dockerfile is run?? It makes sense in start_script.sh, but maybe not here.

[shankari]

Right. I was hoping you would come to that. A lot of these decisions were made a long time ago, when our deployment profile was very different than what we do now. If you recall, part of this redesign was to simplify and unify these startup scripts.

You should ask yourself these questions, answer them and then keep only the parts that are relevant.

Even in start_script.sh, why do you need this, given that there is a fallback in the config file?
And if you do need it, why does it need to be hostname -i?

[nataliejschultz]

I think I finally understand.
DB_HOST will never need to be set when running with containers

• because it should be specified in the compose for testing and
• containers run on their own IP(unless configured otherwise), so they're not going to connect using localhost anyway

start_script.sh runs all the tests, and the tests (and every server call in general) call get_database.py which gets the environment variable from emission.core.config. This has the fallback:

def get_config_data_from_env():
    config_data_env = {
        "timeseries": {
            "url": os.getenv('DB_HOST', "localhost"),

using hostname -i or localhost makes sense when not using containers, and the fallback to localhost should account for that. It seems like using localhost would be a rare case for someone running the server locally without containers.

[shankari]

why do we reload the config only here and nowhere else?

[nataliejschultz]

The STUDY_CONFIG environment variable is pulled directly above, so changing this value and reloading allows testers/users to set the variable to different configs in the container using export and test.

[shankari]

This statement doesn't answer my question. I asked why the config is reloaded here and not anywhere else.

• If we want testers/users to "set the variable to different configs in the container", why are we not concerned about people setting DB_HOST, for example.
• I am not sure what you mean by "using export and test". The only way to restart a webapp in the container is to restart the container. At which point, the webapp will read the config again. Why do we need to reload here?

[nataliejschultz]

I tested the server with and without the reload. When I removed the reload, one of the tests failed: This particular test calls the cfc_webapp.py. It appears that the reload is useful in this case for refreshing the value of "not_found_redirect"

[shankari]

I also don't think we need this given that we are going to use the environment variable going forward and it falls back to localhost "url": os.getenv('DB_HOST', "localhost"),

[shankari]

we should also remove the sample file to avoid confusion since we don't use it. Note that before this change, we used to fall back to the sample file if the db.conf file did not exist, but we have removed that now, so it is not clear what benefit the sample file gives us in terms of working out of the box or anything

[nataliejschultz]

Our logic here was that we wanted to maintain the functionality of db.conf for users running the server locally, in case they have it set in a certain way.

[shankari]

I would be fine with retaining this for now as a backwards compat hack but it should be removed in the next release.

[shankari]

Almost done; just need to finish cleanup

[shankari]

Right. I was hoping you would come to that. A lot of these decisions were made a long time ago, when our deployment profile was very different than what we do now. If you recall, part of this redesign was to simplify and unify these startup scripts.

You should ask yourself these questions, answer them and then keep only the parts that are relevant.

Even in start_script.sh, why do you need this, given that there is a fallback in the config file?
And if you do need it, why does it need to be hostname -i?

[shankari]

For example, you could also set the default value of DB_HOST here instead of having the if check above

[shankari]

nit: I would rename this to debug.conf.prod.json and debug.conf.dev.json to make this more clear.

[shankari]

when fixing the dockerized start script, we should fix this as well

[nataliejschultz]

I don't think we need this environment variable set here anyway, since start_integration_tests.sh runs via a Dockerfile whose corresponding compose has the variable hardcoded.

[shankari]

This statement doesn't answer my question. I asked why the config is reloaded here and not anywhere else.

• If we want testers/users to "set the variable to different configs in the container", why are we not concerned about people setting DB_HOST, for example.
• I am not sure what you mean by "using export and test". The only way to restart a webapp in the container is to restart the container. At which point, the webapp will read the config again. Why do we need to reload here?

[shankari]

This has not been fixed yet.

[shankari]

why do we need the tag_file.txt to be uploaded here given that we are passing it directly to the workflows on line 77?

[nataliejschultz]

See Mukul's comment in the issue:

Why I chose to add artifact method as well?

The issue I was facing was with fetching the latest timestamp for the image tag in case of a push event trigger. This is because in the workflow dispatch, the server workflow itself would trigger the workflows and hence was in a way connected to these workflows. However, push events would only trigger the specific workflow in that specific dashboard repository to build and push the image and hence would not be able to retrieve the image tag directly.

So, I utilized the artifact upload and download method to:

• upload the image timestamp as an artifact in the workflow run for future use.
• download the uploaded artifact from the latest previously successful and completed workflow run in e-mission-server repo for a specific branch (currently set to tags-combo-approach but to be changed to master once changes are final).