Skip to content

fix: unbreak go vet and make build on master - #940

Open
omlahore wants to merge 1 commit into
openkruise:masterfrom
omlahore:fix/csi-copylocks
Open

fix: unbreak go vet and make build on master#940
omlahore wants to merge 1 commit into
openkruise:masterfrom
omlahore:fix/csi-copylocks

Conversation

@omlahore

@omlahore omlahore commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #905

Two separate things stop make build producing a binary on a clean checkout of master. The issue diagnosed the first; the second only becomes visible once the first is fixed.

1. go vet fails, and vet gates the build

#876 bumped container-storage-interface/spec from v1.9.0 to v1.13.0. In v1.13.0 csi.NodePublishVolumeRequest carries a protoimpl.MessageState, which embeds a sync.Mutex. The storage CLI passes that struct by value everywhere, so vet now reports 18 copylocks findings and exits 1:

pkg/agent-runtime/storage-cli/storage/csi_runner.go:52:67: RunNodePublishVolume passes lock by value
pkg/agent-runtime/storage-cli/main.go:198:35: validateGeneralParams passes lock by value
...

Makefile:81 and Makefile:110 both declare vet as a prerequisite, so build and test-e2e stop before doing anything.

Fixed by taking *csi.NodePublishVolumeRequest through the Provider interface, RunNodePublishVolume and validateGeneralParams. That is the ordinary way to handle a generated protobuf message, and it removes a real struct copy from the mount path rather than just silencing the check. main.go already did proto.Unmarshal(configRaw, &csiReq), so it had the pointer in hand anyway.

No production Provider implementations exist outside the test fakes, so the interface change is contained.

2. The build target compiles a file, not a package

With vet clean, make build gets further and then fails:

cmd/agent-sandbox-controller/main.go:367:2: undefined: executeCABindings

executeCABindings lives in cmd/agent-sandbox-controller/ca_binding.go, same package. The target was:

go build -o bin/agent-sandbox-controller cmd/agent-sandbox-controller/main.go

Naming a single .go file builds only that file, so its siblings are invisible. go build ./cmd/agent-sandbox-controller/ has always worked, which is why this never showed up in go build ./... or in CI.

Changed to build the package. The build-okactl target two lines below already uses ./cmd/okactl, so this makes the two consistent.

Verification

$ go vet ./... ; echo $?
0

$ make build
...
go build -o bin/agent-sandbox-controller ./cmd/agent-sandbox-controller
$ ls -l bin/agent-sandbox-controller
-rwxrwxr-x 1 ... 103290525 ... bin/agent-sandbox-controller

go test ./pkg/agent-runtime/storage-cli/... passes across all four packages.

37 insertions, 39 deletions. Most of it is the mechanical csi.NodePublishVolumeRequest to *csi.NodePublishVolumeRequest change in the test fakes.

One note on scope

make build runs go fmt ./..., which reformatted four unrelated test files on my machine (pkg/identity/ca_cert_injector_test.go and three others, all struct-field alignment). I reverted those rather than fold unrelated churn in here, but they are unformatted on master and something will keep re-flagging them. Worth a separate cleanup.

Why CI missed it

No workflow runs go vet. The undefined: executeCABindings case is worse, since it only appears in make build and never in go build ./.... A CI job that runs make build would catch both.

The csi spec bump in openkruise#876 (v1.9.0 to v1.13.0) gave
csi.NodePublishVolumeRequest a protoimpl.MessageState, which embeds a
sync.Mutex. The storage-cli passes that struct by value throughout, so
go vet started reporting 18 copylocks findings and exiting 1.

Makefile declares vet as a prerequisite of both build and test-e2e, so
neither ran. Take the request by pointer instead, which is the normal
way to handle a protobuf message and removes a real struct copy from the
mount path.

make build still produced no binary after that, for a second and
unrelated reason: the target compiled a single file rather than the
package, so executeCABindings in ca_binding.go was invisible to it.
Build the package, matching what the okactl target two lines below
already does.

go vet ./... is now clean and make build produces
bin/agent-sandbox-controller.

Fixes openkruise#905

Signed-off-by: Om <omlahore47@gmail.com>
@kruise-bot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign furykerry for approval by writing /assign @furykerry in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.07%. Comparing base (1781b7c) to head (acf2c2e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #940      +/-   ##
==========================================
- Coverage   83.08%   83.07%   -0.01%     
==========================================
  Files         259      259              
  Lines       22555    22555              
==========================================
- Hits        18739    18738       -1     
- Misses       3093     3095       +2     
+ Partials      723      722       -1     
Flag Coverage Δ
unittests 83.07% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants