CORS-4005: refactor worker manifest serialization#10407
CORS-4005: refactor worker manifest serialization#10407patrickdillon wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request references CORS-4005 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. |
WalkthroughAdded an internal Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test e2e-azure-ovn e2e-vsphere-ovn e2e-gcp-ovn e2e-openstack-ovn e2e-nutanix-ovn e2e-ibmcloud-ovn |
|
@jcpowermac @vr4manta vsphere ip address claims are affected here. which tests would exercise those so we can see the address claim manifests? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/machines/worker.go`:
- Around line 730-738: The loops that append pointers currently capture the
reused range variables (for _, m := range data.Machines, for _, c := range
data.IPClaims, for _, a := range data.IPAddresses) causing pointer aliasing; fix
by creating a new local copy inside each loop (e.g., mCopy := m / cCopy := c /
aCopy := a) and append the address of that copy to machines, ipClaims, and
ipAddrs respectively so each slice element points to a distinct struct instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 284beed1-37a1-40ca-aaa8-bed89d5f703a
📒 Files selected for processing (1)
pkg/asset/machines/worker.go
| for _, m := range data.Machines { | ||
| machines = append(machines, &m) | ||
| } | ||
| for _, c := range data.IPClaims { | ||
| ipClaims = append(ipClaims, &c) | ||
| } | ||
| for _, a := range data.IPAddresses { | ||
| ipAddrs = append(ipAddrs, &a) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant loop + append sites.
rg -n -C2 'for _, m := range data\.Machines|machines = append\(machines, &m\)|for _, c := range data\.IPClaims|ipClaims = append\(ipClaims, &c\)|for _, a := range data\.IPAddresses|ipAddrs = append\(ipAddrs, &a\)' pkg/asset/machines/worker.go
# After the fix, this should produce no matches.
rg -n 'append\((machines|ipClaims|ipAddrs), &[mca]\)' pkg/asset/machines/worker.go || trueRepository: openshift/installer
Length of output: 483
Fix pointer aliasing in range loops before serialization
At Lines 731, 734, and 737, taking &m, &c, and &a captures reused range variables. All appended pointers will reference the same memory location, ending up with slices containing repeated pointers to the last iterated object instead of distinct elements.
Proposed fix
- for _, m := range data.Machines {
- machines = append(machines, &m)
- }
- for _, c := range data.IPClaims {
- ipClaims = append(ipClaims, &c)
- }
- for _, a := range data.IPAddresses {
- ipAddrs = append(ipAddrs, &a)
- }
+ for i := range data.Machines {
+ machines = append(machines, &data.Machines[i])
+ }
+ for i := range data.IPClaims {
+ ipClaims = append(ipClaims, &data.IPClaims[i])
+ }
+ for i := range data.IPAddresses {
+ ipAddrs = append(ipAddrs, &data.IPAddresses[i])
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/machines/worker.go` around lines 730 - 738, The loops that append
pointers currently capture the reused range variables (for _, m := range
data.Machines, for _, c := range data.IPClaims, for _, a := range
data.IPAddresses) causing pointer aliasing; fix by creating a new local copy
inside each loop (e.g., mCopy := m / cCopy := c / aCopy := a) and append the
address of that copy to machines, ipClaims, and ipAddrs respectively so each
slice element points to a distinct struct instance.
There was a problem hiding this comment.
No fix needed. The module uses Go 1.25, and since Go 1.22, range variables are scoped per iteration — each m, c, and a is a fresh variable, so &m, &c, and &a point to distinct instances. There's no aliasing bug here.
There was a problem hiding this comment.
See https://go.dev/doc/go1.22#language for release note that contains the fix for pointer aliasing.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Make manifest serialization a common, shared function.
patrickdillon
force-pushed
the
refactor-worker-serialize
branch
from
9771fbb to
d2e1be6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/machines/worker.go`:
- Around line 937-955: In serialize, compute the key and final filename before
calling yaml.Marshal so error messages use the actual filename: generate key
(using padFormat or meta.Accessor when useObjectName is true) and build filename
:= filepath.Join(directory, fmt.Sprintf(fileName, key)) prior to marshaling,
then call yaml.Marshal(m) and on marshal error return fmt.Errorf("marshaling %s:
%w", filename, err); finally use that same filename when creating the asset.File
(asset.File{Filename: filename, ...}) to ensure consistent, correct error output
and file assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fd89d48-82c8-42ad-8c9c-ed3c61013847
📒 Files selected for processing (1)
pkg/asset/machines/worker.go
| func serialize(manifests []runtime.Object, fileName string, useObjectName bool) ([]*asset.File, error) { | ||
| files := make([]*asset.File, len(manifests)) | ||
| padFormat := fmt.Sprintf("%%0%dd", len(fmt.Sprintf("%d", len(manifests)))) | ||
| for i, m := range manifests { | ||
| data, err := yaml.Marshal(m) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshaling %s: %w", fmt.Sprintf(fileName, i), err) | ||
| } | ||
|
|
||
| key := fmt.Sprintf(padFormat, i) | ||
| if useObjectName { | ||
| accessor, err := meta.Accessor(m) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("accessing object metadata: %w", err) | ||
| } | ||
| key = accessor.GetName() | ||
| } | ||
| files[i] = &asset.File{ | ||
| Filename: filepath.Join(directory, fmt.Sprintf(fileName, key)), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "serialize() error path:"
sed -n '937,945p' pkg/asset/machines/worker.go
echo
echo "Filename format definitions used by the changed call sites:"
rg -n -C1 'workerMachineSetFileName =|workerMachineFileName =|ipClaimFileName\s*=|ipAddressFileName\s*=' pkg/asset/machinesRepository: openshift/installer
Length of output: 1544
🏁 Script executed:
cat -n pkg/asset/machines/worker.go | sed -n '937,960p'Repository: openshift/installer
Length of output: 974
Move key and filename computation before the marshal call to fix the error message.
Line 943 uses fmt.Sprintf(fileName, i) where fileName is a format string with %s placeholder (e.g., "99_openshift-cluster-api_worker-machines-%s.yaml"), but i is an int. This produces malformed error output like marshaling 99_openshift-cluster-api_worker-machines-%!s(int=0).yaml: ... instead of the actual filename. Build key and filename first, then reuse filename in both the error message and the File assignment.
Suggested fix
func serialize(manifests []runtime.Object, fileName string, useObjectName bool) ([]*asset.File, error) {
files := make([]*asset.File, len(manifests))
padFormat := fmt.Sprintf("%%0%dd", len(fmt.Sprintf("%d", len(manifests))))
for i, m := range manifests {
- data, err := yaml.Marshal(m)
- if err != nil {
- return nil, fmt.Errorf("marshaling %s: %w", fmt.Sprintf(fileName, i), err)
- }
-
key := fmt.Sprintf(padFormat, i)
if useObjectName {
accessor, err := meta.Accessor(m)
if err != nil {
return nil, fmt.Errorf("accessing object metadata: %w", err)
}
key = accessor.GetName()
}
+ filename := filepath.Join(directory, fmt.Sprintf(fileName, key))
+
+ data, err := yaml.Marshal(m)
+ if err != nil {
+ return nil, fmt.Errorf("marshaling %s: %w", filename, err)
+ }
files[i] = &asset.File{
- Filename: filepath.Join(directory, fmt.Sprintf(fileName, key)),
+ Filename: filename,
Data: data,
}
}
return files, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/machines/worker.go` around lines 937 - 955, In serialize, compute
the key and final filename before calling yaml.Marshal so error messages use the
actual filename: generate key (using padFormat or meta.Accessor when
useObjectName is true) and build filename := filepath.Join(directory,
fmt.Sprintf(fileName, key)) prior to marshaling, then call yaml.Marshal(m) and
on marshal error return fmt.Errorf("marshaling %s: %w", filename, err); finally
use that same filename when creating the asset.File (asset.File{Filename:
filename, ...}) to ensure consistent, correct error output and file assignment.
There was a problem hiding this comment.
+1 here. It should only affect error message, but makes sense to fix 👍
There was a problem hiding this comment.
@tthvo, thanks for the confirmation! Here's the planned fix:
- Move
keycomputation beforeyaml.Marshalinserialize() - Build
filenamefromkeybefore marshaling - Use
filenamein both the error message and theasset.Fileassignment
Failed to handle agent chat message. Please try again.
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/approve
I just have a nit and coderabbit also had a good suggestion above :D
| // e.g. "99_openshift-machine-api_claim-cluster-worker-0-claim-0-0.yaml". | ||
| // When false, a zero-padded index is used instead, | ||
| // e.g. "99_openshift-cluster-api_worker-machineset-0.yaml". | ||
| func serialize(manifests []runtime.Object, fileName string, useObjectName bool) ([]*asset.File, error) { |
There was a problem hiding this comment.
| func serialize(manifests []runtime.Object, fileName string, useObjectName bool) ([]*asset.File, error) { | |
| func serialize(manifests []runtime.Object, fileNameTemplate string, useObjectName bool) ([]*asset.File, error) { |
nit: It's actually a name template, right? Probably clearer to use it this way.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tthvo 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
I am guessing these jobs (I noticed last time we fixed the IPAM version in #10169) 👀 /test e2e-vsphere-static-ovn |
|
/payload-job periodic-ci-openshift-release-master-nightly-4.22-e2e-vsphere-static-ovn-techpreview |
|
@tthvo: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-vsphere-static-ovn-techpreview |
|
@tthvo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/877a1590-2310-11f1-98cf-849bf6d4e350-0 |
3 participants
Make manifest serialization a common, shared function.
This is in preparation for adding CAPI compute node manifests, where we will be expanding this boiler plate code even more.