stale allocation data leads to incorrect (and even negative) metrics

[cgbaker]

client: was not using up-to-date client state in determining which alloc count towards allocated resources

this is the second issue where using AllocRunner.Alloc() instead of AllocRunner.AllocState() meant that the ClientState had the wrong data. @schmichael previously noted how nefarious this is, that we should look for a solution. i haven't done anything to address the underlying issue with this PR; I've just made the simplest fix to correct the metrics calculation. in fact, the call to AllocState() may be a little more expensive than is necessary for this; we only need to know whether the allocation is still resident.

fixes #5570

[cgbaker]

per the comment above about using AllocState(), could maybe get the info we need like this instead:

nomad/client/allocrunner/alloc_runner.go

Lines 541 to 547 in e964c7f

	// Compute the ClientStatus
	if ar.state.ClientStatus != "" {
	// The client status is being forced
	a.ClientStatus, a.ClientDescription = ar.state.ClientStatus, ar.state.ClientDescription
	} else {
	a.ClientStatus, a.ClientDescription = getClientStatus(taskStates)
	}

[schmichael]

I don't think this is safe as you're modifying ar.alloc. I think you need to copy it first.

This also gives allAllocs pretty special behavior. It returns a view of each alloc not available anywhere else. At a minimum we should comment this unusual special case, but since there only appears to be a single caller I think we should probably scrap this method entirely and inline the behavior in the caller (or write a one-off function we can unit test).

[cgbaker]

agreed with the criticism and the suggestion.

[cgbaker]

@schmichael , updated as suggested.

https://github.com/hashicorp/nomad/pull/5637/files#diff-1cd41a2611da49ce63630d5a51bd82c5R2714

[notnoop]

Good catch! Thanks for digging into this!

[langmartin]

lgtm, fwiw

[langmartin]

I figured out right here why you were doubling the resource settings through this test, and I like it. Maybe worth a comment up near the top just to draw attention to it?

[cgbaker]

i actually started to write a comment and it felt like i was ruining the joke ;)

[schmichael]

Such unfortunately subtle code. Thanks for cleaning this up!

[github-actions]

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.