Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e2e: deploy Nomad from testing framework #6949

Merged
merged 1 commit into from
Jan 21, 2020

Conversation

tgross
Copy link
Member

@tgross tgross commented Jan 16, 2020

The e2e framework instantiates clients for Nomad/Consul but the provisioning of the actual Nomad cluster is left to Terraform. The Terraform provisioning process uses remote-exec to deploy specific versions of Nomad so that we don't have to bake an AMI every time we want to test a new version. But Terraform treats the resulting instances as immutable, so we can't use the same tooling to update the version of Nomad in-place. This is a prerequisite for upgrade testing.

This changeset extends the e2e framework to provide the option of deploying Nomad (and, in the future, Consul/Vault) with specific versions to running infrastructure. This initial implementation is focused on deploying to a single cluster via ssh (because that's our current need), but provides interfaces to hook the test run at the start of the run, the start of each suite, or the start of a given test case.

The input JSON for configuring provisioning targets can be generated by Terraform (or written by hand). That work is #6948

e2e/e2e_test.go Outdated Show resolved Hide resolved
// TODO: these override each other. local_file > sha > version
// but we should assert at most 1 is set.
var fProvisionNomadLocalBinary = flag.String("nomad.local_file", "",
"provision this specific local binary of Nomad (ignored for no-op provisioning).")
Copy link
Member

Choose a reason for hiding this comment

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

:D

@@ -50,6 +116,10 @@ type Environment struct {

// New creates a Framework
func New() *Framework {
flag.Parse()
Copy link
Member

@shoenig shoenig Jan 17, 2020

Choose a reason for hiding this comment

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

Our flag parsing is pretty complex (and there are TODOs to add more), it might be time to create a flag parser (which we can test), call parse on that and provide a resulting config object into New. Just a thought for later

Name: name,
ExpectConsul: s.Consul,
ExpectVault: s.Vault,
})
if err != nil {
return false, fmt.Errorf("could not provision cluster for case: %v", err)
}
defer f.provisioner.DestroyCluster(info.ID)
defer f.provisioner.TearDownTestCase(t, info.ID)
Copy link
Member

Choose a reason for hiding this comment

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

defer inside a loop is often a bug (and linters warn against it) - previously DestroyCluster was a noop and so it didn't really matter, but now I'm thinking we actually want to tear down each test case after it runs, otherwise I think we're putting the provisioned cluster into a kind of accumulated state as each Case runs

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, you're totally right. Will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in aa3b15d. I want to make sure we're still defering the teardown so that t.Fatal doesn't prevent the teardown step from happening, so I factored this inner loop out into a runCase function so that we run the deferred teardown at the end of that scope.

if err != nil && opts.ExpectConsul {
return nil, err
if err != nil {
return nil, fmt.Errorf("expected Consul: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

i really like errors.Wrapf(err, "expected Consul") (from github.com/pkg/errors)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong opinion on it, but it looks like we've avoided that in all but one spot in the Nomad codebase, so I'd like to keep the existing idiom if we can? IIRC there's some cool new error wrapping features in a newer golang, so maybe there's a codebase-wide improvement to be had once we get bumped to the right minimum version.

The e2e framework instantiates clients for Nomad/Consul but the
provisioning of the actual Nomad cluster is left to Terraform. The
Terraform provisioning process uses `remote-exec` to deploy specific
versions of Nomad so that we don't have to bake an AMI every time we
want to test a new version. But Terraform treats the resulting
instances as immutable, so we can't use the same tooling to update the
version of Nomad in-place. This is a prerequisite for upgrade testing.

This changeset extends the e2e framework to provide the option of
deploying Nomad (and, in the future, Consul/Vault) with specific
versions to running infrastructure. This initial implementation is
focused on deploying to a single cluster via `ssh` (because that's our
current need), but provides interfaces to hook the test run at the
start of the run, the start of each suite, or the start of a given
test case.

The input JSON for configuring provisioning targets can be generated
by Terraform (or written by hand). That will be under a separate PR.
@tgross
Copy link
Member Author

tgross commented Jan 21, 2020

I've rebased this and the target branch off master and did a full end-to-end test (including Windows) with #6948. Once tests pass I'll merge this and then open a PR with the combined branch vs master.

@tgross tgross merged commit 047fc42 into e2e_provisioning_rework Jan 21, 2020
@tgross tgross deleted the e2e_framework_updates branch January 21, 2020 21:38
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants