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.

Sign up for GitHub

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

Jump to bottom

health: detect missing task checks #7366

Closed
wants to merge 3 commits into from
Closed

health: detect missing task checks #7366

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b9e3e12
tests: add a check for failing service checks
Mar 17, 2020
0047882
health: detect missing task checks
Mar 17, 2020
a263a4e
more targetted health tests
Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions client/allochealth/tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,10 +372,14 @@ OUTER:
// Detect if all the checks are passing
passed := true

foundChecks := 0

CHECKS:
for _, treg := range allocReg.Tasks {
for _, sreg := range treg.Services {
for _, check := range sreg.Checks {
foundChecks++

if check.Status == api.HealthPassing {
continue
}
Expand All @@ -387,6 +391,13 @@ OUTER:
}
}

if foundChecks < t.consulCheckCount {
// didn't see all the checks we expect, task might be slow to start
// or restarting tasks that had their service hooks deregistered
passed = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this logic work when it is a migration for a batch job:

nomad/client/allocrunner/health_hook.go

Line 253 in c346a47

minHealthyTime = strategy.MinHealthyTime
?

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 believe so - the assumption this depends on that checks are unique and that job definition has a 1 to 1 mapping with consul agent check registration, regardless of exact parameter or minHealthyTime. This needs to be tweaked for task group level checks, but will address that in a follow up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry should have added more for my thinking. If it is a batch job, the tasks can finish and then the number of checks that will be registered in Consul will be less than those defined in the job spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah - excellent point. Going to drawing board a bit to think this through a bit more. Not sure if I fully understand expectations of service health checks in batch - but it's definitely a problem for service jobs with init containers with task lifecycle/dependencies features. Thanks for raising it.

Copy link
Contributor Author

@notnoop notnoop Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Checked - and looks like batch jobs don't support migrate or update/deployments [1] - do you know when this code is relevant for batch by any chance?

[1] https://github.com/hashicorp/nomad/blob/v0.10.4/nomad/structs/structs.go#L4997-L5021

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry for the noise! Thought batch supported migrate!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) You helped found a bug though - addressed it with a different approach in #7383 .

t.setCheckHealth(false)
}

if !passed {
// Reset the timer since we have transitioned back to unhealthy
if primed {
Expand Down
179 changes: 177 additions & 2 deletions client/allocrunner/health_hook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,9 @@ func TestHealthHook_Postrun(t *testing.T) {
require.NoError(h.Postrun())
}

// TestHealthHook_SetHealth asserts SetHealth is called when health status is
// TestHealthHook_SetHealth_healthy asserts SetHealth is called when health status is
// set. Uses task state and health checks.
func TestHealthHook_SetHealth(t *testing.T) {
func TestHealthHook_SetHealth_healthy(t *testing.T) {
t.Parallel()
require := require.New(t)

Expand Down Expand Up @@ -300,6 +300,181 @@ func TestHealthHook_SetHealth(t *testing.T) {
require.NoError(h.Postrun())
}

// TestHealthHook_SetHealth_unhealthy asserts SetHealth notices unhealthy allocs
func TestHealthHook_SetHealth_unhealthy(t *testing.T) {
t.Parallel()
require := require.New(t)

alloc := mock.Alloc()
alloc.Job.TaskGroups[0].Migrate.MinHealthyTime = 1 // let's speed things up
task := alloc.Job.TaskGroups[0].Tasks[0]

newCheck := task.Services[0].Checks[0].Copy()
newCheck.Name = "failing-check"
task.Services[0].Checks = append(task.Services[0].Checks, newCheck)

// Synthesize running alloc and tasks
alloc.ClientStatus = structs.AllocClientStatusRunning
alloc.TaskStates = map[string]*structs.TaskState{
task.Name: {
State: structs.TaskStateRunning,
StartedAt: time.Now(),
},
}

// Make Consul response
checkHealthy := &consulapi.AgentCheck{
Name: task.Services[0].Checks[0].Name,
Status: consulapi.HealthPassing,
}
checksUnhealthy := &consulapi.AgentCheck{
Name :task.Services[0].Checks[1].Name,
Status: consulapi.HealthCritical,
}
taskRegs := map[string]*agentconsul.ServiceRegistrations{
task.Name: {
Services: map[string]*agentconsul.ServiceRegistration{
task.Services[0].Name: {
Service: &consulapi.AgentService{
ID: "foo",
Service: task.Services[0].Name,
},
Checks: []*consulapi.AgentCheck{checkHealthy, checksUnhealthy},
},
},
},
}

logger := testlog.HCLogger(t)
b := cstructs.NewAllocBroadcaster(logger)
defer b.Close()

// Don't reply on the first call
called := false
consul := consul.NewMockConsulServiceClient(t, logger)
consul.AllocRegistrationsFn = func(string) (*agentconsul.AllocRegistration, error) {
if !called {
called = true
return nil, nil
}

reg := &agentconsul.AllocRegistration{
Tasks: taskRegs,
}

return reg, nil
}

hs := newMockHealthSetter()

h := newAllocHealthWatcherHook(logger, alloc.Copy(), hs, b.Listen(), consul).(*allocHealthWatcherHook)

// Prerun
require.NoError(h.Prerun())

// Wait to ensure we don't get a healthy status
select {
case <-time.After(5 * time.Second):
// great no healthy status
case health := <-hs.healthCh:
require.False(health.healthy)

// Unhealthy allocs shouldn't emit task events
ev := health.taskEvents[task.Name]
require.NotNilf(ev, "%#v", health.taskEvents)
Copy link
Member

Choose a reason for hiding this comment

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

The comment says "unhealthy alloc shouldn't emit task events" yet the assertion is non-nil.

I'm also a little confused why both cases (timeout and health=false) seem valid? Is this test non-deterministic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry - This case case never actually gets exercised - it's dead code, I was trying to get some error messages. will remove it.

}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We seem to lack tests for health check logic for failure and it's not obvious to me how to effectively test this quickly without changing many interfaces :(. Waiting 5 seconds is pretty long. Suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Setting HealthyDeadline=2 * time.Second might be safe and tighten things up a little bit. Not sure how else to test this without extensive changes. t.Parallel should help us out a lot here, but 5s minimum would be unfortunate.

Copy link
Member

Choose a reason for hiding this comment

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

Also testing in the allochealth package may allow peeking at internals in a time-independent way? I'm not sure after a quick peek, but the deadline is only taken into account in this package.

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 added a test in allochealth that's not so time sensitive and lowered the tests here to 2s.


// Postrun
require.NoError(h.Postrun())
}

// TestHealthHook_SetHealth_missingchecks asserts SetHealth recovers from
// missing checks
func TestHealthHook_SetHealth_missingchecks(t *testing.T) {
t.Parallel()
require := require.New(t)

alloc := mock.Alloc()
alloc.Job.TaskGroups[0].Migrate.MinHealthyTime = 1 // let's speed things up
task := alloc.Job.TaskGroups[0].Tasks[0]

newCheck := task.Services[0].Checks[0].Copy()
newCheck.Name = "failing-check"
task.Services[0].Checks = append(task.Services[0].Checks, newCheck)

// Synthesize running alloc and tasks
alloc.ClientStatus = structs.AllocClientStatusRunning
alloc.TaskStates = map[string]*structs.TaskState{
task.Name: {
State: structs.TaskStateRunning,
StartedAt: time.Now(),
},
}

// Make Consul response
checkHealthy := &consulapi.AgentCheck{
Name: task.Services[0].Checks[0].Name,
Status: consulapi.HealthPassing,
}
taskRegs := map[string]*agentconsul.ServiceRegistrations{
task.Name: {
Services: map[string]*agentconsul.ServiceRegistration{
task.Services[0].Name: {
Service: &consulapi.AgentService{
ID: "foo",
Service: task.Services[0].Name,
},
// notice missing check
Checks: []*consulapi.AgentCheck{checkHealthy },
},
},
},
}

logger := testlog.HCLogger(t)
b := cstructs.NewAllocBroadcaster(logger)
defer b.Close()

// Don't reply on the first call
called := false
consul := consul.NewMockConsulServiceClient(t, logger)
consul.AllocRegistrationsFn = func(string) (*agentconsul.AllocRegistration, error) {
if !called {
called = true
return nil, nil
}

reg := &agentconsul.AllocRegistration{
Tasks: taskRegs,
}

return reg, nil
}

hs := newMockHealthSetter()

h := newAllocHealthWatcherHook(logger, alloc.Copy(), hs, b.Listen(), consul).(*allocHealthWatcherHook)

// Prerun
require.NoError(h.Prerun())

// Wait to ensure we don't get a healthy status
select {
case <-time.After(5 * time.Second):
// great no healthy status
case health := <-hs.healthCh:
require.False(health.healthy)

// Unhealthy allocs shouldn't emit task events
ev := health.taskEvents[task.Name]
require.NotNilf(ev, "%#v", health.taskEvents)
}

// Postrun
require.NoError(h.Postrun())
}


// TestHealthHook_SystemNoop asserts that system jobs return the noop tracker.
func TestHealthHook_SystemNoop(t *testing.T) {
t.Parallel()
Expand Down