keep placed canaries aligned in raft store #6975
Conversation
drewbailey
changed the title
| @@ -3442,6 +3442,20 @@ func (s *StateStore) updateDeploymentWithAlloc(index uint64, alloc, existing *st | |||
| state.HealthyAllocs += healthy | |||
| state.UnhealthyAllocs += unhealthy | |||
|
|
|||
| // Ensure PlacedCanaries accurately reflects the alloc canary status | |||
| if alloc.DeploymentStatus != nil && alloc.DeploymentStatus.Canary { | |||
There was a problem hiding this comment.
Actually, do we need to update UpdateDeploymentPromotion so that canaries that get promoted are removed from PlacedCanaries?
There was a problem hiding this comment.
I don't think so, since UpdateDeploymentPromotion deals with promoting the deployment/canaries I think that the values for Placed shouldn't ever be decremented. If you want to see the deployment status of a previous deployment the value for Placed should be the total that were placed
There was a problem hiding this comment.
Umm.. I'm a bit concerned about the reconciler logic. Currently, handleGroupCanaries returns canaries, by checking PlacedCanaries exclusively. If a canary got promoted, how do we ensure that scheduler no longer treats it as one.
There was a problem hiding this comment.
I believe (and can followup / confirm) Canary promotion only happens when a deployment is promoted. If there is an old deployment with non-promoted canaries, or the deployment is failed, it will use PlacedCanaries that haven't been promoted to be stopped.
// Cancel any non-promoted canaries from the older deployment
if a.oldDeployment != nil {
for _, s := range a.oldDeployment.TaskGroups {
if !s.Promoted {
stop = append(stop, s.PlacedCanaries...)
}
}
}
// Cancel any non-promoted canaries from a failed deployment
if a.deployment != nil && a.deployment.Status == structs.DeploymentStatusFailed {
for _, s := range a.deployment.TaskGroups {
if !s.Promoted {
stop = append(stop, s.PlacedCanaries...)
}
}
}
And then after the deployment is promoted, the promotion eval includes the promoted deployment canaries as canaries, but then further down canaryState will be false
canaryState := dstate != nil && dstate.DesiredCanaries != 0 && !dstate.Promoted
Which is used in computeStop to stop the previous version non canary allocs. I think after all of this, the deploy is marked as successful, and even though placed canaries stays populated, we check deployment status and no new canaries are ever placed until a new deployment
drewbailey
force-pushed
the
b-update-placed-canaries
branch
from
9d86337
to
0651341
Compare
| @@ -529,10 +529,6 @@ func (s *GenericScheduler) computePlacements(destructive, place []placementResul | |||
| // If we are placing a canary and we found a match, add the canary | |||
| // to the deployment state object and mark it as a canary. | |||
| if missing.Canary() && s.deployment != nil { | |||
| if state, ok := s.deployment.TaskGroups[tg.Name]; ok { | |||
| state.PlacedCanaries = append(state.PlacedCanaries, alloc.ID) | |||
| } | |||
There was a problem hiding this comment.
I'm unclear why an inplace update is inappropriate or the source of a bug here. It's hard to follow this flow, but s.deployment is set by the reconciler which creates a copy of the deployment:
Line 170 in f5441e6
This seems like an intentional decision to make deployments safe for inplace updates, and it appears s.deployment == s.Plan.Deployment which means it will get submitted along with the plan.
If that's not the case I'd love to see a little more cleanup or commenting in the code to help future developers. Maybe an explicit comment on the s.deployment field about its mutability, and removing the deployment.Copy() if it is immutable.
There was a problem hiding this comment.
So there is definitely some misdirection going on with all these deployments I'm going to outline my understanding of them and then see about commenting or cleaning up.
but s.deployment is set by the reconciler which creates a copy of the deployment
The scheduler deployment is actually not set by the reconciler, the reconciler copies the scheduler deployment to its own deployment and uses it as a readonly reference to the current state of the deployment and returns a results.deployment and results.deploymentUpdates
https://github.com/hashicorp/nomad/blob/master/scheduler/generic_sched.go#L355-L357
Currently the reconciler will only return a results.deployment if the deployment is newly created, otherwise it will just be deploymentUpdates.
So the following logic is mutating state that never gets written to the raft log because only changes on s.plan are ultimately submitted, in a single node cluster the state will remain locally modified and show the correct number of canaries but in multi node clusters a different worker won't have these changes
if state, ok := s.deployment.TaskGroups[tg.Name]; ok {
state.PlacedCanaries = append(state.PlacedCanaries, alloc.ID)
}
|
Leaving reproduction steps for testing purposes Setup and run a local dev cluster (https://github.com/hashicorp/nomad/blob/master/dev/cluster/cluster.sh) run repo.hc.job "failing-nomad-test01" {
datacenters = ["dc1"]
type = "service"
constraint {
attribute = "${meta.tag}"
value = "foo"
}
update {
health_check = "checks"
max_parallel = 4
min_healthy_time = "10s"
healthy_deadline = "3m"
progress_deadline = "10m"
auto_revert = false
auto_promote = false
canary = 4
}
migrate {
max_parallel = 3
health_check = "checks"
min_healthy_time = "10s"
healthy_deadline = "5m"
}
group "group" {
count = 4
restart {
attempts = 0
interval = "30m"
delay = "15s"
mode = "fail"
}
ephemeral_disk {
size = 300
}
task "rails" {
driver = "docker"
env {
RAILS_ENV = "production"
RAILS_SERVE_STATIC_FILES = "1"
RAILS_LOG_TO_STDOUT = "1"
TEST = "2020-01-09 01:23:18 +0000"
version = "129"
}
config {
image = "kaspergrubbe/diceapp:0.0.6"
command = "bundle"
args = ["exec", "unicorn", "-c", "/app/config/unicorn.rb"]
port_map {
web = 8080
}
}
resources {
cpu = 750
memory = 250
network {
mbits = 50
port "web" {}
}
}
service {
name = "failing-nomad-test00"
tags = []
port = "web"
check {
name = "failing-nomad-test00 healthcheck"
type = "http"
protocol = "http"
path = "/"
interval = "5s"
timeout = "3s"
}
}
}
}
}wait for initial deployment to become healthy start a deploy with failing job `nomad job run repro-fail.hcljob "failing-nomad-test01" {
datacenters = ["dc1"]
type = "service"
constraint {
attribute = "${meta.tag}"
value = "foo"
}
update {
health_check = "checks"
max_parallel = 4
min_healthy_time = "10s"
healthy_deadline = "3m"
progress_deadline = "10m"
auto_revert = false
auto_promote = false
canary = 4
}
migrate {
max_parallel = 3
health_check = "checks"
min_healthy_time = "10s"
healthy_deadline = "5m"
}
group "group" {
count = 4
restart {
attempts = 0
interval = "30m"
delay = "15s"
mode = "fail"
}
ephemeral_disk {
size = 300
}
task "rails" {
driver = "docker"
env {
RAILS_ENV = "production"
RAILS_SERVE_STATIC_FILES = "1"
RAILS_LOG_TO_STDOUT = "1"
TEST = "2020-01-09 01:23:18 +0000"
}
config {
image = "kaspergrubbe/diceapp:0.0.6"
command = "false"
port_map {
web = 8080
}
}
resources {
cpu = 750
memory = 250
network {
mbits = 50
port "web" {}
}
}
service {
name = "failing-nomad-test00"
tags = []
port = "web"
check {
name = "failing-nomad-test00 healthcheck"
type = "http"
protocol = "http"
path = "/"
interval = "5s"
timeout = "3s"
}
}
}
}
}
observe 4 failed canaries, and delayed rescheduling attempt Once rescheduled eval runs, it should kill off the 4 healthy previous allocs instead of placing 4 new canaries after patch, and after the failed deployed reschedule eval runs, the state should look similar to |
drewbailey
force-pushed
the
b-update-placed-canaries
branch
from
0651341
to
f7fb621
Compare
|
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. |
Nomad state store must be modified through raft;
DeploymentState.PlacedCanariesis currently modified in-place which masked a bug in single node clusters, but made evaluations in other nodes unaware of the true state of Canaries during a deployment.This change reconciles the state of
PlacedCanarieswhen committing deployment/alloc updates to raft instead of modifying local state of a specific scheduler.fixes #6936
fixes #6864