[
https://issues.apache.org/jira/browse/YUNIKORN-3408?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dale Richardson updated YUNIKORN-3408:
--------------------------------------
Description:
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.
was:
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.
The individual issues are linked from this JIRA ("Testing discovered"); the
table below is filled in as they are filed.
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. That is the kind of change this is meant to stop
at review time.
> 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
>
> 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]
