[WIP] HIVE-2139: Reuse allocated buffers in the resource helper#2873
Closed
dlom wants to merge 2 commits intoopenshift:masterfrom
Closed
[WIP] HIVE-2139: Reuse allocated buffers in the resource helper#2873dlom wants to merge 2 commits intoopenshift:masterfrom
dlom wants to merge 2 commits intoopenshift:masterfrom
Conversation
Claude wrote the actual benchmarks and the script for comparing them. Go claude! It's very specific to this benchmark I'm testing, so I'll update it later to make it more generic Assisted-by: Claude Sonnet 4.5 (Anthropic)
openshift-ci-robot
added
the
jira/valid-reference
|
@dlom: This pull request references HIVE-2139 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughIntroduces environment test (envtest) setup automation and buffer pooling for resource efficiency. Adds setup-envtest tooling integration through build system changes, vendors substantial controller-runtime testing infrastructure, and implements comprehensive benchmark suites for Apply, Patch, and controller reconciliation operations. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Runner
participant Env as Environment
participant ControlPlane as Control Plane
participant APIServer as API Server
participant Etcd as Etcd
participant CRDMgr as CRD Manager
participant WebhookMgr as Webhook Manager
Test->>Env: Start()
Env->>ControlPlane: Initialize
ControlPlane->>Etcd: Start()
activate Etcd
Etcd-->>ControlPlane: Ready
deactivate Etcd
ControlPlane->>APIServer: Start()
activate APIServer
APIServer-->>ControlPlane: Ready
deactivate APIServer
Env->>Env: Wait for Default Namespace
Env->>CRDMgr: InstallCRDs()
activate CRDMgr
CRDMgr->>APIServer: Create CRD Resources
CRDMgr->>APIServer: Wait for Discovery
CRDMgr-->>Env: Complete
deactivate CRDMgr
Env->>WebhookMgr: Install Webhooks
activate WebhookMgr
WebhookMgr->>APIServer: Setup Webhook Configs
WebhookMgr->>APIServer: Verify Availability
WebhookMgr-->>Env: Complete
deactivate WebhookMgr
Env-->>Test: REST Config Ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@dlom: This pull request references HIVE-2139 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
Author
|
/retitle [WIP] HIVE-2139: Reuse allocated buffers in the resource helper |
openshift-ci
bot
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@dlom: This pull request references HIVE-2139 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (3)
hack/setup-envtest.sh (1)
3-4: Redundant shell option.
set -eandset -o errexitare equivalent; one can be removed.♻️ Suggested simplification
set -e -set -o errexit set -o nounset set -o pipefail🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/setup-envtest.sh` around lines 3 - 4, The script redundantly enables the same shell option twice by using both "set -e" and "set -o errexit"; remove one of them (either "set -e" or "set -o errexit") from the top of hack/setup-envtest.sh so the script still exits on error but avoids duplicate settings, leaving only a single invocation that enforces errexit.vendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/apiserver.go (1)
33-38: Minor typo in upstream comment.Line 36-37 has a copy-paste error in the comment - both comments reference "saKeyFile" but line 37 defines
saCertFile. This is a minor documentation issue in the vendored upstream controller-runtime code.// saKeyFile is the name of the service account signing private key file. saKeyFile = "sa-signer.key" // saKeyFile is the name of the service account signing public key (cert) file. // <-- should say "saCertFile" saCertFile = "sa-signer.crt"Since this is vendored code, consider reporting this upstream to controller-runtime if desired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/apiserver.go` around lines 33 - 38, Fix the copy-paste comment: update the second comment that currently mentions "saKeyFile" to correctly reference saCertFile so the comment matches the constant it documents; locate the constant block containing saKeyFile and saCertFile in apiserver.go and change the comment for the cert constant to say "saCertFile is the name of the service account signing public key (cert) file."vendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/etcd.go (1)
45-60: Minor typo in upstream comment.Line 46 has a typo:
"they will be evaluated"should be"they will be evaluated"`. This is a minor documentation issue in the vendored upstream controller-runtime code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/etcd.go` around lines 45 - 60, The comment above the Args field in the Etcd struct contains a typo ("the`y will be evaluated"); update the comment to read "they will be evaluated" to correct the documentation for Args in the Etcd type (edit the comment block that describes Args in the Etcd struct).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/benchmark-apply-comparison.sh`:
- Around line 37-45: The script currently uses git stash push/pop and STASHED
which is unreliable because the buffer-pool change is already committed; replace
the stash-based flow with creating a temporary git worktree for the baseline
commit (e4203bc0b or the commit hash/tag you want to benchmark), run the same go
test bench command (using BENCHTIME, COUNT and TEMP_DIR) inside that worktree
and redirect output to without.txt, then remove the temporary worktree; then run
the “with” benchmark in the main worktree producing with.txt—this avoids git
stash push/pop and the STASHED flag and prevents restoring unrelated stashes.
In `@hack/setup-envtest.sh`:
- Around line 8-10: The script currently warns when the setup-envtest binary is
missing but continues and later fails; modify the hack/setup-envtest.sh check so
that after detecting command -v setup-envtest returns false it exits with a
non-zero status (e.g., echo the message and run exit 1) to stop execution
immediately; reference the check that invokes setup-envtest and ensure any
callers won’t proceed when the binary is absent.
In `@pkg/resource/apply_benchmark_test.go`:
- Around line 290-314: The benchmark calls b.Fatalf from within goroutines
launched by b.RunParallel (in BenchmarkConcurrentApply and the inner closure
using helper.Apply), which is unsafe; change the error handling to record the
first error from concurrent goroutines using a concurrency-safe mechanism (e.g.,
an atomic.Value or an int32 flag plus storing the error string) and call
b.Fatalf (or b.Fatal) exactly once from the main benchmark goroutine after
RunParallel returns if any error was recorded; add the sync/atomic import and
modify the loop to set the atomic error/flag when helper.Apply returns an error
instead of calling b.Fatalf directly.
In `@vendor/sigs.k8s.io/controller-runtime/pkg/envtest/crd.go`:
- Around line 299-306: The code calls os.Stat(path) and only checks
os.IsNotExist(err), then dereferences info via info.IsDir(), which can panic for
non-ENOENT errors; update the logic around the os.Stat call (the variables info
and err) so you first check if err != nil: if os.IsNotExist(err) then handle
options.ErrorIfPathMissing as before (continue or return), otherwise return the
original err; only when err == nil should you dereference info and call
info.IsDir(). Ensure this change is applied to the block that uses os.Stat(path)
and the surrounding control flow that references options.ErrorIfPathMissing.
In `@vendor/sigs.k8s.io/controller-runtime/pkg/envtest/webhook.go`:
- Around line 258-267: The current loop treats apierrors.IsNotFound(err) as "not
ready" but still returns the NotFound error immediately; change the error
handling in the loop (the block that references names.Delete(name), allFound and
the err checks) so that if apierrors.IsNotFound(err) you mark allFound = false
and continue polling instead of returning err, only returning err for
non-NotFound, non-nil errors; keep names.Delete(name) only when err == nil so
successful finds still remove entries from names.
- Around line 111-118: The updateClientConfig function currently only rewrites
service-based configs when cc.Service.Path is set; change it to rewrite whenever
cc.Service != nil so manifests that omit Service.Path are converted to the local
envtest URL. In updateClientConfig, keep setting cc.CABundle, then if cc.Service
!= nil build a URL using hostPort and the optional path (e.g. if cc.Service.Path
!= nil append "/" + *cc.Service.Path, otherwise use hostPort with no extra path
or a single "/"), assign cc.URL to that value and set cc.Service = nil;
reference the updateClientConfig function and the cc.Service / cc.Service.Path
symbols to locate the change.
In `@vendor/sigs.k8s.io/controller-runtime/pkg/internal/flock/flock_unix.go`:
- Around line 33-47: In Acquire, close the file descriptor if unix.Flock fails
(both when errors.Is(err, unix.EWOULDBLOCK) and on any other flock error) to
avoid leaking fds; after calling unix.Flock(fd, ...), if err != nil then
close(fd) before returning the error or wrapping it as ErrAlreadyLocked. Also
remove the dead os.ErrExist branch after unix.Open since O_CREAT without O_EXCL
never returns os.ErrExist, simplifying the open error handling to just return
err on failure.
In `@vendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/addr/manager.go`:
- Around line 124-127: The comment above Suggest (reserve-window) is out of sync
with the constant; update the comment to reflect the actual port reserve
duration used by portReserveTime (currently set to 2 minutes) or change the
constant if the intended window is 1 minute; locate the portReserveTime constant
and the reserve-window sentence near Suggest in manager.go and make the wording
and numeric value consistent (e.g., "within 2 minutes" if keeping
portReserveTime as-is).
- Around line 129-139: The loop defers listener.Close() causing listeners to
stay open across retries; remove the defer and explicitly close the listener
immediately after using it. In the retry loop that calls suggest(listenHost) and
then cache.add(port), replace the deferred Close with immediate calls to
listener.Close(): call listener.Close() before any return paths (both the
success branch where cache.add returns ok and the error branch where it returns
err) and also close it before continuing the loop when cache.add returns (false,
nil); adjust the code around suggest, listener, and cache.add accordingly so no
listener remains open across iterations.
In `@vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/env/env.go`:
- Around line 423-457: PathMatches erroneously accepts a path as soon as
versionFromPathName parses a versioned basename, which can mutate
e.Version/e.Platform to values that don't satisfy the caller's requested
selectors; change PathMatches so that it does not immediately accept/commit
parsed values from versionFromPathName — instead, first parse the
version/platform from the basename (call versionFromPathName in a non-mutating
way or extract the parsed result without assigning to e.Version/e.Platform),
verify the parsed version/platform satisfy the current selector/constraints, and
only then set e.Version, e.Platform and return true; if the parsed values do not
match the selector, continue checking binaries (and fallback logic) as before.
Ensure the same non-mutating verification approach is applied to the analogous
code path later in the file that currently returns early after
versionFromPathName succeeds.
- Around line 269-276: The current flow fetches e.Platform.Hash via
e.Client.FetchSum but never uses it when downloading and storing the archive;
modify the download path (the code that calls e.HTTPClient.GetVersion and then
e.Store.Add) to verify the downloaded bytes against e.Platform.Hash when
e.VerifySum is true: compute the same hash algorithm used by FetchSum (e.g.,
SHA-256) while streaming the response (or after full download), compare the
computed digest to e.Platform.Hash, and return/ExitCause on mismatch before
calling e.Store.Add; ensure the verification logic references e.VerifySum,
e.Platform.Hash, e.Client.FetchSum, e.HTTPClient.GetVersion and e.Store.Add so
the integrity check is applied consistently.
In `@vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/env/helpers.go`:
- Around line 31-34: Update the doc comment above the PrintEnv symbol to use the
correct command name "setup-envtest" instead of "fetch-envtest"; locate the
comment block referencing PrintEnv in helpers.go and replace the example
invocation ``source $(fetch-envtest switch -p env 1.20.x)`` with ``source
$(setup-envtest switch -p env 1.20.x)`` so the documentation matches the actual
tool name.
In `@vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/README.md`:
- Around line 107-112: Fix two typos in the README text: change “As noted about”
to “As noted above” and “gziped” to “gzipped” in the paragraph that mentions
ENVTEST_INSTALLED_ONLY and ENVTEST_USE_ENV and the follow-up sentence about the
sideload command; update the strings in the README (look for occurrences of
ENVTEST_INSTALLED_ONLY, ENVTEST_USE_ENV, and “sideload”) so the corrected
wording reads naturally.
In
`@vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/versions/platform.go`:
- Around line 35-41: BaseName currently formats only major.minor.patch which
drops prerelease parts and causes collisions; update Platform.BaseName to
include ver.Prerelease (and any build metadata if present) when constructing the
version string so prerelease-aware versions (e.g., 1.31.0-alpha.1) stay
distinct, and ensure ArchiveName (which uses BaseName) inherits that change;
locate and update the BaseName method in the Platform type and any other places
that duplicate the base-name construction (see functions named BaseName and
ArchiveName and the regex helpers ExtractWithPlatform / ConcreteVersionRE /
VersionPlatformRE for consistency) so stored/archive names preserve prerelease
identifiers.
---
Nitpick comments:
In `@hack/setup-envtest.sh`:
- Around line 3-4: The script redundantly enables the same shell option twice by
using both "set -e" and "set -o errexit"; remove one of them (either "set -e" or
"set -o errexit") from the top of hack/setup-envtest.sh so the script still
exits on error but avoids duplicate settings, leaving only a single invocation
that enforces errexit.
In
`@vendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/apiserver.go`:
- Around line 33-38: Fix the copy-paste comment: update the second comment that
currently mentions "saKeyFile" to correctly reference saCertFile so the comment
matches the constant it documents; locate the constant block containing
saKeyFile and saCertFile in apiserver.go and change the comment for the cert
constant to say "saCertFile is the name of the service account signing public
key (cert) file."
In
`@vendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/etcd.go`:
- Around line 45-60: The comment above the Args field in the Etcd struct
contains a typo ("the`y will be evaluated"); update the comment to read "they
will be evaluated" to correct the documentation for Args in the Etcd type (edit
the comment block that describes Args in the Etcd struct).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80940d01-4185-4914-9617-ef7bf412455d
📒 Files selected for processing (48)
Makefilego.modhack/benchmark-apply-comparison.shhack/setup-envtest.shpkg/dependencymagnet/doc.gopkg/resource/apply.gopkg/resource/apply_benchmark_test.gopkg/resource/bufferpool.gopkg/resource/controller_sim_benchmark_test.gopkg/resource/patch.gopkg/resource/serializer.govendor/modules.txtvendor/sigs.k8s.io/controller-runtime/pkg/envtest/crd.govendor/sigs.k8s.io/controller-runtime/pkg/envtest/doc.govendor/sigs.k8s.io/controller-runtime/pkg/envtest/helper.govendor/sigs.k8s.io/controller-runtime/pkg/envtest/server.govendor/sigs.k8s.io/controller-runtime/pkg/envtest/webhook.govendor/sigs.k8s.io/controller-runtime/pkg/internal/flock/doc.govendor/sigs.k8s.io/controller-runtime/pkg/internal/flock/errors.govendor/sigs.k8s.io/controller-runtime/pkg/internal/flock/flock_other.govendor/sigs.k8s.io/controller-runtime/pkg/internal/flock/flock_unix.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/addr/manager.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/certs/tinyca.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/apiserver.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/auth.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/etcd.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/kubectl.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/plane.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/process/arguments.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/process/bin_path_finder.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/process/procattr_other.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/process/procattr_unix.govendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/process/process.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/LICENSEvendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/README.mdvendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/env/env.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/env/exit.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/env/helpers.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/main.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/remote/client.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/remote/http_client.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/remote/read_body.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/store/helpers.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/store/store.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/versions/parse.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/versions/platform.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/versions/version.govendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/workflows/workflows.go
Comment on lines
+37
to
+45
| git stash push -q pkg/resource/bufferpool.go pkg/resource/apply.go pkg/resource/patch.go | ||
| STASHED=1 | ||
|
|
||
| echo "Running benchmarks WITHOUT buffer pool..." | ||
| go test -bench='^Benchmark(Apply|Patch|Controller)' -benchmem -benchtime=${BENCHTIME} -count=${COUNT} ./pkg/resource/ > "${TEMP_DIR}/without.txt" 2>&1 | ||
|
|
||
| echo "Restoring changes..." | ||
| git stash pop -q | ||
| STASHED=0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full script context around lines 37-45
cat -n hack/benchmark-apply-comparison.sh | head -50Repository: openshift/hive
Length of output: 2056
🏁 Script executed:
# Check if the files mentioned (bufferpool.go, apply.go, patch.go) exist
fd -t f "bufferpool.go|apply.go|patch.go" pkg/resource/Repository: openshift/hive
Length of output: 128
🏁 Script executed:
# Let's also check the git log to see what commits might be involved
# Get a summary of recent commits touching these files
git log --oneline -20 -- pkg/resource/bufferpool.go pkg/resource/apply.go pkg/resource/patch.go 2>/dev/null | head -10Repository: openshift/hive
Length of output: 105
Benchmark A/B switch is unreliable: git stash push won't stash committed changes.
The buffer pool optimization (commit e4203bc) is already committed, so git stash push on lines 37–38 stashes nothing. Both benchmark runs measure identical code. Additionally, line 44's git stash pop -q may restore an unrelated pre-existing stash, corrupting developer state.
Use a temporary worktree to safely benchmark the baseline commit:
🛠️ Safer approach: benchmark baseline via a temporary worktree
TEMP_DIR=$(mktemp -d)
-STASHED=0
+WITHOUT_TREE=""
@@
-# Ensure git stash is restored on exit
+# Ensure temp worktree is cleaned up on exit
cleanup() {
- if [ "$STASHED" -eq 1 ]; then
- echo ""
- echo "Restoring stashed changes..."
- git stash pop -q || echo "Warning: failed to restore stash"
- fi
+ if [ -n "$WITHOUT_TREE" ] && [ -d "$WITHOUT_TREE" ]; then
+ git worktree remove --force "$WITHOUT_TREE" >/dev/null 2>&1 || true
+ fi
}
trap cleanup EXIT
@@
-echo "Stashing buffer pool changes..."
-git stash push -q pkg/resource/bufferpool.go pkg/resource/apply.go pkg/resource/patch.go
-STASHED=1
+echo "Preparing baseline worktree (without buffer pool commit)..."
+WITHOUT_TREE=$(mktemp -d)
+git worktree add --detach "$WITHOUT_TREE" HEAD~1 >/dev/null
@@
echo "Running benchmarks WITHOUT buffer pool..."
-go test -bench='^Benchmark(Apply|Patch|Controller)' -benchmem -benchtime=${BENCHTIME} -count=${COUNT} ./pkg/resource/ > "${TEMP_DIR}/without.txt" 2>&1
-
-echo "Restoring changes..."
-git stash pop -q
-STASHED=0
+(cd "$WITHOUT_TREE" && go test -bench='^Benchmark(Apply|Patch|Controller)' -benchmem -benchtime=${BENCHTIME} -count=${COUNT} ./pkg/resource/) > "${TEMP_DIR}/without.txt" 2>&1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/benchmark-apply-comparison.sh` around lines 37 - 45, The script
currently uses git stash push/pop and STASHED which is unreliable because the
buffer-pool change is already committed; replace the stash-based flow with
creating a temporary git worktree for the baseline commit (e4203bc0b or the
commit hash/tag you want to benchmark), run the same go test bench command
(using BENCHTIME, COUNT and TEMP_DIR) inside that worktree and redirect output
to without.txt, then remove the temporary worktree; then run the “with”
benchmark in the main worktree producing with.txt—this avoids git stash push/pop
and the STASHED flag and prevents restoring unrelated stashes.
Comment on lines
+8
to
+10
| if ! command -v setup-envtest &> /dev/null; then | ||
| echo "run 'make install-tools' to install setup-envtest" | ||
| fi |
There was a problem hiding this comment.
Script continues even when setup-envtest is missing.
The check detects when setup-envtest is not installed but doesn't exit, causing the script to fail later with a confusing error when it tries to execute setup-envtest use.
🐛 Proposed fix
if ! command -v setup-envtest &> /dev/null; then
echo "run 'make install-tools' to install setup-envtest"
+ exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ! command -v setup-envtest &> /dev/null; then | |
| echo "run 'make install-tools' to install setup-envtest" | |
| fi | |
| if ! command -v setup-envtest &> /dev/null; then | |
| echo "run 'make install-tools' to install setup-envtest" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/setup-envtest.sh` around lines 8 - 10, The script currently warns when
the setup-envtest binary is missing but continues and later fails; modify the
hack/setup-envtest.sh check so that after detecting command -v setup-envtest
returns false it exits with a non-zero status (e.g., echo the message and run
exit 1) to stop execution immediately; reference the check that invokes
setup-envtest and ensure any callers won’t proceed when the binary is absent.
Comment on lines
+290
to
+314
| // BenchmarkConcurrentApply benchmarks concurrent apply operations | ||
| func BenchmarkConcurrentApply(b *testing.B) { | ||
| _, cfg := setupTestEnv(b) | ||
|
|
||
| logger := log.New() | ||
| logger.SetLevel(log.ErrorLevel) | ||
|
|
||
| helper, err := NewHelper(logger, FromRESTConfig(cfg), WithControllerName("benchmark")) | ||
| if err != nil { | ||
| b.Fatalf("failed to create helper: %v", err) | ||
| } | ||
|
|
||
| b.RunParallel(func(pb *testing.PB) { | ||
| resources := [][]byte{configMapYAML, secretYAML, serviceAccountYAML} | ||
| i := 0 | ||
| for pb.Next() { | ||
| resource := resources[i%len(resources)] | ||
| _, err := helper.Apply(resource) | ||
| if err != nil { | ||
| b.Fatalf("apply failed: %v", err) | ||
| } | ||
| i++ | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Calling b.Fatalf inside b.RunParallel is unsafe.
b.Fatalf should not be called from goroutines spawned by b.RunParallel. It calls runtime.Goexit() which only exits the current goroutine, potentially causing race conditions or incomplete test termination. Use error collection with synchronization instead.
🔧 Proposed fix using atomic error tracking
// BenchmarkConcurrentApply benchmarks concurrent apply operations
func BenchmarkConcurrentApply(b *testing.B) {
_, cfg := setupTestEnv(b)
logger := log.New()
logger.SetLevel(log.ErrorLevel)
helper, err := NewHelper(logger, FromRESTConfig(cfg), WithControllerName("benchmark"))
if err != nil {
b.Fatalf("failed to create helper: %v", err)
}
+ var applyErr atomic.Pointer[error]
b.RunParallel(func(pb *testing.PB) {
resources := [][]byte{configMapYAML, secretYAML, serviceAccountYAML}
i := 0
for pb.Next() {
resource := resources[i%len(resources)]
_, err := helper.Apply(resource)
if err != nil {
- b.Fatalf("apply failed: %v", err)
+ applyErr.CompareAndSwap(nil, &err)
+ return
}
i++
}
})
+ if e := applyErr.Load(); e != nil {
+ b.Fatalf("apply failed: %v", *e)
+ }
}Add the import for sync/atomic at the top of the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/resource/apply_benchmark_test.go` around lines 290 - 314, The benchmark
calls b.Fatalf from within goroutines launched by b.RunParallel (in
BenchmarkConcurrentApply and the inner closure using helper.Apply), which is
unsafe; change the error handling to record the first error from concurrent
goroutines using a concurrency-safe mechanism (e.g., an atomic.Value or an int32
flag plus storing the error string) and call b.Fatalf (or b.Fatal) exactly once
from the main benchmark goroutine after RunParallel returns if any error was
recorded; add the sync/atomic import and modify the loop to set the atomic
error/flag when helper.Apply returns an error instead of calling b.Fatalf
directly.
Comment on lines
+299
to
+306
| if info, err = os.Stat(path); os.IsNotExist(err) { | ||
| if options.ErrorIfPathMissing { | ||
| return nil, err | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| if !info.IsDir() { |
There was a problem hiding this comment.
Handle non-ENOENT os.Stat errors before dereferencing info.
Line 299 only special-cases os.IsNotExist(err). A permission or transient I/O error leaves info == nil, and Line 306 then panics on info.IsDir() instead of returning the original error.
Proposed fix
- // Return the error if ErrorIfPathMissing exists
- if info, err = os.Stat(path); os.IsNotExist(err) {
- if options.ErrorIfPathMissing {
- return nil, err
- }
- continue
- }
+ if info, err = os.Stat(path); err != nil {
+ if os.IsNotExist(err) && !options.ErrorIfPathMissing {
+ continue
+ }
+ return nil, err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if info, err = os.Stat(path); os.IsNotExist(err) { | |
| if options.ErrorIfPathMissing { | |
| return nil, err | |
| } | |
| continue | |
| } | |
| if !info.IsDir() { | |
| if info, err = os.Stat(path); err != nil { | |
| if os.IsNotExist(err) && !options.ErrorIfPathMissing { | |
| continue | |
| } | |
| return nil, err | |
| } | |
| if !info.IsDir() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/sigs.k8s.io/controller-runtime/pkg/envtest/crd.go` around lines 299 -
306, The code calls os.Stat(path) and only checks os.IsNotExist(err), then
dereferences info via info.IsDir(), which can panic for non-ENOENT errors;
update the logic around the os.Stat call (the variables info and err) so you
first check if err != nil: if os.IsNotExist(err) then handle
options.ErrorIfPathMissing as before (continue or return), otherwise return the
original err; only when err == nil should you dereference info and call
info.IsDir(). Ensure this change is applied to the block that uses os.Stat(path)
and the surrounding control flow that references options.ErrorIfPathMissing.
Comment on lines
+111
to
+118
| func updateClientConfig(cc *admissionv1.WebhookClientConfig, hostPort string, caData []byte) { | ||
| cc.CABundle = caData | ||
| if cc.Service != nil && cc.Service.Path != nil { | ||
| url := fmt.Sprintf("https://%s/%s", hostPort, *cc.Service.Path) | ||
| cc.URL = &url | ||
| cc.Service = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Rewrite service-based webhook configs even when service.path is omitted.
admissionregistration.k8s.io/v1 makes Service.Path optional. With the current cc.Service != nil && cc.Service.Path != nil guard, manifests that omit path keep the in-cluster Service reference and never get switched to the local envtest URL.
Proposed fix
func updateClientConfig(cc *admissionv1.WebhookClientConfig, hostPort string, caData []byte) {
cc.CABundle = caData
- if cc.Service != nil && cc.Service.Path != nil {
- url := fmt.Sprintf("https://%s/%s", hostPort, *cc.Service.Path)
+ if cc.Service != nil {
+ path := ""
+ if cc.Service.Path != nil {
+ path = *cc.Service.Path
+ }
+ url := fmt.Sprintf("https://%s%s", hostPort, path)
cc.URL = &url
cc.Service = nil
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/sigs.k8s.io/controller-runtime/pkg/envtest/webhook.go` around lines
111 - 118, The updateClientConfig function currently only rewrites service-based
configs when cc.Service.Path is set; change it to rewrite whenever cc.Service !=
nil so manifests that omit Service.Path are converted to the local envtest URL.
In updateClientConfig, keep setting cc.CABundle, then if cc.Service != nil build
a URL using hostPort and the optional path (e.g. if cc.Service.Path != nil
append "/" + *cc.Service.Path, otherwise use hostPort with no extra path or a
single "/"), assign cc.URL to that value and set cc.Service = nil; reference the
updateClientConfig function and the cc.Service / cc.Service.Path symbols to
locate the change.
Comment on lines
+269
to
+276
| if e.VerifySum && e.Platform.Hash == nil { | ||
| if err := e.Client.FetchSum(ctx, *e.Version.AsConcrete(), &e.Platform); err != nil { | ||
| ExitCause(2, err, "unable to fetch hash for requested version") | ||
| } | ||
| } | ||
| if !e.VerifySum { | ||
| e.Platform.Hash = nil // skip verification | ||
| } |
There was a problem hiding this comment.
VerifySum never validates the downloaded archive.
Lines 269-276 fetch e.Platform.Hash, but Lines 298-310 stream the archive straight into Store.Add. In the code shown here, neither HTTPClient.GetVersion nor Store.Add consumes that hash, so a tampered download is still accepted even when VerifySum is true.
Also applies to: 298-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/env/env.go` around
lines 269 - 276, The current flow fetches e.Platform.Hash via e.Client.FetchSum
but never uses it when downloading and storing the archive; modify the download
path (the code that calls e.HTTPClient.GetVersion and then e.Store.Add) to
verify the downloaded bytes against e.Platform.Hash when e.VerifySum is true:
compute the same hash algorithm used by FetchSum (e.g., SHA-256) while streaming
the response (or after full download), compare the computed digest to
e.Platform.Hash, and return/ExitCause on mismatch before calling e.Store.Add;
ensure the verification logic references e.VerifySum, e.Platform.Hash,
e.Client.FetchSum, e.HTTPClient.GetVersion and e.Store.Add so the integrity
check is applied consistently.
Comment on lines
+423
to
+457
| func (e *Env) PathMatches(value string) bool { | ||
| e.Log.V(1).Info("checking if (env var) path represents our desired version", "path", value) | ||
| if value == "" { | ||
| // if we're unset, | ||
| return false | ||
| } | ||
|
|
||
| if e.versionFromPathName(value) { | ||
| e.Log.V(1).Info("path appears to be in our store, using that info", "path", value) | ||
| return true | ||
| } | ||
|
|
||
| e.Log.V(1).Info("path is not in our store, checking for binaries", "path", value) | ||
| for _, expected := range expectedExecutables { | ||
| _, err := e.FS.Stat(filepath.Join(value, expected)) | ||
| if err != nil { | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| // one of our required binaries is missing, return false | ||
| e.Log.V(1).Info("missing required binary in (env var) path", "binary", expected, "path", value) | ||
| return false | ||
| } | ||
| ExitCause(2, err, "unable to check for existence of binary %s from existing (env var) path %s", value, expected) | ||
| } | ||
| } | ||
|
|
||
| // success, all binaries present | ||
| e.Log.V(1).Info("all required binaries present in (env var) path, using that", "path", value) | ||
|
|
||
| // don't bother checking the version, the user explicitly asked us to use this | ||
| // we don't know the version, so set it to wildcard | ||
| e.Version = versions.AnyVersion | ||
| e.Platform.OS = "*" | ||
| e.Platform.Arch = "*" | ||
| e.manualPath = value | ||
| return true |
There was a problem hiding this comment.
Reject store paths that do not satisfy the current version/platform selector.
PathMatches returns true as soon as versionFromPathName can parse a versioned basename. Because versionFromPathName also mutates e.Version and e.Platform, a path like 1.29.0-darwin-amd64 can satisfy a request for a different version or platform.
Also applies to: 464-477
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/env/env.go` around
lines 423 - 457, PathMatches erroneously accepts a path as soon as
versionFromPathName parses a versioned basename, which can mutate
e.Version/e.Platform to values that don't satisfy the caller's requested
selectors; change PathMatches so that it does not immediately accept/commit
parsed values from versionFromPathName — instead, first parse the
version/platform from the basename (call versionFromPathName in a non-mutating
way or extract the parsed result without assigning to e.Version/e.Platform),
verify the parsed version/platform satisfy the current selector/constraints, and
only then set e.Version, e.Platform and return true; if the parsed values do not
match the selector, continue checking binaries (and fallback logic) as before.
Ensure the same non-mutating verification approach is applied to the analogous
code path later in the file that currently returns early after
versionFromPathName succeeds.
Comment on lines
+31
to
+34
| // PrintEnv prints the path with the corresponding env variable, so that | ||
| // you can source the output like | ||
| // `source $(fetch-envtest switch -p env 1.20.x)`. | ||
| PrintEnv |
There was a problem hiding this comment.
Comment example appears to use the wrong command name.
Line 33 shows fetch-envtest, but the tool/documentation uses setup-envtest. This can confuse future edits.
✏️ Suggested comment fix
- // `source $(fetch-envtest switch -p env 1.20.x)`.
+ // `source $(setup-envtest use -p env 1.20.x)`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/env/helpers.go`
around lines 31 - 34, Update the doc comment above the PrintEnv symbol to use
the correct command name "setup-envtest" instead of "fetch-envtest"; locate the
comment block referencing PrintEnv in helpers.go and replace the example
invocation ``source $(fetch-envtest switch -p env 1.20.x)`` with ``source
$(setup-envtest switch -p env 1.20.x)`` so the documentation matches the actual
tool name.
Comment on lines
+107
to
+112
| As noted about, you can use `ENVTEST_INSTALLED_ONLY=true` to switch `-i` | ||
| on by default, and you can use `ENVTEST_USE_ENV=true` to switch | ||
| `--use-env` on by default. | ||
|
|
||
| - If you want to use this tool, but download your gziped tarballs | ||
| separately, you can use the `sideload` command. You'll need to use the |
There was a problem hiding this comment.
Fix small wording typos in offline usage docs.
Line 107 reads “As noted about” (likely “As noted above”), and Line 111 uses “gziped” (should be “gzipped”).
✏️ Suggested doc fix
- As noted about, you can use `ENVTEST_INSTALLED_ONLY=true` to switch `-i`
+ As noted above, you can use `ENVTEST_INSTALLED_ONLY=true` to switch `-i`
@@
-- If you want to use this tool, but download your gziped tarballs
+- If you want to use this tool, but download your gzipped tarballs📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| As noted about, you can use `ENVTEST_INSTALLED_ONLY=true` to switch `-i` | |
| on by default, and you can use `ENVTEST_USE_ENV=true` to switch | |
| `--use-env` on by default. | |
| - If you want to use this tool, but download your gziped tarballs | |
| separately, you can use the `sideload` command. You'll need to use the | |
| As noted above, you can use `ENVTEST_INSTALLED_ONLY=true` to switch `-i` | |
| on by default, and you can use `ENVTEST_USE_ENV=true` to switch | |
| `--use-env` on by default. | |
| - If you want to use this tool, but download your gzipped tarballs | |
| separately, you can use the `sideload` command. You'll need to use the |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/README.md` around
lines 107 - 112, Fix two typos in the README text: change “As noted about” to
“As noted above” and “gziped” to “gzipped” in the paragraph that mentions
ENVTEST_INSTALLED_ONLY and ENVTEST_USE_ENV and the follow-up sentence about the
sideload command; update the strings in the README (look for occurrences of
ENVTEST_INSTALLED_ONLY, ENVTEST_USE_ENV, and “sideload”) so the corrected
wording reads naturally.
Comment on lines
+35
to
+41
| func (p Platform) BaseName(ver Concrete) string { | ||
| return fmt.Sprintf("%d.%d.%d-%s-%s", ver.Major, ver.Minor, ver.Patch, p.OS, p.Arch) | ||
| } | ||
|
|
||
| // ArchiveName returns the full archive name for this version and platform. | ||
| func (p Platform) ArchiveName(ver Concrete) string { | ||
| return "envtest-v" + p.BaseName(ver) + ".tar.gz" |
There was a problem hiding this comment.
Preserve prerelease identifiers in BaseName.
ExtractWithPlatform and ConcreteVersionRE support prerelease components, but BaseName collapses everything to major.minor.patch. That makes versions like 1.31.0-alpha.1 and 1.31.0 share the same store/archive name and breaks round-tripping through VersionPlatformRE.
Proposed fix
func (p Platform) BaseName(ver Concrete) string {
- return fmt.Sprintf("%d.%d.%d-%s-%s", ver.Major, ver.Minor, ver.Patch, p.OS, p.Arch)
+ return fmt.Sprintf("%s-%s-%s", ver, p.OS, p.Arch)
}Also applies to: 94-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest/versions/platform.go`
around lines 35 - 41, BaseName currently formats only major.minor.patch which
drops prerelease parts and causes collisions; update Platform.BaseName to
include ver.Prerelease (and any build metadata if present) when constructing the
version string so prerelease-aware versions (e.g., 1.31.0-alpha.1) stay
distinct, and ensure ArchiveName (which uses BaseName) inherits that change;
locate and update the BaseName method in the Platform type and any other places
that duplicate the base-name construction (see functions named BaseName and
ArchiveName and the regex helpers ExtractWithPlatform / ConcreteVersionRE /
VersionPlatformRE for consistency) so stored/archive names preserve prerelease
identifiers.
Contributor
Author
|
need a way to exclude vendor changes from coderabbit... |
Contributor
|
@dlom: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2873 +/- ##
==========================================
- Coverage 50.30% 50.27% -0.03%
==========================================
Files 280 281 +1
Lines 34310 34327 +17
==========================================
Hits 17259 17259
- Misses 15690 15707 +17
Partials 1361 1361
🚀 New features to boost your workflow:
|
Member
Contributor
Author
|
closing this for now, we need a better benchmark suite first in a separate PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
xref: HIVE-2139
This is the first of potentially multiple PRs addressing memory issues in the clustersync controller (and hive in general). This PR is split into two commits: The first adds a basic benchmark harness for the resource helper, focused mainly on driving the
ApplyandPatchmethods. This pulls inenvtestas a test dependency. The second commit is the actual optimization: reusing allocated buffers in the resource helper.Claude wrote the benchmarks and the comparison script for me, so take them with a grain of salt
Preliminary benchmark results:
-benchtime=3x -count=6My opinion: 23% speedup from not allocating buffers (and not GCing them) is great, but the memory effects are negligible. Slight overhead on memory from the pool (+0.02%), and the actual allocation savings are a drop in the bucket compared to the rest of the allocations made (-0.08%).
/assign @2uasimojo
Summary by CodeRabbit
New Features
Tests
Chores