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

connect: deployments should wait for Connect sidecar checks #19334

Merged
merged 2 commits into from
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 3 additions & 0 deletions .changelog/19334.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
connect: Fixed a bug where deployments would not wait for Connect sidecar task health checks to pass
```
17 changes: 17 additions & 0 deletions client/allochealth/tracker.go
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,8 @@ func evaluateConsulChecks(services []*structs.Service, registrations *servicereg
}
}

// The sidecar service and checks are created when the service is
// registered, not on job registration, so they won't appear in the jobspec.
if !maps.Equal(expChecks, regChecks) {
return false
}
Expand All @@ -595,6 +597,21 @@ func evaluateConsulChecks(services []*structs.Service, registrations *servicereg
}
}
}

// Check the health of Connect sidecars, so we don't accidentally
// mark an allocation healthy before min_healthy_time. These don't
// currently support on_update.
if service.SidecarService != nil {
for _, check := range service.SidecarChecks {
switch check.Status {
case api.HealthWarning:
return false
case api.HealthCritical:
return false
}
}
}

}
}

Expand Down
36 changes: 36 additions & 0 deletions client/allochealth/tracker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1466,6 +1466,42 @@ func TestTracker_evaluateConsulChecks(t *testing.T) {
},
},
},
{
name: "failing sidecar checks only",
exp: false,
tg: &structs.TaskGroup{
Services: []*structs.Service{{
Name: "group-s1",
Checks: []*structs.ServiceCheck{
{Name: "c1"},
},
}},
},
registrations: &serviceregistration.AllocRegistration{
Tasks: map[string]*serviceregistration.ServiceRegistrations{
"group": {
Services: map[string]*serviceregistration.ServiceRegistration{
"abc123": {
ServiceID: "abc123",
Checks: []*consulapi.AgentCheck{
{
Name: "c1",
Status: consulapi.HealthPassing,
},
},
SidecarService: &consulapi.AgentService{},
SidecarChecks: []*consulapi.AgentCheck{
{
Name: "sidecar-check",
Status: consulapi.HealthCritical,
},
},
},
},
},
},
},
},
}

for _, tc := range cases {
Expand Down
6 changes: 6 additions & 0 deletions client/serviceregistration/service_registration.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,12 @@ type ServiceRegistration struct {

// Checks is the status of the registered checks.
Checks []*api.AgentCheck

// SidecarService is the AgentService registered in Consul for any Connect sidecar
SidecarService *api.AgentService

// SidecarChecks is the status of the registered checks for any Connect sidecar
SidecarChecks []*api.AgentCheck
}

func (s *ServiceRegistration) copy() *ServiceRegistration {
Expand Down
9 changes: 9 additions & 0 deletions command/agent/consul/service_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -1760,6 +1760,15 @@ func (c *ServiceClient) AllocRegistrations(allocID string) (*serviceregistration
sreg.Checks = append(sreg.Checks, check)
}
}

if sidecarService := services[serviceID+sidecarSuffix]; sidecarService != nil {
tgross marked this conversation as resolved.
Show resolved Hide resolved
sreg.SidecarService = sidecarService
for _, check := range checks {
if check.ServiceID == sidecarService.ID {
sreg.SidecarChecks = append(sreg.SidecarChecks, check)
}
}
}
}
}

Expand Down