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

Backport of scheduler: annotate tasksUpdated with reason and purge DeepEquals into release/1.5.x #16491

Conversation

hc-github-team-nomad-core
Copy link
Contributor

Backport

This PR is auto-generated from #16421 to be assessed for backporting due to the inclusion of the label backport/1.5.x.

The below text is copied from the body of the original PR.


reflect.DeepEqual is not suitable for comparing Job structs.

By design, it

  • treats nil vs empty slices/maps as different

Note that a non-nil empty slice and a nil slice (for example, []byte{} and []byte(nil)) are not deeply equal.

  • compares private struct fields

Struct values are deeply equal if their corresponding fields, both exported and unexported, are deeply equal.

The use of reflect.DeepEqual has been the root of bugs a few times in the past.

#14917
#10896
#9330

This PR introduces the OpaqueMapEquals helper function as a replacement for DeepEqual for the specific cases of comparing map[comparable]interface{}, which is necessary in cases like opaque plugin config, and envoy configs. Unlike DE, OME exhibits the opposite behavior in the two scenarios above. For other (known) types, implement .Equal method on structs and compare values directly, or defer to other sensible helpers.

The tasksUpdated helper has also been modified to return a struct containing the first mis-match of fields triggering a destructive update (as opposed to a simple boolean). Folks tracking down misbehavior now need only to add a log line to understand why tasksUpdated is (or isn't) triggering updates.

Benchmark

before

nomad/scheduler on main
➜ go test -count=3 -bench=BenchmarkTasksUpdated -run BenchmarkTasksUpdated
BenchmarkTasksUpdated-24          105920             14791 ns/op
BenchmarkTasksUpdated-24           82866             14376 ns/op
BenchmarkTasksUpdated-24           89450             14674 ns/op
PASS
ok      github.com/hashicorp/nomad/scheduler    4.485s

after

nomad/scheduler on tasks-updated-annotate
➜ go test -count=3 -bench=BenchmarkTasksUpdated -run BenchmarkTasksUpdated
BenchmarkTasksUpdated-24           54816             24469 ns/op
BenchmarkTasksUpdated-24           47431             24392 ns/op
BenchmarkTasksUpdated-24           48696             24565 ns/op
PASS
ok      github.com/hashicorp/nomad/scheduler    4.436s

The slowdown is primarily caused by OpaqueMapEquals - replacing its implementation with DeepEqual gets the numbers back down. But these numbers are still negligible and the tradeoff for correctness seems worthwhile.

Closes #16290

@hc-github-team-nomad-core hc-github-team-nomad-core force-pushed the backport/tasks-updated-annotate/intensely-saving-adder branch from b449e5e to 6b46559 Compare March 14, 2023 14:46
@hc-github-team-nomad-core hc-github-team-nomad-core merged commit f585651 into release/1.5.x Mar 14, 2023
@hc-github-team-nomad-core hc-github-team-nomad-core deleted the backport/tasks-updated-annotate/intensely-saving-adder branch March 14, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants