Skip to content

Deep-copy CD object only when mutation needed #647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jgehrcke wants to merge 1 commit into from
Closed

Deep-copy CD object only when mutation needed #647

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions cmd/compute-domain-daemon/computedomain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,19 +220,11 @@ func (m *ComputeDomainManager) onAddOrUpdate(ctx context.Context, obj any) error
func (m *ComputeDomainManager) UpdateComputeDomainNodeInfo(ctx context.Context, cd *nvapi.ComputeDomain) (rerr error) {
var nodeInfo *nvapi.ComputeDomainNode

// Create a deep copy of the ComputeDomain to avoid modifying the original
newCD := cd.DeepCopy()

defer func() {
if rerr == nil {
m.MaybePushNodesUpdate(newCD)
}
}()

// Try to find an existing entry for the current k8s node
for _, node := range newCD.Status.Nodes {
for _, node := range cd.Status.Nodes {
Copy link
Copy Markdown
Contributor

@klueska klueska Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be done on the newCD, otherwise we run the risk of this object changing out from under us (it's a pointer).

Copy link
Copy Markdown
Contributor Author

@jgehrcke jgehrcke Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're saying that

nodeInfo = node.DeepCopy()

right below won't do the trick because we're just copying a pointer?

Will review.

Copy link
Copy Markdown
Contributor

@klueska klueska Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm saying we can't loop over cd.Status.Nodes because that might change in the course of the loop because cd is a pointer into an object in the informer cache.

In this particular case it might be fine since there are no pointers in cd.Status.Nodes itself, but in general we would need to (at a minimum) iterate over a deep copy of cd.Status.Nodes to be sure nothing embedded in the object we are looking at gets changed behind our backs.

Copy link
Copy Markdown
Contributor Author

@jgehrcke jgehrcke Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some conclusions:

  • Of course because we pass the CD in by pointer there could hypothetically be a concurrent execution unit mutating the object underneath us (we know/assume this to not be true right now)
  • DeepCopy() doesn't generally protect from concurrent mutation (it might happen during DeepCopy execution, and only locking would be able to guard against that).
  • The informer itself is not mutating the CD object underneath us (other callbacks however may, if they run concurrently -- not sure if they ever do).
  • Performance of just DeepCopy() is not a concern; it may only become significant when we call this function at pathologically high rate (in which case we should look into changing that).

Let's come back to this later. And when we do: we probably want to adjust all similar places in the same way, for symmetry. Maybe even using common code.

if node.Name == m.config.nodeName {
nodeInfo = node
// Create copy because we may mutate it below.
nodeInfo = node.DeepCopy()
break
}
}
Expand All @@ -242,6 +234,15 @@ func (m *ComputeDomainManager) UpdateComputeDomainNodeInfo(ctx context.Context,
return nil
}

// Create a deep copy of the ComputeDomain to avoid modifying the original
newCD := cd.DeepCopy()

defer func() {
if rerr == nil {
m.MaybePushNodesUpdate(newCD)
}
}()

// If there isn't one, create one and append it to the list
if nodeInfo == nil {
// Get the next available index for this new node
Expand Down