Zebra: Stop using the default ports in the zebrad acceptance tests

[teor2345]

When I run the zebrad command acceptance tests, they use the default zebrad ports.

If we use the default ports, the tests could fail if there is a running zebrad instance. (Or they could make it impossible to launch another zebrad instance.)

TODO:

• [ ] create a config that overrides the default Zcash protocol listener port in zebrad
• [ ] pass "0" for the Zcash protocol listener port, so that zebrad chooses any available port
• [ ] run all the tests at the same time, to make sure there aren't any port or file conflicts

Diagnostics and Dependencies:

• [x] check that any optional ports are disabled by default (#660)
  
  
  
  • If we run all the tests at the same time, they will test that these ports are disabled
  
  
• [x] print the listener ports on startup (#867)
  
  
  
  • [x] if we can't get the listener ports easily, we should just print the configured port
  
  

Currently, Zebra's Zcash protocol listener is enabled by default, and all the other listeners are off by default.

Edit: we don't use env vars any more
Edit: #660 made the tracing and metrics listeners optional and disabled by default

[teor2345]

@oxarbitrage do you have time to fix this issue?

It's not as important as #806, because the ports are only open for a short time.

[teor2345]

See also #660, which is the long-term change we want to make. But we don't want to change the defaults right now, because that means updating the docker configs.

But it's ok to add override environmental variables, because the docket configs will still use the defaults.

[oxarbitrage]

We have 2 additional ports that will be open when zebrad is running:

1. metrics:

[metrics]
endpoint_addr = '0.0.0.0:9999'

1. tracing:

[tracing]
endpoint_addr = '0.0.0.0:3000'

Ideally instead of adding env variables we could change the config in the tests. This is possible with the abscissa runner, ex: https://github.com/iqlusioninc/abscissa/blob/develop/cli/template/tests/acceptance.rs.hbs#L72 but we moved away from this in favour of launching our own process. I am unsure if we can implement that to our codebase, maybe @yaahc can advice here.

[teor2345]

Ideally instead of adding env variables we could change the config in the tests.

We don't need to use abscissa. We could write a short toml file to the temporary directory, and tell the test to load it using -c. I did a quick test, and it looks like port 0 makes zebrad choose an unused port.

Here's a file that could work:

[metrics]
endpoint_addr = '127.0.0.1:0'
[network]
listen_addr = '127.0.0.1:0'
[tracing]
endpoint_addr = '127.0.0.1:0'

[hdevalence]

I don't think we should add an extra env variable that controls the port selection, since doing so means that in addition to the documented config system, we also have an additional, hidden selection mechanism that might conflict with it. Of course we could document it and specify how it interacts with the normal config mechanism in case of conflicts, but I think it would be much simpler not to have it at all.

It seems like the motivation for doing this is so that tests don't fail if there is already a running zebrad instance. This could happen in one of two ways:

1. multiple tests are run concurrently, so that there are multiple instances from the same test suite;
2. the test suite is run at the same time as an external zebrad instance.

In the first case, I think that the problem would be avoided if we ran the acceptance tests sequentially rather than concurrently. Our acceptance tests are a relatively small portion of our full test suite (and I expect this to remain the case), so the slowdown is fairly minor.

In the second case, my feeling is that this is both an uncommon case, as well as one that would be better solved by other means. It's uncommon because the only people it affects are people who run a zebrad instance on the same machine as they run the test suite, with no resource isolation between them. And I think it would be better solved by asking people who want to do that to specify a config for the test suite that partitions the shared resources, rather than having us try to decide how to share resources on their behalf (e.g., with automatic port selection).

[oxarbitrage]

I think that if we do this, we will do it with the config file method(https://github.com/ZcashFoundation/zebra/issues/807#issuecomment-667980110) instead of env vars originally suggested. Agree with @hdevalence that the env vars add unnecessary complexity, documentation needs, etc.

[teor2345]

As a general rule, if we allow people to override the test config, we should provide a default that makes the tests pass under most reasonable circumstances.

If we don't, what will happen in practice is that we'll get spurious bug reports, and have to spend time responding to them.

I'm also going to note that this is a temporary measure, until we disable these ports by default in #660.

[hdevalence]

If this is intended to be a temporary measure, could we fix #660 instead of doing this?

[oxarbitrage]

closing in favour of https://github.com/ZcashFoundation/zebra/issues/660

[teor2345]

If this is intended to be a temporary measure, could we fix #660 instead of doing this?

We don't want to change the defaults right now, because that means updating the zebrad docker configs to use zebrad.toml. And we don't want to make changes to docker until after the initial alpha release. So #660 is blocked by that release (or the docker task).

After #827 merges, this should be a really quick change.

We just need to add the listener lines to the test zebrad.toml file:
https://github.com/ZcashFoundation/zebra/pull/827/files#diff-8ceec19aa66391cf648b9cc56c9e3ac9R21

[oxarbitrage]

Ok, that is reasonable. Thanks for reopening. I am adding the blocked label until #827 is merged then i can probably take care of it.

[hdevalence]

I don't think we should merge this, #660 is the right fix and should not be blocked on changes to Docker images.

[oxarbitrage]

Ok, by https://github.com/ZcashFoundation/zebra/issues/660#issuecomment-669284518 it seems this can be re closed but i will let you guys take the decision :)

[teor2345]

660 doesn't disable the Zcash listener port, so we'll have to do that using a custom config in this ticket.

I've updated the description of this ticket based on #660 and this discussion.

[oxarbitrage]

I was trying to work in a subtask of this ticket: "print the listener ports on startup". Pretty easy however i am unsure on what to do when the user do something like 127.0.0.1:0 and the kernel picks up a port. Any ideas ?

[hdevalence]

@oxarbitrage It looks like the TcpListener::local_addr method might be useful: https://docs.rs/tokio/0.2.22/tokio/net/struct.TcpListener.html#method.local_addr

This has to be called on the listener after it's bound, so you need access to the listener variable. The listener is constructed in the listen function in src/peer_set/initialize.rs, so I think that would be the right place to add it.

[oxarbitrage]

@hdevalence thanks, that sounds good to get the correct port of the network. However, for tracing and metric endpoints we don't have this tcp listeners.

[teor2345]

@oxarbitrage:

Metrics

For metrics, we'd need to call server.local_addr after server.bind here:
https://docs.rs/metrics-exporter-http/0.3.0/src/metrics_exporter_http/lib.rs.html#65

But we don't have any way to get access to the Server instance.

So if Zebra users need this feature, they should ask the maintainers of metrics-exporter-http or hyper to add the local address to the tracing logs. (Or discover the address using OS process info tools.)

I've edited the task description, and deleted printing the metrics address.

Tracing

We can call server.local_addr after Server::try_bind().serve() here:
https://github.com/ZcashFoundation/zebra/blob/main/zebrad/src/components/tracing/endpoint.rs#L65

Here are the docs for server.local_addr:
https://docs.rs/hyper/0.13.7/hyper/server/struct.Server.html#method.local_addr

[hdevalence]

The default setting for the metrics and tracing endpoints is that they're disabled, so I think that the original issue (conflicting port assignments) won't happen unless a user manually specifies a port. I'm not sure it would ever make sense to specify automatic port assignment for these endpoints, because the point is for other software to talk to them. For instance, for the metrics endpoint, you have to configure a prometheus collector to scrape it, so automatic port assignments don't really make sense. For the tracing endpoint, we will eventually roll that into a real RPC mechanism, and the same rationale applies. So I think it would be fine just to include the user-specified address in the existing info messages when those components start up.

[teor2345]

I have updated the list of tasks in the ticket description based on #660, and this conversation.

[teor2345]

@oxarbitrage I think we still need to override the default Zcash protocol listener, and test that the override works by running all the acceptance tests in parallel:

• create a config that overrides the default Zcash protocol listener port in zebrad
• pass "0" for the Zcash protocol listener port, so that zebrad chooses any available port
• run all the tests at the same time, to make sure there aren't any port or file conflicts

[teor2345]

This issue was fixed by #920 and related PRs.