[
https://issues.apache.org/jira/browse/YUNIKORN-3408?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Wilfred Spiegelenburg updated YUNIKORN-3408:
--------------------------------------------
Issue Type: Task (was: Test)
> 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: Task
> 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], a custom
> extension of gVisor's {{checklocks}} with additional annotations and checking
> logic I authored, In this tranche their are 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.
> Further tranches will enable additional analysis such as the lock-ordering
> analyser and the runtime lock-order checker, and the release-then-reacquire
> ({{{}lockgap{}}}) analyser. Each is a follow-up PR.
> 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]
