allocrunner: fix health check monitoring for Consul services

[lgfa29]

Services must be canonicalized and have its values interpolated before comparing with the values returned by Consul.

Close #16382
Close #16307

[lgfa29]

After all my failed attempts to refactor the service interpolation so it's closer to the Service struct I decided to take a step back and understand this part of the code better.

Then I realized that my initial implementation was quite wrong. As the name suggests, each TaskEnv holds information for a specific task (or nothing task-related for a group). Since allocHealthWatcherHook is an alloc runner hook we actually need one TaskEnv per task (+1 for the group).

allocHealthWatcherHook also implements the Update() hook, so the allocation may change while the hook is active, in which case we need to update the allocation in the taskenv.Builder and recreate the TaskEnv.

And since taskenv.Builder mutates itself in several different ways, we can't share instances across hooks. taskenv.Builder also mutate themselves in ways that cannot be undone (for example, calling UpdateAlloc(alloc, nil) will not reset the task to empty, it will just ignore the change).

So allocHealthWatcherHook now takes a taskenv.Builder factory to creates a fresh builder when the allocation updates to prevent spurious task information at the group level TaskEnv.

[shoenig]

LGTM; just one suggestion you could ignore. my thinking is we want folks to complain about bugs if we are validating / canonicalizing things incorrectly. if we try to do the right thing they may just ignore what is otherwise a serious bug

[shoenig]

we [should] already guard against this in Validate/Canonicalize; instead of trying to make things maybe work in an unexpected situation I think we should just do nothing, e.g.

if env != nil {
 // assign
} else {
  logger.Error("you found a bug!")
}

[lgfa29]

Hum...good point. Changed to this in 925d347

[tgross]

👍