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

job: add job submission on register #405

Merged
merged 5 commits into from
Dec 19, 2023
Merged

job: add job submission on register #405

merged 5 commits into from
Dec 19, 2023

Conversation

lgfa29
Copy link
Contributor

@lgfa29 lgfa29 commented Dec 14, 2023

Send job submission on register to retain original job source.

Closes #392

lgfa29 added a commit that referenced this pull request Dec 14, 2023
lgfa29 added a commit that referenced this pull request Dec 15, 2023
Base automatically changed from deps-update-nomad-1-7 to main December 15, 2023 17:07
lgfa29 added a commit that referenced this pull request Dec 15, 2023
@lgfa29 lgfa29 requested a review from jrasell December 15, 2023 17:10
lgfa29 added a commit that referenced this pull request Dec 15, 2023
lgfa29 added a commit that referenced this pull request Dec 16, 2023
@lgfa29 lgfa29 requested a review from gulducat December 18, 2023 22:56
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

Couple comments regarding diff detection to consider, but overall LGTM!

log.Printf("[WARN] failed to read job submission: %v", err)
} else if sub != nil {
if sub.Source != "" {
d.Set("jobspec", sub.Source)
Copy link
Member

Choose a reason for hiding this comment

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

To clarify the implication here, considering your recent comment on issue #1 -- This means that if the jobspec HCL is updated via UI or CLI (or appropriately crafted API call) outside of TF, this will pull that down for comparison, so remote HCL changes can be detected that the current jobspecEqual() might otherwise miss.

It does not affect what Nomad considers to be a "change" (as with nomad plan), like if semantically unimportant things (like hcl comments) change, then there still would be no change detected, as is the current state, but if there is some meaningful change, then it may be better detected and compared with this new ability.

Do I have that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so remote HCL changes can be detected that the current jobspecEqual() might otherwise miss.

Yes, currently jobspecEqual() doesn't even detect any remote changes because, up until now, it didn't have access to the remote jobspec.

if semantically unimportant things (like hcl comments) change, then there still would be no change detected

Right, but that's not because of this change, this is because jobspecEqual() does a semantic comparison, meaning, it parses the raw string and compares the result job object, so changes like comments and formatting are ignored.

func jobspecEqual(k, old, new string, d ResourceFieldGetter) bool {
var oldJob *api.Job
var newJob *api.Job
var oldErr error
var newErr error
// Read job parsing config.
jobParserConfig, err := parseJobParserConfig(d)
if err != nil {
log.Printf("[ERROR] %v", err)
return false
}
switch {
case jobParserConfig.JSON.Enabled:
oldJob, oldErr = parseJSONJobspec(old)
newJob, newErr = parseJSONJobspec(new)
case jobParserConfig.HCL1.Enabled:
oldJob, oldErr = jobspec.Parse(strings.NewReader(old))
newJob, newErr = jobspec.Parse(strings.NewReader(new))
default:
oldJob, oldErr = parseHCL2Jobspec(old, jobParserConfig.HCL2)
newJob, newErr = parseHCL2Jobspec(new, jobParserConfig.HCL2)
}
if oldErr != nil {
log.Println("error parsing old jobspec")
log.Printf("%v\n", oldJob)
log.Printf("%v", oldErr)
return false
}
if newErr != nil {
log.Println("error parsing new jobspec")
log.Printf("%v\n", newJob)
log.Printf("%v", newErr)
return false
}
// Init
oldJob.Canonicalize()
newJob.Canonicalize()
// Check for jobspec equality
return reflect.DeepEqual(oldJob, newJob)
}

Comment on lines 661 to 693
// Update HCL2 variables if present.
hcl2, ok := d.GetOk("hcl2")
Copy link
Member

Choose a reason for hiding this comment

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

If I do not have hcl2{} in my TF resource, but I then manually (UI or CLI) update the job with a variable value, TF will see "no changes."

Put differently, only if my nomad_job resource has hcl2{} in it (per this d.GetOk()) will TF state contain anything about variables (per d.Set() below), otherwise remote changes are invisible to TF.

Maybe that's an intended feature? but is a little surprising to me.

Copy link
Contributor Author

@lgfa29 lgfa29 Dec 19, 2023

Choose a reason for hiding this comment

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

That's right, but I kind of did that on purpose to avoid changes to state as much as possible. If you're not using HCL2, you probably have a reason for that, so switching to it outside of Terraform means you're doing something funky 😛

But if that's surprising then 070092d forces updates if variables change. Is this better?

If available, use the job submission source to detect changes to
`jobspec`. This can mitigate drift detection problems such as #1.
Read HCL2 variables from job submission even if the `nomad_job` resource
does not speify an `hcl2` block.
@lgfa29 lgfa29 merged commit 6788d0a into main Dec 19, 2023
3 checks passed
@lgfa29 lgfa29 deleted the f-submit-hcl2 branch December 19, 2023 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support the HCL source when submitting the job
2 participants