e2e provider is extended with Disconnect, Reconnect and CheckUpgraded
#852
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| e2e "github.com/cometbft/cometbft/test/e2e/pkg" | ||
|
|
@@ -24,33 +25,63 @@ func (p *Provider) Setup() error { | |
| return nil | ||
| } | ||
|
|
||
| var ymlPlaybookSeq int | ||
|
|
||
| func getNextPlaybookFilename() string { | ||
| const ymlPlaybookAction = "playbook-action" | ||
| ymlPlaybookSeq++ | ||
| return ymlPlaybookAction + strconv.Itoa(ymlPlaybookSeq) + ".yml" | ||
| } | ||
|
|
||
| func (p Provider) StartNodes(ctx context.Context, nodes ...*e2e.Node) error { | ||
| nodeIPs := make([]string, len(nodes)) | ||
| for i, n := range nodes { | ||
| nodeIPs[i] = n.ExternalIP.String() | ||
| } | ||
| playbook := ansibleSystemdBytes(true) | ||
| playbookFile := getNextPlaybookFilename() | ||
| if err := p.writePlaybook(playbookFile, playbook); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return execAnsible(ctx, p.Testnet.Dir, playbookFile, nodeIPs) | ||
| } | ||
| func (p Provider) StopTestnet(ctx context.Context) error { | ||
| nodeIPs := make([]string, len(p.Testnet.Nodes)) | ||
| for i, n := range p.Testnet.Nodes { | ||
| nodeIPs[i] = n.ExternalIP.String() | ||
| } | ||
|
|
||
| playbook := ansibleSystemdBytes(false) | ||
| playbookFile := getNextPlaybookFilename() | ||
| if err := p.writePlaybook(playbookFile, playbook); err != nil { | ||
| return err | ||
| } | ||
| return execAnsible(ctx, p.Testnet.Dir, playbookFile, nodeIPs) | ||
| } | ||
| func (p Provider) Disconnect(ctx context.Context, _ string, ip string) error { | ||
| playbook := ansiblePerturbConnectionBytes(true) | ||
| playbookFile := getNextPlaybookFilename() | ||
| if err := p.writePlaybook(playbookFile, playbook); err != nil { | ||
| return err | ||
| } | ||
| return execAnsible(ctx, p.Testnet.Dir, playbookFile, []string{ip}) | ||
| } | ||
| func (p Provider) Reconnect(ctx context.Context, _ string, ip string) error { | ||
| playbook := ansiblePerturbConnectionBytes(false) | ||
| playbookFile := getNextPlaybookFilename() | ||
| if err := p.writePlaybook(playbookFile, playbook); err != nil { | ||
| return err | ||
| } | ||
| return execAnsible(ctx, p.Testnet.Dir, playbookFile, []string{ip}) | ||
| } | ||
|
|
||
| func (p Provider) CheckUpgraded(_ context.Context, node *e2e.Node) (string, bool, error) { | ||
| // Upgrade not supported yet by DO provider | ||
| return node.Name, false, nil | ||
| } | ||
|
|
||
| func (p Provider) writePlaybook(yaml, playbook string) error { | ||
| //nolint: gosec | ||
| // G306: Expect WriteFile permissions to be 0600 or less | ||
| err := os.WriteFile(filepath.Join(p.Testnet.Dir, yaml), []byte(playbook), 0o644) | ||
|
|
@@ -60,25 +91,58 @@ func (p Provider) writePlaybook(yaml string, starting bool) error { | |
| return nil | ||
| } | ||
|
|
||
| const basePlaybook = `- name: e2e custom playbook | ||
|
There was a problem hiding this comment. While I'm pretty confident that this would work, it is rather difficult to know what the final playbook for each provider method would look like (making it more difficult to debug in case something goes wrong). Would it not make it more readable to define a template per provider method (e.g. one for
6D40
There was a problem hiding this comment.
Regarding this, check my comment above Yes, I thought about using the template library, but in the end I decided to go simple. As we add more logic from There was a problem hiding this comment. Interestingly I would consider using the template library to be the simpler option from a coding/debugging perspective. My concern's not so much with the generated code as it is with the Go code and the strings within the Go code. For instance, some of the string concatenations are performed with strings with indentation where that indentation has meaning. It's usually far easier to reason about and extend when the whole template's visible, like with the Docker compose template: cometbft/test/e2e/pkg/infra/docker/docker.go Lines 53 to 117 in dc28d6b For the current code, we need to now know that some strings require indentation with 4 spaces, others with 2 spaces, etc., making it more difficult to debug/extend since you can't see those strings within their intended context. There was a problem hiding this comment. You've got a point with the indentation. Current approach is not the best for that, I agree.
Maybe, a solution that represents a best balance would be to have a template for the beginning, and then other templates, one for each of the different kind of tasks we support (shell, systemd, apt, etc). However, I'd like to defer this to the next round of improvements. |
||
| hosts: all | ||
| gather_facts: yes | ||
| vars: | ||
| ansible_host_key_checking: false | ||
|
|
||
| tasks: | ||
| ` | ||
|
|
||
| func ansibleAddTask(playbook, name, contents string) string { | ||
| return playbook + " - name: " + name + "\n" + contents | ||
| } | ||
|
|
||
| func ansibleAddSystemdTask(playbook string, starting bool) string { | ||
| startStop := "stopped" | ||
| if starting { | ||
| startStop = "started" | ||
| } | ||
| contents := fmt.Sprintf(` ansible.builtin.systemd: | ||
| name: testappd | ||
| state: %s | ||
| enabled: yes`, startStop) | ||
|
|
||
| return ansibleAddTask(pl F438 aybook, "operate on the systemd-unit", contents) | ||
| } | ||
|
|
||
| func ansibleAddShellTasks(playbook, name string, shells ...string) string { | ||
| for _, shell := range shells { | ||
| contents := fmt.Sprintf(" shell: \"%s\"\n", shell) | ||
| playbook = ansibleAddTask(playbook, name, contents) | ||
| } | ||
| return playbook | ||
| } | ||
|
|
||
| // file as bytes to be written out to disk. | ||
| // ansibleStartBytes generates an Ansible playbook to start the network | ||
| func ansibleSystemdBytes(starting bool) string { | ||
| return ansibleAddSystemdTask(basePlaybook, starting) | ||
| } | ||
|
|
||
| func ansiblePerturbConnectionBytes(disconnect bool) string { | ||
| disconnecting := "reconnect" | ||
| op := "-D" | ||
| if disconnect { | ||
| disconnecting = "disconnect" | ||
| op = "-A" | ||
| } | ||
| playbook := basePlaybook | ||
| for _, dir := range []string{"INPUT", "OUTPUT"} { | ||
| playbook = ansibleAddShellTasks(playbook, disconnecting+" node", | ||
| fmt.Sprintf("iptables %s %s -p tcp --dport 26656 -j DROP", op, dir)) | ||
| } | ||
| return playbook | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this so that playbooks don't clash with one another?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for troubleshooting, a late addition to this PR. I wanted to see what actual playbook had been generated, but the runner was overwriting the file with new playbooks.
This way we keep the history, and can inspect.
All these files are part of the "testnet" dir, so they will be wiped out on the next run (or upon cleanup). But, if we see that the number of files is very large for a run (not the case now), we could extend this to only produce, e.g., N files and use them in round-robin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't think we need to optimize this. I was just double-checking my understanding.