Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the flexible runtime for the webapp #4226

Merged
merged 11 commits into from
Feb 6, 2025
Merged

Use the flexible runtime for the webapp #4226

merged 11 commits into from
Feb 6, 2025

Conversation

past
Copy link
Member

@past past commented Feb 3, 2025

Implements the proposal in #4219. I tested the new Dockerfile locally running the following from the top-level directory:

docker build -f webapp/web/Dockerfile --build-arg=LOCAL_SRC=. -t webapp .

@jcscottiii
Copy link
Collaborator

I see that searchcache does this interesting thing where it creates a temporary wpt.fyi directory beforehand

wpt.fyi/Makefile

Lines 210 to 217 in 81b71a1

if [[ "$${APP_PATH}" == "api/query/cache/service" ]]; then \
TMP_DIR=$$(mktemp -d); \
rm -rf $(WPTD_PATH)$${APP_PATH}/wpt.fyi; \
cp -r $(WPTD_PATH)* $${TMP_DIR}/; \
mkdir $(WPTD_PATH)$${APP_PATH}/wpt.fyi; \
cp -r $${TMP_DIR}/* $(WPTD_PATH)$${APP_PATH}/wpt.fyi/; \
rm -rf $${TMP_DIR}; \
fi

We should modify the conditional to include webapp too (since we are copying how the searchcache image is built). That should fix the error of where cloud builder can't find wpt.fyi

Long term we can investigate if we even need that temporary directory.

Also, something to consider for a future PR. Change the final image type from gcr.io/distroless/static-debian11 to gcr.io/distroless/static-debian12 since debian11 is superseded by debian12. This change would go in both the searchcache and webapp images. But to reduce the amount of variables in this PR, feel free to do it later.

@past past force-pushed the past/appengine-flex branch from 181fb84 to e97c5f7 Compare February 4, 2025 22:36
@jcscottiii
Copy link
Collaborator

jcscottiii commented Feb 4, 2025

I took a look. The package.json is missing because of the .gcloudignore. (the package.json file wasn't found in the manifest.json)

# Configurations
.eslintrc.json
package-lock.json
package.json
renovate.json
wct.conf.json

We can probably use the simpler one used for searchcache since the docker file does the multi-stage compilation into a single binary

# Files matching the following rules won't be uploaded to AppEngine (webapp).
# The syntax is the same as `.gitignore`; we do not use `.gitignore` directly
# because we need to update some generated files.
# Google Cloud secret
client-secret.json

webapp/web/Dockerfile Outdated Show resolved Hide resolved
past and others added 2 commits February 4, 2025 15:57
@past
Copy link
Member Author

past commented Feb 5, 2025

Deployed successfully, but the deployed version doesn't appear to be working yet. More investigation tomorrow.

@past
Copy link
Member Author

past commented Feb 5, 2025

I think I need to copy some more files, like node modules, to the final container.

@jcscottiii
Copy link
Collaborator

One time instructions:

docker network create wptfyi-network
docker pull google/cloud-sdk:latest
docker run -d \
  --name datastore-emulator \
  --network wptfyi-network \
  -p 8081:8081 \
  google/cloud-sdk:latest \
  gcloud beta emulators datastore start --project=local --host-port 0.0.0.0:8081 --no-store-on-disk

Iterative instructions:

make package_service PROJECT=wptdashboard-staging APP_PATH=webapp/web/app.staging.yaml
pushd webapp/web
docker build -t wpt-web-app-img .
docker rm -f  wpt-web-app; docker run \
  --name wpt-web-app \
  --network wptfyi-network \
  -p 8082:8080 \
  -e DATASTORE_EMULATOR_HOST=datastore-emulator:8081 \
  -e DATASTORE_PROJECT_ID=local \
  -e GAE_DEPLOYMENT_ID=local \
  -i wpt-web-app-img
# ctrl-c to stop then type popd to go back and start the build again.
popd

Open the browser to http://localhost:8082. Even though it says 8080.

If you want to inspect the files, it becomes harder since we are using a distro less image. You could temporarily swap to a debug image. More about it here in the Temporary section.

-FROM gcr.io/distroless/static-debian12
+FROM gcr.io/distroless/static-debian12:debug

Clean up instructions (when finally done):

docker rm -f  wpt-web-app datastore-emulator
docker network  rm  wptfyi-network

@jcscottiii
Copy link
Collaborator

jcscottiii commented Feb 6, 2025

The static files are not served because the flexible environment does not support declaring handlers in the app.yaml.

I switched us to using Nginx and added those handlers in the Nginx config (based on this example). It should work on app engine now but I just realized that the local environment uses this outdated library. And that library will read the app.yaml and create a non production server to read those static files from:

// This behaves differently in prod and locally:
// * Prod: the provided app.yaml is not used; it simply starts the
// DefaultServerMux on $PORT (defaults to 8080).
// * Local: in addition to the prod behaviour, it also starts some
// static handlers according to app.yaml, which effectively replaces
// dev_appserver.py.
nicehttp.Serve("webapp/web/app.staging.yaml", nil)

In this PR, we should probably revert my changes to the app.yaml files. But then follow up with my change and removal of nicehttp (and verify that following the directions in the README for local development work). And instead, leverage NGINX for both local and app engine environments.

We also use NGINX in webstatus for the frontend service too.

@jcscottiii
Copy link
Collaborator

It is up now! But I would click around to make sure nothing else is broken.

@past past mentioned this pull request Feb 6, 2025
@past past requested a review from jcscottiii February 6, 2025 19:43
@past
Copy link
Member Author

past commented Feb 6, 2025

I think this is working as intended now both locally and in the staging deployment. Besides the TODO for nicehttp, I have another TODO for figuring out what to do for the VPC connector. It doesn't seem to hurt keeping it in for now, any thoughts on that?

@jcscottiii
Copy link
Collaborator

@past I think it is fine to keep those VPC connections for now. We can always clean up in the future.

@past past merged commit 56f3a71 into main Feb 6, 2025
12 checks passed
@past past deleted the past/appengine-flex branch February 6, 2025 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants