Dale Richardson created YUNIKORN-3349:
-----------------------------------------
Summary: Enhance Developer concurrency documentation
Key: YUNIKORN-3349
URL: https://issues.apache.org/jira/browse/YUNIKORN-3349
Project: Apache YuniKorn
Issue Type: Improvement
Components: documentation
Reporter: Dale Richardson
The site documents what YuniKorn does — states, configuration, features — but
very little about how to change it safely. The conventions a contributor must
follow to avoid introducing a deadlock or a data race are largely undocumented,
and the one place they are partly written down is a design proposal for a
migration that completed years ago.
YUNIKORN-3348 corrects the factual errors in that proposal and documents the
deadlock detection tooling. This issue covers the larger gap it exposed.
h3. What is documented today
* {{docs/design/cache_removal.md}} — per-object lock responsibilities (cluster,
partition, queue, application, node), the "never lock more than one queue at a
time" rule, and the "do not iterate while holding a lock" rule. Written for the
YUNIKORN-317 migration, not maintained as a reference.
* {{docs/design/user_group_manager.md}} — the queueTracker is deliberately lock
free and locking is delegated to userTracker/groupTracker.
* {{src/pages/community/coding_guidelines.md}} — formatting and linting only:
gofmt, golangci-lint, IDE setup. No semantic conventions.
h3. What is not documented anywhere
* *The concrete lock order.* ClusterContext -> PartitionContext -> Application
-> \{Queue, Node, ugm, Allocation\}.
* *The partition rule.* Stated in the code at
{{yunikorn-core/pkg/scheduler/partition.go:72}} but nowhere on the site: the
PartitionContext write lock must not be held while manipulating an Application,
because scheduling holds the application write lock and then requests nodes
from the partition.
* *The FSM contract.* {{docs/developer_guide/scheduler_object_states.md}}
documents every state and transition for both the core and the shim, with no
mention of locking. Undocumented: the caller holds the object lock across
{{sm.Event(...)}}; callbacks must therefore be lock free (the rule is written
in a code comment at {{yunikorn-k8shim/pkg/cache/application.go:96-104}}); raw
{{SetState}} bypasses all callbacks; {{"no transition"}} is not an error.
* *The comment vocabulary.* The code uses banners such as "{{// Lock free call
...}}", "{{// This function MUST be called holding the lock for the queue}}"
and "{{// NOTE: this is a lock free call. It must NOT be called holding the
PartitionContext lock}}" as a locking contract between caller and callee. Their
meaning is not written down, so they are easy to omit or to copy incorrectly
onto a function where they do not hold.
* *Resource semantics.* {{resources.Resource}} is sparse and mutable. nil, a
zero value and an absent type mean three different things for quotas; getters
return clones; {{Prune()}} changes meaning; the {{OnlyExisting}} and
{{MaxUndef}} comparison variants differ in ways that have produced real bugs.
* *The threading model.* {{docs/design/architecture.md}} is a short component
overview with no concurrency content. Not documented: scheduling runs as a
single background goroutine, state changes arrive as events on buffered
channels, and which background goroutines exist.
* *Shim specifics.* {{Context.lock}} is a serialisation lock as well as a data
lock, stated in a code comment at {{yunikorn-k8shim/pkg/cache/context.go:79}}
and nowhere on the site; the dispatcher is a single goroutine draining one
channel; every informer that is created must also be started and stopped.
h3. Proposal
Add {{docs/developer_guide/concurrency.md}} covering, at minimum:
# The lock order, and the two hard prohibitions: partition versus application,
and queue versus parent.
# The comment banner vocabulary, and what each phrase obliges the caller to do.
# The FSM contract for both repositories.
# Resource semantics: nil versus zero versus absent, cloning, mutability.
# The threading model, and the shim's dispatcher and informer model.
# A short reviewer checklist.
Link it from:
* {{src/pages/community/coding_guidelines.md}} — a contributor reading the page
titled "Coding Guidelines" end to end currently learns nothing about the lock
order.
* {{docs/developer_guide/scheduler_object_states.md}} — for the FSM callback
contract.
* {{docs/design/architecture.md}} — for the threading model.
h3. Follow-up for cache_removal.md
{{cache_removal.md}} is a design proposal for a migration completed under
YUNIKORN-317, yet its "Current locking" section is the de facto locking
reference for the project. Once a maintained page exists, the locking content
should be lifted into it and the proposal marked accordingly. The project
already has a convention for this: documents under {{docs/archived_design/}}
carry a {{:::caution}} banner naming the superseding document.
h3. Why it is worth doing
Several deadlocks and races found in the scheduler trace back to conventions
that hold throughout the code but are written down only as individual comments:
locks released in the middle of a callback, FSM callbacks acquiring another
object's lock, raw {{SetState}} bypassing the state machine. Contributors
cannot follow rules they cannot find, and reviewers re-derive them from scratch
on every patch.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
