Dale Richardson created YUNIKORN-3408:
-----------------------------------------
Summary: 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
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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
