Result: Merged (2026-10-22 20:19, kubevirt/containerized-data-importer#4320). Actually retrieved from GitHub PRs, reviews, comments, timelines, test records, CDI source (the base commit of the PR
9cd4fbdb), Go standard library documentation, and the draft of the contribution ledger. All times are in KST (Korea Standard Time).
Table of Contents
- One-Line Summary
- 1. What Was the Issue: Two Lines Found While Reading Code
- 2. Cause: Which Line in the Source
- 3. What Was Changed: Diff Summary
- 4. Reaction: Approval in 16 Hours, Then a Single e2e Test
- 5. What I Learned: Tips for Open Source Contributions
- Links
One-Line Summary
| PR | kubevirt/containerized-data-importer#4320 Log the storage class name, not its pointer, in filesystem overhead |
| Changes | pkg/controller/common/util.go Two log lines in . 2 lines added, 2 lines deleted |
| Reaction | Maintainer approved after 16 hours /approve·/lgtm. When an unrelated e2e failure was explained, a maintainer overrode it |
| Result | PR opened 10-20 00:49 → Approved 17:05 → Overrode 10-22 16:42 → Merged 20:19, approx. 2 days, 19 hours, and 30 minutes |
1. What Was the Issue: Two Lines Found While Reading Code
This issue was not found in production logs. While writing the "Reading KubeVirt Source Code" serieson this blog and following CDI's size calculation logic, I jotted it down as the fifth item on the contribution ledger's "Contribution Candidates Found by Reading Code" list. This log is located in the function covered in Part 8that selects filesystem headroom values per storage class.
GetFilesystemOverheadForStorageClass checks for a default class if the storage class is not found, and falls back to global headroom if that is also absent. At these two branching points, it logs at verbosity level 3 (-v=3). The output noted in the PR description was as follows:
Storage class0x7035abda0020not found, trying default storage class
A memory address appears where the storage class name should be, and there are no spaces around it. After the fix, it prints like this:
Storage class "local-path" not found, trying default storage class
The PR description stated that this output was reproduced, but the original output of that reproduction does not remain in the contribution ledger. The two lines above are taken from the example in the PR description. I also did not verify whether CDI in the TT Lab cluster was running at verbosity level 3 (unverified).
2. Cause: Which Line in the Source
Line numbers are based on the base commit of the PR 9cd4fbdb .
pkg/controller/common/util.goline 650:klog.V(3).Info("Storage class", storageClassName, "not found, trying default storage class")- Line 664 in the same file is:
klog.V(3).Info("Storage class", storageClassName, "not found, continuing with global overhead")
storageClassName . *string This appears to be a mistake stemming from the common Kubernetes API pattern of receiving "strings that can be empty" as pointers.
Two factors combine to produce the output above.
klog.Infojoins arguments using thefmt.Sprintmethod. Go'sfmt.Sprintdocumentationstates that "spaces are added only when neither operand is a string." Between"Storage class"and the pointer, and between the pointer and"not found…", spaces are omitted because one side is a string.%vPrinting a pointer with outputs the address rather than the referenced value. Consequently, values like0x7035abda0020end up in the name position.
Info Replacing Infof with and providing a format string resolves the spacing issue. Pointers must be dereferenced, but since they might be nil, I used
k8s.io/utils/ptr 's ptr.Deref(storageClassName, "") which was already imported in this file.
3. What Was Changed: Diff Summary
targetStorageClass, err := GetStorageClassByNameWithK8sFallback(ctx, client, storageClassName)
if err != nil || targetStorageClass == nil {
- klog.V(3).Info("Storage class", storageClassName, "not found, trying default storage class")
+ klog.V(3).Infof("Storage class %q not found, trying default storage class", ptr.Deref(storageClassName, ""))
...
if targetStorageClass == nil {
- klog.V(3).Info("Storage class", storageClassName, "not found, continuing with global overhead")
+ klog.V(3).Infof("Storage class %q not found, continuing with global overhead", ptr.Deref(storageClassName, ""))
%q Choosing ensures that even when the name is an empty string, it appears as "" . The other log in the same function, "no default storage class found", was left as is because it takes a single string argument and had no issues. The PR description noted that the change only modifies logs and stated what was verified. go vet ./pkg/controller/common/ was clean, and all 43 tests in
go test ./pkg/controller/common/ passed (golang:1.27.2 container, -mod=vendor). No separate issue was opened; I simply noted that it was "found while reading code".
The use of AI was disclosed in the commit trailer Assisted-by: Claude , the DCO sign-off, and a paragraph at the end of the body stating, "Found and fixed with the help of Claude, reviewed, and reproduced the log output above." This commit also appears as an unsigned commit on GitHub. It seems the SSH signature required by rule 3 was missing.
4. Reaction: Approval in 16 Hours, Then a Single e2e Test
| Time | Day |
|---|---|
| 10-20 00:49 | PR opened. Prow assigned the release-note-none·size/XS label and two reviewers |
| 10-20 17:05 | Maintainer reviewed with /approve·/lgtm and a word of thanks. lgtm·approved label |
| 10-21 22:23 | We ran /retest-required , but the bot replied that "untrusted authors cannot trigger tests directly" |
| 10-22 01:05 | Comment detailing the failed test and asking for a retest |
| 10-22 16:42 | Maintainer /override pull-containerized-data-importer-e2e-ceph-wffc |
| 10-22 20:19 | tide merged |
Approval was quick. Since it was just two lines with the before-and-after logs shown side-by-side, there likely wasn't much to question. This maintainer is the same person who reviewed Part 7and Part 6.
What blocked merging after approval was the e2e suite e2e-ceph-wffc . Judging by the name, it appears to be a suite running with Ceph storage and late binding (WaitForFirstConsumer). There was only a single failure, a test named
DataImportCron … with snapshot sources [test_id:10033] . Out of 546 tests, 545 passed. It seemed highly unlikely that a change modifying two log strings could break a DataImportCron test involving snapshot sources.
So I first ran the retest command /retest-required , but Prow rejected it, stating it was a command from an external author. In this repository, untrusted authors cannot trigger tests with /ok-to-test or rerun them. A few hours later, I posted a comment noting the failed test name and the 545/546 count, explained in a single sentence why this change had nothing to do with that test, and asked someone to trigger a retest. Instead of retesting, the maintainer overrode the check, and it was merged four hours later.
5. What I Learned: Open Source Contribution Tips
- 「
InfoLogs that concatenate values with commas deserve a second look.」fmt.SprintDue 's spacing rules, mixing string and non-string values causes spaces to disappear. Mixing pointers in results in addresses instead of values.Infofand%qalong with a nil-safe dereference are the shortest solution. - 「Code reading is also a source of contributions.Even without experiencing inconvenience in production, tracing code line-by-line to understand its architecture reveals things like this. I noted what I found in the ledger's candidate list and submitted it separately as a small PR while the larger PR (Part 7) was under review.
- 「External contributors explain rather than retesting.When you lack the permissions to rerun tests yourself, providing the information for a maintainer to make a judgment is what you can do. I included the failed test name, the number of passed tests, and the reason why the change was unrelated in a single comment. The maintainer saw that and chose to override.
- 「When commands are blocked, read the bot's response.」
/retest-requiredRunning
was a dead end, but the bot's response clearly explained what external authors cannot do in this repository. It's the same rule seen in the main KubeVirt repository in Part 6. Titi's Word.
Links
All materials were accessed on 2026-10-29.
- PR: kubevirt/containerized-data-importer#4320 (Merge commit
b2e1e1e8) - Fixed source (base commit):
pkg/controller/common/util.goline 650, line 664 - Go 문서:
fmt.Sprint,k8s.io/utils/ptr.Deref - PR explaining filesystem headroom calculations: Part 8, containerized-data-importer#4318