[
https://issues.apache.org/jira/browse/YUNIKORN-3408?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated YUNIKORN-3408:
-------------------------------------
Labels: pull-request-available (was: )
> Adopt the vet-lock static lock analyser in core and k8shim (lock annotations
> + build wiring)
> --------------------------------------------------------------------------------------------
>
> Key: YUNIKORN-3408
> URL: https://issues.apache.org/jira/browse/YUNIKORN-3408
> Project: Apache YuniKorn
> Issue Type: Test
> Components: core - common, core - scheduler, shim - kubernetes
> Reporter: Dale Richardson
> Priority: Major
> Labels: pull-request-available
>
> We document which lock guards which field and which functions expect a lock
> to be held (YUNIKORN-3349), but nothing checks it: a change that reads a
> guarded field in the wrong place is caught only if a reviewer happens to know
> the rule. This adds a static analyser that checks those rules on every PR,
> the same way goleak (YUNIKORN-3357) now checks goroutine leaks.
> The analyser is [vet-lock|https://github.com/tigerquoll/vet-lock], gVisor's
> {{checklocks}} pass plus the fixes we needed that are still pending upstream,
> and two extra checks that use the same annotations: {{lockstringer}}
> ({{String()}} methods that take their own lock, which {{fmt}} and {{zap}}
> call at a point the type does not control) and {{lockblocking}} (a blocking
> call made while a lock is held).
> One PR per repo, behaviour neutral:
> * {{pkg/locking}}: the go-deadlock lock becomes a named field with forwarding
> methods (inlined), so that an acquisition is attributed to the wrapper the
> annotations name. Without it no annotation matches.
> * The tool pinned in an isolated tools module ({{scripts/vetlock}}, never in
> the main go.mod), a {{make vetlock}} target in {{test_all}} and the
> pre-commit workflow. Non-test sources only; inferred guards off.
> * {{+checklocks}} annotations across core ({{pkg/common}}, {{pkg/events}},
> {{pkg/locking}}, {{pkg/metrics}}, {{pkg/plugins}}, {{pkg/rmproxy}},
> {{pkg/scheduler}} and its sub-packages, {{pkg/webservice}}) and the shim
> ({{pkg/locking}}, {{pkg/cache}}, {{pkg/admission}}, {{pkg/client}},
> {{pkg/dispatcher}}, {{pkg/shim}}, {{pkg/plugin}}, {{pkg/common/test}},
> {{pkg/cmd/admissioncontroller}}).
> * A canary file with one known violation per check, so {{make vetlock}} fails
> if a check silently stops reporting.
> Not in these PRs: the lock-ordering analyser and the runtime lock-order
> checker, and the release-then-reacquire ({{lockgap}}) analyser. Each is a
> follow-up.
> h3. The suppression list is a burn-down list
> Where the code does not do what the annotation says, the annotation is not
> weakened to fit. The site keeps a one-line {{// YUNIKORN-nnnn:}} comment
> naming the problem plus the analyser's ignore directive, so the check lands
> green and blocks new violations straight away, and each of those comments is
> removed by the fix for its own JIRA. Nothing is fixed in the adoption PRs.
> Where a suppression is by design (closures, fsm callback dispatch, interface
> dispatch, construction-time code the analyser cannot follow) the site has a
> plain comment and no JIRA.
> ||Repo||JIRA||Issue||Sites||
> |core|YUNIKORN-3409|AddRejectedApplication writes the rejected-application
> map without the partition lock|2|
> |core|YUNIKORN-3410|Partition removal from a config reload or RM
> re-registration self-deadlocks on the ClusterContext lock|6|
> |core|YUNIKORN-3411|Application write lock held across the RM release round
> trip|11|
> |core|YUNIKORN-3412|Event system handler reads the fields Stop() rewrites;
> Stop() blocks under the write lock|1|
> |core|YUNIKORN-3413|Wildcard limit config read without the manager lock on
> scheduling paths|1|
> |core|YUNIKORN-3414|Manager reaches into user/group tracker internals without
> the tracker lock|3|
> |core|YUNIKORN-3415|Node.String() reads guarded fields without the lock and
> cannot take it|1|
> |core|YUNIKORN-3416|UserGroupCache cleanup locks the singleton instead of the
> receiver; nil dereference after Stop()|2|
> |core|YUNIKORN-3417|RM event replies are unbuffered sends made under the
> ClusterContext write lock|1 (+9 sends in the same handlers)|
> |core|YUNIKORN-3418|User/group resolution runs under the PartitionContext
> read lock|1|
> |core|YUNIKORN-3419|Queue fields read without the queue lock in parent-first
> paths and the constructors|7|
> |core|YUNIKORN-3420|String() methods that take their own lock|4|
> |shim|YUNIKORN-3421|shouldAppRelease drops the task lock inside an FSM
> callback: deadlock with a concurrent task event|2|
> |shim|YUNIKORN-3422|SchedulerCache node-list getters populate their cache
> under the read lock|3|
> |shim|YUNIKORN-3423|postAppAccepted reads taskGroups and taskMap without the
> application lock|1|
> |shim|YUNIKORN-3424|createAppPlaceholders walks the application task map
> without the application lock|1|
> |shim|YUNIKORN-3425|Admission controller serving goroutine reads the server
> field Shutdown nils|1|
> |shim|YUNIKORN-3426|Pod bind retries with backoff run under the task write
> lock|2|
> |shim|YUNIKORN-3427|flushReleaseableTasks removes from the context map and
> reads task state under the application lock only|2|
> |shim|YUNIKORN-3428|Application.taskMap read without the lock from String()
> and AreAllTasksTerminated|2|
> |shim|YUNIKORN-3429|onReserving's goroutine reads originatingTask after the
> lock is released|1|
> |shim|YUNIKORN-3430|registerNodesInternal releases and retakes the context
> lock around the wait group|1|
> Rebasing the shim branch onto current master made the check report two new
> sites: YUNIKORN-2884 runs both bind attempts through {{retry.OnError}} while
> holding the task write lock (YUNIKORN-3426). That is the kind of change this
> is meant to stop at review time.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
