bisgardo / concordium-docker

A collection of scripts and configuration files to build and deploy a containerized node for the Concordium blockchain.
The Unlicense
5 stars 2 forks source link

Confusing parameters wrt logging? mainnet/testnet #24

Closed sderuiter closed 2 years ago

sderuiter commented 2 years ago

My goal is to start fresh on a new host. My end result should be:

  1. a mainnet node + new service tx logger
  2. a testnet node + new service tx logger

None of these nodes should run the legacy tx logger from the node itself.

Pain points right now:

  1. mainnet-txlog.env enables legacy logging (I think?).
  2. mainnet-txlog.env doesn't contain an entry for PGPASSWORD to be used? Did I miss this?
  3. https://github.com/bisgardo/concordium-docker/blob/0bcd4933359e1dc7751fdd71cbb61a06496259f5/docker-compose.yaml#L60 makes the port for the node hardcoded (again, I think). This prevents me from running 2 nodes on the same host, using this, correct?

Ideally, I would:

  1. clone this repo
  2. edit mainnet-txlog.env with the correct PGPASSWORD and PGDATABASE that I've setup for the mainnet service logging
  3. edit testnet-txlog.env with the correct PGPASSWORD and PGDATABASE that I've setup for the testnet service logging
  4. Do NODE_NAME=concordium-explorer-mainnet ./run.sh mainnet-txlog
  5. Do NODE_NAME=concordium-explorer-testnet ./run.sh testnet-txlog
  6. Be happy with 2 running nodes with tx logging through the service.

The above doesn't seem possible in it's current form, right?

sderuiter commented 2 years ago

Re reading, I think I forgot to mention that I can't seem to figure out which tx env variables I need to include to make it work.

bisgardo commented 2 years ago

The above doesn't seem possible in it's current form, right?

Correct.

mainnet-txlog.env enables legacy logging (I think?).

Yes. The .env files are just for convenience. You could set all the variables manually as described here. You can edit the file as you see fit (e.g. remove the line with CONCORDIUM_NODE_TRANSACTION_OUTCOME_LOGGING) or add the extra vars to the standard mainnet.env like

TRANSACTION_LOGGER_IMAGE=concordium/transaction-logger:0.2.2-0 TRANSACTION_LOGGER_PGDATABASE=mainnet_concordium_txlog_service TRANSACTION_LOGGER_PGPASSWORD=... NODE_NAME=concordium-explorer-mainnet ./run.sh mainnet

There are many ways of skinning this.

mainnet-txlog.env doesn't contain an entry for PGPASSWORD to be used? Did I miss this?

This var (TRANSACTION_LOGGER_PGPASSWORD) must be provided on runtime to keep it out of the public git repo.

Re reading, I think I forgot to mention that I can't seem to figure out which tx env variables I need to include to make it work.

The env vars for tx logging (the service, not legacy) are referenced here (them not being in the readme is a bug): It's TRANSACTION_LOGGER_xxx, where xxx are the same as for the legacy logger.

... makes the port for the node hardcoded (again, I think). This prevents me from running 2 nodes on the same host, using this, correct?

Yes, but it's not just that port, it's also 8888, 9090, and all the other port referenced in the file. I believe it would work if you cloned the Compose file and changed all the host-side ports. Would like to hear if it does.

Running multiple nodes on the same machine (particularly on different networks) is stretching the capabilities of Docker Compose. You could duplicate the services in the config file to make a mega deployment and that would probably work fine. Or change the ports as above. Otherwise I think you're entering Kubernetes land. I'm working on upgrading the Helm chart (https://github.com/bisgardo/concordium-docker/pull/21) but it's not yet properly tested.

bisgardo commented 2 years ago

If you aren't using the node dashboard, you could remove the services related to that. Then there aren't that many ports left to keep track of.

bisgardo commented 2 years ago

Hi @sderuiter did the explanations above help?

bisgardo commented 2 years ago

Note that #21 mentioned above has now been merged.

sderuiter commented 2 years ago

To be honest, no. I've started a new node+logger, I think it works, but the logger is extremely slow (still hasn't caught up after 5 days?).

I've abandoned running the logger (+ homegrown additional db on top) for the Explorer, it's too much maintenance at this moment.

Still have loggers running for statistics, but hoping to get to a state where I can move to a new host and install a node + logger without hassle. Sorry.

sderuiter commented 2 years ago

Not sure if this in the planning, but ideally either the node gets its logger removed, or the new logger uses parameters that have a distinctive naming pattern (to not confuse them with the node logging parameters).

My preference would be the first.

bisgardo commented 2 years ago

To be honest, no. I've started a new node+logger, I think it works, but the logger is extremely slow (still hasn't caught up after 5 days?).

I've also seen these very long catchup times. I was assuming that it was due to the old transaction logging. The new one should be much faster, especially if you enable parallelization with TRANSACTION_LOGGER_NUM_PARALLEL_QUERIES.

Not sure if this in the planning, but ideally either the node gets its logger removed, or the new logger uses parameters that have a distinctive naming pattern (to not confuse them with the node logging parameters).

The new logger service has been completely verified to be consistent with the old one, so it is indeed the intent to remove it from both this project and longer term also from the node.

I'm not sure what you mean by distinctive naming patters as they already use distinct prefixes (TXLOG for the old TRANSACTION_LOGGER for the new). The fact that they use the same names after the prefixes is intentional.

Once the old feature is removed (which it will be soon; filed #40), I will probably settle on just the TXLOG values.

bisgardo commented 2 years ago

All referenced tasks have been fixed (updated Helm chart, removed legacy transaction logging, improved documentation). Does this leave any outstanding issues? @sderuiter

bisgardo commented 2 years ago

Most relevant changes happened in https://github.com/bisgardo/concordium-docker/pull/41.

sderuiter commented 2 years ago

I'm not sure what you mean by distinctive naming patters as they already use distinct prefixes (TXLOG for the old TRANSACTION_LOGGER for the new). The fact that they use the same names after the prefixes is intentional.

Are you sure this is consistently applied in the repo? the .ENV files are called +TX_LOG, which then refers to the old logger, while I think the purpose of these .ENV files is to enable to new logger? The READMD.md file, under heading Transaction Logger, also discusses the new logger, but uses the name TX_LOG?

Also, that file has: TXLOG_PGDATABASE=mainnet_concordium_txlog

Is this referring to the new or old logger?

sderuiter commented 2 years ago

Further, in the readme under Transaction logger, you mention: TXLOG_PGDATABASE (default: concordium_txlog): Name of the database in the PostgreSQL instance created for the purpose.

In mainnet+txlog.env there's: TXLOG_PGDATABASE=mainnet_concordium_txlog

Now, in docker-compose.yml, there's: transaction-logger: .... dbname=${TXLOG_PGDATABASE-concordium_txlog}

Wouldn't this lead to a db name of mainnet_concordium_txlog-concordium_txlog?

bisgardo commented 2 years ago

Are you sure this is consistently applied in the repo? the .ENV files are called +TX_LOG, which then refers to the old logger, while I think the purpose of these .ENV files is to enable to new logger? The READMD.md file, under heading Transaction Logger, also discusses the new logger, but uses the name TX_LOG?

The term "txlog" was/is meant to refer to transaction logging in general, not a particular method. The old method was originally the only method; when the new one was added it had to have different names where they conflicted. The only place where this applied were in the environment variables.

The *-txlog.env files enabled both methods as explained in the readme (and further up this thread). The purpose of these files is entirely convenience.

The reason for keeping the legacy method until now was for verifying the new service.

Also, that file has: TXLOG_PGDATABASE=mainnet_concordium_txlog Is this referring to the new or old logger?

I used to use this database name for the old logger. Now it's used for the only logger.

Wouldn't this lead to a db name of mainnet_concordium_txlog-concordium_txlog?

No it means that it defaults to concordium_txlog if TXLOG_PGDATABASE is undefined (docs) as the readme says. ${TXLOG_PGDATABASE}-concordium_txlogwould have given that result.

bisgardo commented 2 years ago

The legacy transaction logging feature has been removed from the node as of v4.2.2 (https://github.com/Concordium/concordium-node/pull/395).