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

provider/nomad: Nomad provider for managing jobs #9538

Merged
merged 3 commits into from
Nov 10, 2016
Merged

Conversation

mitchellh
Copy link
Contributor

This adds a new provider nomad with a single resource nomad_job that can be used for managing jobs within a Nomad cluster.

The primary purpose for this provider is to manage system tasks or always-running service tasks that are used as part of cluster bootstrapping. For example, monitoring systems, core services, etc. Other jobs are usually submitted directly to Nomad (such as basic app deployment).

This is documented and tested (unit + acceptance). I rebased the commits so you can look at the provider/nomad commit to just see the provider separate from the vendoring.

Finally, this provider is built to work with Nomad 0.4.1. It likely won't work for earlier versions.

// How to run the acceptance tests for this provider:
//
// - Obtain an official Nomad release from https://nomadproject.io
// and extra "nomad" somewhere
Copy link
Contributor

Choose a reason for hiding this comment

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

extract? extract the nomad binary perhaps?

log.Printf("[DEBUG] Deregistering job: %q", prevId)
_, _, err := client.Jobs().Deregister(prevId, nil)
if err != nil {
return fmt.Errorf("error deregistering previous job %q: %s", prevId, err)
Copy link
Contributor

@jamtur01 jamtur01 Oct 24, 2016

Choose a reason for hiding this comment

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

previous feels odd here. error deregistering prior job would work? Or existing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want the user to be confused why we may be deregistering a job "foo" when the jobspec perhaps has job "bar". I've updated the wording to be a bit more explicit about that, pushing soon.


The following arguments are supported:

* `address` - (Optional) The HTTP(S) API address of the agent to use. Defaults to "127.0.0.1:4646". `NOMAD_ADDR` can also be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

The HTTP(S) API address of the Nomad agent to use - make it explicit to make it consistent.

I'd probably also: 127.0.0.1:4646.


The following arguments are supported:

* `address` - (Optional) The HTTP(S) API address of the agent to use. Defaults to "127.0.0.1:4646". `NOMAD_ADDR` can also be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

The "NOMAD_ADDR" environment can also be used.? WIth NOMAD_ADDR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto with the NOMAD_REGION line.

Manages a job registered in Nomad.

This can be used to initialize your cluster with system jobs, common services,
and more. In day to day Nomad use it is common for developers to submit
Copy link
Contributor

Choose a reason for hiding this comment

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

The second sentence doesn't make sense to me? I think I'm either missing a Nomad concept or it's a little convoluted?


```
resource "nomad_job" "app" {
jobspec = "${file("${path.module}/job.hcl")}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt anyone is ever going to write a whole jobspec inline but maybe an example of a jobspec somewhere so folks know that it is they are looking for?

@@ -11,7 +11,7 @@ import (
// How to run the acceptance tests for this provider:
//
// - Obtain an official Nomad release from https://nomadproject.io
// and extra "nomad" somewhere
// and extra the "nomad" binary
Copy link
Contributor

Choose a reason for hiding this comment

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

Still missing the t :)

@mitchellh
Copy link
Contributor Author

mitchellh commented Oct 24, 2016

@jamtur01 Thanks! Addressed feedback (and fixed that typo), while adding more examples.

Apologies for the rebased push, I wouldn't normally do that but the history became a mess since I was missing dependencies. I'll address future feedback in normal commits and only rebase prior to merging.

@jamtur01
Copy link
Contributor

@mitchellh Very neat BTW! Does << EOT blah EOT work in HCL? I had no idea. I wonder if that's documented anywhere? I'll go look. :)

@mitchellh
Copy link
Contributor Author

@jamtur01 Yes it does! You can even see it in "action" in the acceptance tests for this provider. It may not be documented actually... but I can do that separate from this PR.

@jamtur01
Copy link
Contributor

@mitchellh It's actually in the syntax docs already. Had just missed it! Thanks.

@jen20
Copy link
Contributor

jen20 commented Oct 24, 2016

@jamtur01 Yes - so does <<- to remove hanging indent (à la Ruby)

@apparentlymart
Copy link
Contributor

I was looking at #8769 primarily so that Terraform could interface with Nomad in a way that doesn't defeat Nomad's own diffing mechanism.

Certainly not meaning to suggest that this has to be blocked by #8769, but just wanted to point it out in case it helps to think about 1) whether such behavior is desirable anyway, and 2) assuming yes, whether adding it later would require a breaking change here that might make it worth tweaking the design now. (I think probably not, but I'm not 100% sure...)

@mitchellh
Copy link
Contributor Author

@apparentlymart I think in Nomad's cause that would be nice but doesn't block it since Nomad is idempotent. If you make a stylistic change that causes a plan to update the job, Nomad won't actually do anything once the job is resubmitted. I think that makes this okay.

@jamtur01
Copy link
Contributor

@jen20 Do you mean <<~ (that removes hanging indent in Ruby - <<- preserves whitespace.

@mitchellh mitchellh merged commit fd498fb into master Nov 10, 2016
@mitchellh mitchellh deleted the f-nomad-provider branch November 10, 2016 02:34
@ghost
Copy link

ghost commented Apr 20, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants