Dale Richardson created YUNIKORN-3348:
-----------------------------------------
Summary: Develop doc fixups
Key: YUNIKORN-3348
URL: https://issues.apache.org/jira/browse/YUNIKORN-3348
Project: Apache YuniKorn
Issue Type: Improvement
Components: documentation
Reporter: Dale Richardson
The only statement of the scheduler's lock-ordering rule anywhere on the site
is malformed, and the surrounding text has been overtaken by a feature added in
2024. Two adjacent gaps make the same information hard to find. All four items
below are small, self-contained fixes.
h3. 1. The lock-order rule is garbled
{{docs/design/cache_removal.md}}, lines 424-426:
{quote}
It is possible to acquire another lock while holding a lock, but we need to
make sure that we do not allow:
* Holding A.lock and acquire B's lock.
* Holding B.lock and acquire B's lock.
{quote}
The second bullet should read "Holding *B.lock* and acquire *A's* lock". As
written it says "B then B", which is a tautology, so the rule as published
states nothing.
This is the only lock-ordering statement in the documentation. The same text
appears in every versioned copy from 1.3.0 through 1.9.0.
Suggested fix:
{noformat}
- Holding A.lock and acquire B's lock.
- Holding B.lock and acquire A's lock.
{noformat}
h3. 2. "No known tools" is no longer true
{{docs/design/cache_removal.md}}, line 456:
{quote}
There are no known tools that could be used to detect or describe lock order.
{quote}
This has been false since YUNIKORN-2539 (commit 5758d7a, 2024-04-05) added
{{pkg/locking}}, which wraps {{github.com/sasha-s/go-deadlock}} and performs
both lock-order (ABBA) detection and lock-wait timeout detection.
Suggested replacement:
{quote}
Lock order and lock-wait timeouts can be detected at runtime. The
{{pkg/locking}} package wraps {{go-deadlock}} and is enabled via the
environment variables described in the service configuration documentation.
{quote}
h3. 3. Deadlock detection settings are undocumented
{{pkg/locking}} is configured entirely by environment variables, none of which
appear anywhere on the site:
|| Variable || Default || Effect ||
| {{DEADLOCK_DETECTION_ENABLED}} | false | Master switch. go-deadlock is
compiled in either way, it is only disabled. |
| {{DEADLOCK_TIMEOUT_SECONDS}} | 60 | Lock-wait threshold before a potential
deadlock is reported. |
| {{DEADLOCK_EXIT}} | false | Call {{os.Exit(1)}} when a potential deadlock is
detected. |
| {{DEADLOCK_DISABLE_LOCK_ORDER}} | false | Disable ABBA lock-order detection,
keeping only the timeout check. |
{{docs/user_guide/service_config.md}} already documents comparable runtime
knobs ({{GOGC}}, {{GOMEMLIMIT}}), so that is the natural home.
This matters operationally: with {{DEADLOCK_EXIT=true}} the scheduler process
terminates on detection, and an operator currently has nothing to look up. The
detection state is also exposed over REST — {{/ws/v1/config}} returns
{{DeadlockDetectionEnabled}} and {{DeadlockTimeoutSeconds}} — with no
documentation of what those fields mean.
h3. 4. {{make test}} runs with detection enabled, which is not stated
{{docs/developer_guide/build.md}}, line 165, says only:
{quote}
* A full unit test run {{make test}}
{quote}
The {{test}} target in both yunikorn-core and yunikorn-k8shim exports
{{DEADLOCK_DETECTION_ENABLED=true}}, {{DEADLOCK_TIMEOUT_SECONDS=10}} and
{{DEADLOCK_EXIT=true}}, and runs the suite under {{-race}}. A contributor whose
test run exits with a "POTENTIAL DEADLOCK" dump has no documentation explaining
what produced it or how to adjust the timeout.
h3. Proposed changes
# Fix the A/B typo at {{cache_removal.md}} lines 425-426.
# Replace the "no known tools" sentence at {{cache_removal.md}} line 456 with a
reference to {{pkg/locking}} and the variables above.
# Add a "Deadlock detection" subsection to
{{docs/user_guide/service_config.md}} documenting the four environment
variables.
# Extend the {{make test}} bullet in {{docs/developer_guide/build.md}} to state
that it runs with {{-race}} and deadlock detection enabled (exit on detection,
10 second timeout).
h3. Note on 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. The project already has a convention for this
situation: documents under {{docs/archived_design/}} carry a {{:::caution}}
banner naming the superseding document. Either the locking content should be
lifted into a maintained developer guide page, or the document should state
plainly which parts are still current. A follow-up issue is proposed for that.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
