Could we be a bit more specific about what it means to stop the entire testnet? In docker, these commands will stop the running instances but leave all of the data in place. This doesn't really generalize to digital ocean, where bringing the node 'down' will also remove all of its data.

What do you think it should do? From docker-compose help down:

Stops containers and removes containers, networks, volumes, and images
created by `up`.

By default, the only things removed are:

- Containers for services defined in the Compose file
- Networks defined in the `networks` section of the Compose file
- The default network, if one is used

Networks and volumes defined as `external` are never removed.

I'm not sure there can be an equivalent of an "external" volume for DO servers.

The other tricky bit is that the perturbations have this notion of "stopping" a node that purely involves sending it a SIGTERM, without removing any resources.

My initial thought was that we should only ever totally destroy infrastructure (implying wiping nodes' data) during Cleanup. What do you think? The implication there would be that, for complex tests that rotate new groups of peers in and kill of old groups of peers, we'd end up keeping all VMs running when we may only need some of them.

If we want the division in our infrastructure to have Provision and Start as different operations, we'll need to adjust the current e2e app docker image that we use.

The up command in docker-compose will start a running instance of the Tendermint node since the entrypoint for the image actually runs the process. I'm not sure we really need to maintain two different methods, Start and Provision when Start should really be sufficient. In the DO case, Start can bring the host up if it's down and then kickoff the process, whereas in the docker-compose case, start will just run the image.

The other option, as far as I can tell, is to have a Provision method that is a No-op in the docker case. However, if we're going to do that, I think that this logic should live in the TestnetInfra interface as a Provision method there since we Provision the entire network at once in Terraform, not node-by-node.

The only reason I differentiated the two was because the perturbations have a notion of restarting a node that involves stopping and starting it without any change to infra provisioning, but I could just introduce a RestartNode function to subsume the two separate operations.

Actually, looking at it again now, there are 3 distinct categories of operations in use here:

1. Testnet-wide operations
2. Node-specific operations (including infrastructure)
3. Operations on nodes' processes (not touching the underlying infrastructure)

Actually, looking at it again now, there are 3 distinct categories of operations in use here: ...

I think the distinction makes a bit more sense, although in the case of docker-compose the start+provision step are intermixed at the moment. Looks like the API has been updated to reflect that, which seems right.

We can drop the word Node from these. A node is the only parameter of the method and also included in the name of the type.

See #8754 (comment)

I think the NodeInfra interface methods don't need the word "Node" in the method name.

The reason I did this is that I'm only separating out the TestnetInfra and NodeInfra for code readability. The alternative here is to have all functions directly on the Infra interface, which may be a better idea here.

If we change the way things are named here we'll have clashes between the two sets of functionality (i.e. we need to be able to Start a testnet, and we also need to be able to Start a node, and the functions can't have the same name).

If it's a problem I'd rather just lump all of the functions together in a single interface.

If we change the way things are named here we'll have clashes between the two sets of functionality (i.e. we need to be able to Start a testnet, and we also need to be able to Start a node, and the functions can't have the same name).

I sort of assumed based on this interface design that there wouldn't be situations where the same type would need to implement both interfaces, maybe though, this was not a clear assumption and we can figure out a different organization.

I'm still not convinced that the testnet implementation needs access to the internals of the node implementation, except as an optimization, is this correct?

I'm still not convinced that the testnet implementation needs access to the internals of the node implementation, except as an optimization, is this correct?

Unfortunately not, the runner CLI provides commands by which users can manipulate individual nodes. I won't be able to reduce the surface area of the new interface without substantially reducing the functionality provided by the testnet runner at present, and I'm not sure what the unintended consequences of that will be. Perhaps we can set that as a longer-term goal once we've implemented the Digital Ocean-based infra API?

the runner CLI provides commands by which users can manipulate individual nodes

This might actually be ok to remove. We should check with @cmwaters, but I don't think that any of these CLI operations are used for anything, except in theory making it easier to avoid reaching into the provider directly (which you can always do, and maybe ought to in these cases.)

the runner CLI provides commands by which users can manipulate individual node

Which commands are these - I thought all commands were for an entire testnet

is there ever a reason to have testnet implementations be specific to the provider and not just have the logs/start/stop be forloops over lists of node implementations?

All of the common logic is already in the main runner functions (Start, Setup, Cleanup, etc. in the runner's main package). The only logic I've extracted is that which is explicitly Docker-related.

The way the E2E package has been architected is a bit different to the rest of the codebase, in that it seems as though its style is more functional. For example, the Testnet and Node structs are primarily just data and provide very little functionality, and the runner package provides a bunch of functions that perform operations using them.

Is there some benefit to separating these interfaces? It doesn't look as if we implement either without the other.

If we do want them to be separate for some reason I missed, I'd suggest making this a concrete type with the others as fields. Among other things, that will allow us to swap them out independently (though we could at that point also just pass them as separate parameters too).

I've consolidated the interfaces into a single one now.

Need this return an error? Is there anything actionable the caller can do with an error here?

Separately: I realize Haskell uses "show" for this concept, but I think "print" might be more descriptive. YMMV.

This just seems like the convention used in the E2E tests. Returned errors are passed up through the stack to the Cobra CLI machinery and left to Cobra to deal with/display.

Could we require the implementation to report an error if the node wasn't provisioned? Or alternatively, could we require that StartNode call Provision automatically if it hasn't been? (Or maybe even just fold the two together?)

I've now renamed this to StartNodeProcess - it's a specific bit of functionality required by the perturbation logic where we shouldn't be provisioning any new infrastructure.

Can you explain how this is achieved? Are we modifying the config.toml or just halting the node?

In docker, this works by adding the running container to the docker network. This effectively connects it to the other running nodes. The container can be configured to start on the same network as the other nodes, but if it is not then calling connect will put the nodes onto the same network.

Docker creates virtual networks for containers and containers don't run (by default) on the host's network interface (although they can be configured to behave this way).

https://docs.docker.com/engine/reference/commandline/network_connect/

So this can be used to simulate the network itself failing?

How would this be achieved in DO?

The easiest way is to modify the node's iptables, cutting off incoming and outgoing connections to/from certain ports to "disconnect" a node and then restoring them to "connect" it.

For more advanced network outage simulation we'd need to install something like Toxiproxy on the nodes and configure it with the networking parameters we'd want.

So this can be used to simulate the network itself failing?

Correct.

How would this be achieved in DO?

A few ways I can think of

1. Digital Ocean has a concept of a firewall that can be used to isolate nodes from IP ranges. I haven't looked much into it, but it looks like it can do the trick.
2. If the firewall rules are too cumbersome or don't give us the flexibility we need, the backup is always to use IPtables rules DROP or REJECT rules depending on what we want to achieve. (Drop simulates a slow app not able to keep up with its ACKs, REJECT simulates a node that is completely unreachable behind a firewall (sends RST)).

Digital Ocean does seem to have an idea of a VPC (virtual private cloud networking) that allows nodes to all be on a shared network, but adding/removing nodes to the VPC seems sketchy and bad.
https://docs.digitalocean.com/products/networking/vpc/how-to/migrate-resources/

Do we need this interface to include testnet wide functions? Can't the caller i.e. the CLI, just loop through these for each of the nodes it has in it's manifest.toml

I guess to elaborate, is Setup, Stop, Pause etc. offering functionality that the test harness can't do themselves by calling the other, node-specific, methods

It can, although in the digital ocean case that may be way slower for things like spawning nodes.

As mentioned in #8754 (comment), in this PR all I've done is extract the Docker-specific functionality and hide it behind an interface. It's already the bare minimum interface surface area with the current architecture. For example, you'll notice there's no Start function on the TestnetInfra interface - that's because the Start function in the main package of the runner just uses StartNode in a loop.

Ok your call. I just ideally want to have the interface be as simple as possible

It can, although in the digital ocean case that may be way slower for things like spawning nodes.

Since provision doesn't exist, I'm assuming that the only way to use this method is by using it on a node that's had Start called on it before?

Yes, this method's only used by the perturbation logic at present.

So, in the case of Digital Ocean, provisioning infrastructure is one of the slow steps and I think that doing it all at once with a single or small set of calls to DO is likely going to be pretty advantageous. I think we should add some mechanism to the API to do the provisioning all at once. Perhaps that can happen in Setup, that seems fine to me, but I just wanted to call it out. Perhaps this method will start the node, provisioning any infrastructure if necessary? For example, if we're adding a node to the test late in the test we may need to provision new infrastructure but for simpler tests we can provision all of the infrastructure up front.

This is likely to be the case on any VM-based provider.

I'm also having sort of flashbacks to all of the ways this can go wrong and get aborted or end up leaking nodes at great expense.

In addition to figuring out how this is going to work for DO (presuming that we're just calling out to terraform, perhaps its better to think of this as a "terraform provider" rather than a DO provider,) perhaps it makes sense to make sure that our interface design decisions would make sense for a k8s based deploy (just as an exercise.)

I think we should add some mechanism to the API to do the provisioning all at once.

The only real way to accomplish this is to rearchitect the testnet runner. I initially started doing this over the weekend but thought everyone may be more amenable to reviewing a smaller change, but I'd be happy to revive my alternative approach as I still have it stashed locally. It's just going to be a bigger PR and will look very different to the current one 🙂

But that approach would involve introducing a higher-level interface:

type Runner interface {
    Start(ctx context.Context) error
    Setup(ctx context.Context) error
    Cleanup(ctx context.Context) error
    // etc. - only testnet-level manipulation and not node- or process-level
}

I'm also having sort of flashbacks to all of the ways this can go wrong and get aborted or end up leaking nodes at great expense.

I don't think we need to be too concerned about this. It's easy to delete a DO "project" and all of its associated resources.

In addition to figuring out how this is going to work for DO (presuming that we're just calling out to terraform, perhaps its better to think of this as a "terraform provider" rather than a DO provider,) perhaps it makes sense to make sure that our interface design decisions would make sense for a k8s based deploy (just as an exercise.)

Having built and operated several k8s clusters the hard way from scratch, I'd be very much against using k8s for any of this testnet infra. I'd say we'd need a really good justification to even think of supporting k8s, and I don't see one at present that provides any advantage over a DO or AWS-based approach (a DO-based approach can be far more easily translated to AWS).

As a funky idea, could you just have these methods take in an array of Nodes as opposed to just a single one. Then you could provision them together

Actually, looking at it again now, there are 3 distinct categories of operations in use here: ...

I think the distinction makes a bit more sense, although in the case of docker-compose the start+provision step are intermixed at the moment. Looks like the API has been updated to reflect that, which seems right.

The zero value of this type isn't useful so I would make it private.

This only really needs the dir from testnet, we can just pass only that through instead of the whole testnet. IMO, this is a readability improvement because it makes it easier to tell exactly what's needed by the function.

Perhaps I'm not seeing it, but can we set docker as the default value of this flag so it's used even if the flag is not set?

It's already set as the default value here: https://github.com/tendermint/tendermint/pull/8754/files#diff-e41cdc82eb2fc4213d243e0b7d7a1e30ead913c2416e84c9a2633e322cbad72bR161