[
https://issues.apache.org/jira/browse/YUNIKORN-3349?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated YUNIKORN-3349:
-------------------------------------
Labels: pull-request-available (was: )
> 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
> Labels: pull-request-available
>
> 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]
