[
https://issues.apache.org/jira/browse/YUNIKORN-3349?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dale Richardson updated YUNIKORN-3349:
--------------------------------------
Description:
The site documents what YuniKorn does i.e. 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.
was:
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.
> 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
> Assignee: Dale Richardson
> Priority: Major
>
> The site documents what YuniKorn does i.e. 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]
