Skip to content

Remove mandatory PANDAOPS ENV from docker-compose#356

Closed
kdeme wants to merge 1 commit intoethereum:masterfrom
kdeme:remove-pandaops-env-docker-compose
Closed

Remove mandatory PANDAOPS ENV from docker-compose#356
kdeme wants to merge 1 commit intoethereum:masterfrom
kdeme:remove-pandaops-env-docker-compose

Conversation

@kdeme
Copy link
Contributor

@kdeme kdeme commented Jan 21, 2025

This is in order to allow for running the docker compose setup with non PANDAOPS infrastructure.

Else one has to specifically set:
export GLADOS_PANDAOPS_SECRET=""
export GLADOS_PANDAOPS_ID=""

This is not good UX.

This is in order to allow for running the docker compose setup
with non PANDAOPS infrastructure.

Else one has to specifically set:
export GLADOS_PANDAOPS_SECRET=""
export GLADOS_PANDAOPS_ID=""

This is not good UX.
@Ktl-XV
Copy link
Contributor

Ktl-XV commented Mar 17, 2025

Added a couple of changes to this PR in #380

@carver
Copy link
Contributor

carver commented Apr 8, 2025

Something looks very fishy here (in the original code, not the PR). PANDAOPS_ID and PANDAOPS_SECRET don't seem to be used anywhere. So I think removing those two is a no-op cleanup for every use case.

In contrast, PANDAOPS_CLIENT_ID and PANDAOPS_CLIENT_SECRET are loaded in the monitor code for history and beacon. So I would think removing them from the beacon monitor would break our use case. Except...

They do not seem to be configured in the main glados_monitor image, which is working normally AFAIK. So maybe there is some code that loads the PANDAOPS info, but we're not actively using it? I'll keep digging.

Here's where these were first added in October, for anyone else who goes spelunking to figure out what's going on: dadd731

@carver
Copy link
Contributor

carver commented Apr 8, 2025

Ok, it looks like in the main monitor code, it's only used for the bulk-download-block-data command which is never used in the docker-compose setup. So it can continue to be absent for glados_monitor.

The follow-beacon-pandaops command does use the values though, and will currently just exit with an error if they are missing.

Copy link
Contributor

@carver carver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it acceptable to run glados without beacon monitoring for you? If so, then there are a couple options:

  • at runtime, list every service you want to start, eliminating beacon_monitor (I recognize this kind of sucks, but just listing it for completeness and as a workaround while this PR is in progress. See some examples.)
  • we could add profiles to this docker-compose, and have a much shorter list of things to launch (the profile docs are pretty good)

If you want the beacon monitor to work, then this will take more effort. Perhaps that means a more generalized authorization encoding that defines an endpoint and authorization secret all combined into one, that you can set up to use your own endpoint. (and we can keep using PANDAOPS).

Comment on lines -10 to -11
PANDAOPS_ID: ${GLADOS_PANDAOPS_ID?Glados Pandaops ID Required}
PANDAOPS_SECRET: ${GLADOS_PANDAOPS_SECRET?Glados Pandaops Secret Required}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just this change seems great (see earlier commentary on PR).

Comment on lines -99 to -100
PANDAOPS_CLIENT_ID: ${GLADOS_PANDAOPS_ID?Glados Pandaops ID Required}
PANDAOPS_CLIENT_SECRET: ${GLADOS_PANDAOPS_SECRET?Glados Pandaops Secret Required}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that this change would break our beacon monitor.

@carver
Copy link
Contributor

carver commented Apr 8, 2025

I'm happy to pick up the work from here, but it would be nice to know how much folks care about getting beacon monitoring to work in their own glados setups. cc @kdeme

@Ktl-XV
Copy link
Contributor

Ktl-XV commented Apr 9, 2025

@carver PR #380 keeps the beacon monitor working in the generic case (no auth or basic auth beacon url) but means that the current EF Glados docker-compose will need to be different to the one in this repo.

Is that an acceptable solution? or a docker-compose that works both with a generic beacon url and pandaops is needed?

@kdeme
Copy link
Contributor Author

kdeme commented Apr 10, 2025

I'm happy to pick up the work from here, but it would be nice to know how much folks care about getting beacon monitoring to work in their own glados setups. cc @kdeme

Yes, this PR was basically an attempt to only fix 1 part. I realized that beacon-monitor would still be broken as things are hardcoded there currently to always use pandaops. Hence the issue #357

I'm fine with merging this PR as is or closing it and doing something better that tackles all.

Ideally any pandaops setting would be optional and would be set as is done for these envs: https://github.com/ethereum/glados/blob/master/DOCKER_GUIDE.md#deploying-glados

@kdeme kdeme closed this Oct 7, 2025
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.

3 participants