[
https://issues.apache.org/jira/browse/YUNIKORN-3348?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dale Richardson reassigned YUNIKORN-3348:
-----------------------------------------
Assignee: Dale Richardson
> 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
> Assignee: Dale Richardson
> Priority: Major
>
> 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]
