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

Alloc Health should take Group Checks into Consideration #7375

Closed
schmichael opened this issue Mar 17, 2020 · 3 comments · Fixed by #7383
Closed

Alloc Health should take Group Checks into Consideration #7375

schmichael opened this issue Mar 17, 2020 · 3 comments · Fixed by #7383

Comments

@schmichael
Copy link
Member

This code comment is incorrect:

//TODO(schmichael) for now skip unknown tasks as
//they're task group services which don't currently
//support checks anyway
if v, ok := t.taskHealth[task]; ok {
v.state = state
}

Group checks are supported and should be taken into consideration when setting allocation health.

@notnoop
Copy link
Contributor

notnoop commented Mar 20, 2020

I have tested the code and it looks we properly account for service check health.

The group checks get presented as associated with a "fake" task with the group name as the task name.

The relevant bits are:
[1]

return h.consulClient.RegisterWorkload(services)

[2]
func (h *groupServiceHook) getWorkloadServices() *agentconsul.WorkloadServices {
// Interpolate with the task's environment
interpolatedServices := taskenv.InterpolateServices(h.taskEnvBuilder.Build(), h.services)
// Create task services struct with request's driver metadata
return &agentconsul.WorkloadServices{
AllocID: h.allocID,
Group: h.group,
Restarter: h.restarter,
Services: interpolatedServices,
DriverNetwork: h.driverNet(),
Networks: h.networks,
Canary: h.canary,
}
}

[3]
c.addRegistrations(workload.AllocID, workload.Name(), t)

[4]
func (w *WorkloadServices) Name() string {
if w.Task != "" {
return w.Task
}
return "group-" + w.Group
}

Not sure I fully understand the TODO in code referenced in the issue. Group services don't have task health associated with them. But should they? Should we track the envoy/sidecar containers as task health? Task health differ from checks as they simply check if the task is running.

@notnoop
Copy link
Contributor

notnoop commented Mar 20, 2020

Also, we probably should document that consul registrations may contain synthetic tasks not present in job description - it'd be an easy mistake to make - like I've done in https://github.com/hashicorp/nomad/pull/7366/files .

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 120 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.

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

Successfully merging a pull request may close this issue.

2 participants