feat(server): support socket path in server config #389

MaximeHeckel · 2020-07-24T18:41:20Z

Fixes #385

Add sqlSocketPath option support in the config
Validate config UX with project maintainers
Add tests for new socket path option in config (Looking for guidance here 😄 )
Mention sqlSocketPath in docs/configuration.md

As mentioned in the ticket, my team and I were looking to set up lighthouse-ci server on Google Cloud Run (serverless containers) with a Google Cloud SQL database (Postgres) and our only possibility to link the two was to use a socket path instead of a connection URL.

Let me know what you think of this config, there's probably a few things that still need to be done or validated in terms of how you want the config to look like.
Also @patrickhulce mentioned that testing might be quite hard for this one, so I'm looking for any proposal on how I could tackle this 😄

googlebot · 2020-07-24T18:41:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

MaximeHeckel · 2020-07-24T18:42:40Z

@googlebot I signed it!

googlebot · 2020-07-24T18:42:44Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

patrickhulce

impl looks good to me! want to create another copy of mysql or postgres server test suite that uses an environment variable for the socket path to use?

it would look pretty similar to https://github.com/GoogleChrome/lighthouse-ci/blob/master/packages/server/test/mysql-server.test.js just with your new sqlSocketPath option

docs/configuration.md

packages/server/src/api/storage/sql/sql.js

MaximeHeckel · 2020-07-24T19:08:06Z

@patrickhulce thank you for the suggestions! Will address the comments and add tests

MaximeHeckel · 2020-07-27T15:42:11Z

@patrickhulce maybe you have a bit more context regarding how to use the mysql service with Github Actions but I currently don't seem to have permissions to connect through socket path in CI / Linux:
SequelizeConnectionError: connect EACCES /var/lib/mysql/mysql.sock

patrickhulce · 2020-07-27T16:29:53Z

maybe you have a bit more context regarding how to use the mysql service with Github Actions but I currently don't seem to have permissions to connect through socket path in CI / Linux

Well we'll need to start another copy of mysql that exposes the socket path, right now it's just running in a docker container on port 33306, so that socket file won't exist.

Looks like it's not too bad though https://superuser.com/questions/1411402/how-to-expose-linux-socket-file-from-docker-container-mysql-mariadb-etc-to

So steps...

We create a copy of the mysql entry currently under services but have it use a socket instead.
Expose the socket folder to the host image like the answer linked above
Do the same steps for creating our test database like we do for the port-based on but using the host socket file.
Set the environment variable like you do now to get the tests to use this socket file.

patrickhulce · 2020-07-27T23:55:07Z

.github/workflows/ci.yml

@@ -74,12 +89,17 @@ jobs:
          echo "::set-env name=CHROME_PATH::$(which google-chrome-stable)"
          echo "::set-env name=POSTGRES_DB_URL::postgres://postgres:postgres@localhost/lighthouse_ci_test"
          echo "::set-env name=MYSQL_DB_URL::mysql://root:[email protected]:33306/lighthouse_ci_test"
+          echo "::set-env name=MYSQL_SOCKET_PATH::/mnt/mysqld.sock"


fyi @MaximeHeckel you might want to create subdirs so the docker mount doesn't try to clobber an existing directory with the mount I don't think setting /mnt for both postgres and mysql will work

sorry this is such a pain to debug 😅

you might want to create subdirs so the docker mount doesn't try to clobber an existing directory

Yeah ran into that issue originally with /tmp

sorry this is such a pain to debug 😅

and haha no worries I'm used to use gh action and spending hours of trial and error until a workflow works

Is there a way wit gh actions to create the directories before the service starts? I believe the directories of the host need to be present before the service start right?

Hm I'm not sure to be honest. I would assume there should be but I haven't tried before

Did a bit of investigation yesterday night, it doesn't look like it's possible sadly. (Couldn't find a lot of documentations around volumes, tried to work around it but didn't find any satisfying solution to this)
What we can do maybe is use another path than /mnt? Do you have any suggestions? (I tried /tmp as well but yarn wouldn't be able to successfully run after putting the sockets there)

have you tried using the workspace dir yet? something like ${{ github.workspace }}/mysql:/var/run/mysqld? we just need it to be predictable and accessible

Just gave it a try, but like in the previous attempt the mysqlsocket service doesn't seem to be able to start: https://github.com/GoogleChrome/lighthouse-ci/pull/389/checks?check_run_id=919336503

patrickhulce · 2020-07-28T18:41:34Z

Oof, yeah this is really rough @MaximeHeckel I'm about to give up as well ☹️ https://github.com/GoogleChrome/lighthouse-ci/actions

I'm inclined to just skip the test here at this point since there's not explicit configuration involved if things are working on your end. sockets through docker just seem rough :/

leave the test file in play at least and maybe I'll play around with github actions some more on the subject later. just remove the environment variable for now in the github actions config I guess

patrickhulce · 2020-07-28T18:42:10Z

@googlebot I consent.

MaximeHeckel · 2020-07-28T22:48:03Z

I'm inclined to just skip the test here at this point since there's not explicit configuration involved if things are working on your end. sockets through docker just seem rough :/

yeah not an easy one (funny thing is that I used to work for Docker, and I remember struggling as much back then, seems that things haven't improved)

just remove the environment variable for now in the github actions config I guess

Will do that right now

The tests were passing and locally I didn't have any issues
For reference here's the Github Action to prove it 😄 both Postgres and Mysql working through socket: https://github.com/GoogleChrome/lighthouse-ci/actions/runs/184918838

...
2020-07-27T22:59:11.7719998Z PASS packages/server/test/postgres-socket-path-server.test.js (7.029s)
...
2020-07-27T22:59:17.7798161Z PASS packages/server/test/mysql-socket-path-server.test.js (12.73s)
...

patrickhulce

Oh wait you got it working!? That's amazing! Ignore my removal comment if you fixed it I thought we were stuck in failing limbo :)

thank you very much for your effort here! 🎉

happy to do it in a followup though if you're just done with this PR 😆

MaximeHeckel · 2020-07-29T13:40:06Z

Oh wait you got it working!? That's amazing! Ignore my removal comment if you fixed it I thought we were stuck in failing limbo :)

Oh sorry, I might have caused some confusion here:

I got it working locally
I got it working if mounting a volume in /mnt for example (that's where the logs you see in my previous come from, and that's where the run I linked in that same comment points to)

But like we investigated yesterday, we couldn't get it to work by mounting a volume in /mnt/mysql or /mnt/postgres because we can't create the folder before these services start.

As it stands now this PR has the tests for both mysql and postgres socket path but they are just not running because like you suggested I removed the environment variable for their respective socket paths and thus the tests are skipped.

patrickhulce · 2020-07-29T14:37:12Z

Ah ok gotcha 👍

patrickhulce requested changes Jul 24, 2020

View reviewed changes

docs/configuration.md Outdated Show resolved Hide resolved

packages/server/src/api/storage/sql/sql.js Outdated Show resolved Hide resolved

MaximeHeckel added 2 commits July 24, 2020 15:37

feat(server): support socket path in server config

bc8c2de

docs: Add configuration example for socket path database connection

7907b5d

MaximeHeckel force-pushed the support-socket-path-config branch 6 times, most recently from 365619c to ffe5500 Compare July 27, 2020 15:24

MaximeHeckel force-pushed the support-socket-path-config branch 15 times, most recently from 885a79b to 79fb77a Compare July 27, 2020 22:50

MaximeHeckel force-pushed the support-socket-path-config branch from 79fb77a to 7f7954e Compare July 27, 2020 22:52

patrickhulce reviewed Jul 27, 2020

View reviewed changes

MaximeHeckel force-pushed the support-socket-path-config branch 2 times, most recently from dc2b2cf to 62574f7 Compare July 28, 2020 14:36

patrickhulce mentioned this pull request Jul 28, 2020

tests: add postgresql socket test to CI #390

Merged

patrickhulce added the cla:yes label Jul 28, 2020

MaximeHeckel force-pushed the support-socket-path-config branch from 62574f7 to fa6dd16 Compare July 28, 2020 22:49

MaximeHeckel marked this pull request as ready for review July 28, 2020 22:51

tests: Add tests for mysql and postgres socket path option

8a7d195

MaximeHeckel force-pushed the support-socket-path-config branch from fa6dd16 to 8a7d195 Compare July 28, 2020 23:12

MaximeHeckel requested a review from patrickhulce July 28, 2020 23:28

patrickhulce approved these changes Jul 29, 2020

View reviewed changes

patrickhulce merged commit ab80a43 into GoogleChrome:master Jul 29, 2020

MaximeHeckel deleted the support-socket-path-config branch July 29, 2020 18:18

denkrasnov pushed a commit to Travix-International/lighthouse-ci that referenced this pull request Oct 29, 2020

feat(server): support socket path in server config (GoogleChrome#389)

3282d6a

This was referenced Jun 7, 2022

[Snyk] Security upgrade @lhci/server from 0.3.14 to 0.5.0 thexdesk/lighthouse-ci#19

Open

[Snyk] Security upgrade @lhci/cli from 0.3.9 to 0.5.0 thexdesk/lighthouse-ci#20

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): support socket path in server config #389

feat(server): support socket path in server config #389

MaximeHeckel commented Jul 24, 2020 •

edited

Loading

googlebot commented Jul 24, 2020

MaximeHeckel commented Jul 24, 2020

googlebot commented Jul 24, 2020

patrickhulce left a comment

MaximeHeckel commented Jul 24, 2020

MaximeHeckel commented Jul 27, 2020

patrickhulce commented Jul 27, 2020

patrickhulce Jul 27, 2020

patrickhulce Jul 27, 2020

MaximeHeckel Jul 28, 2020 •

edited

Loading

MaximeHeckel Jul 28, 2020 •

edited

Loading

patrickhulce Jul 28, 2020

MaximeHeckel Jul 28, 2020

patrickhulce Jul 28, 2020 •

edited

Loading

MaximeHeckel Jul 28, 2020

patrickhulce commented Jul 28, 2020

patrickhulce commented Jul 28, 2020

MaximeHeckel commented Jul 28, 2020 •

edited

Loading

patrickhulce left a comment

MaximeHeckel commented Jul 29, 2020

patrickhulce commented Jul 29, 2020

feat(server): support socket path in server config #389

feat(server): support socket path in server config #389

Conversation

MaximeHeckel commented Jul 24, 2020 • edited Loading

googlebot commented Jul 24, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

MaximeHeckel commented Jul 24, 2020

googlebot commented Jul 24, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

MaximeHeckel commented Jul 24, 2020

MaximeHeckel commented Jul 27, 2020

patrickhulce commented Jul 27, 2020

patrickhulce Jul 27, 2020

Choose a reason for hiding this comment

patrickhulce Jul 27, 2020

Choose a reason for hiding this comment

MaximeHeckel Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

MaximeHeckel Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

patrickhulce Jul 28, 2020

Choose a reason for hiding this comment

MaximeHeckel Jul 28, 2020

Choose a reason for hiding this comment

patrickhulce Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

MaximeHeckel Jul 28, 2020

Choose a reason for hiding this comment

patrickhulce commented Jul 28, 2020

patrickhulce commented Jul 28, 2020

MaximeHeckel commented Jul 28, 2020 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

MaximeHeckel commented Jul 29, 2020

patrickhulce commented Jul 29, 2020

MaximeHeckel commented Jul 24, 2020 •

edited

Loading

MaximeHeckel Jul 28, 2020 •

edited

Loading

MaximeHeckel Jul 28, 2020 •

edited

Loading

patrickhulce Jul 28, 2020 •

edited

Loading

MaximeHeckel commented Jul 28, 2020 •

edited

Loading