-
Notifications
You must be signed in to change notification settings - Fork 98
Fix bug in ComputeDomain object being written to mutation cache #721
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.
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
Conversation
Signed-off-by: Kevin Klues <[email protected]>
48b41ad to
ecd1393
Compare
|
/ok to test ecd1393 |
| newCD.Status.Nodes = updatedNodes | ||
| if _, err := m.config.clientsets.Nvidia.ResourceV1beta1().ComputeDomains(newCD.Namespace).UpdateStatus(ctx, newCD, metav1.UpdateOptions{}); err != nil { | ||
| newCD, err = m.config.clientsets.Nvidia.ResourceV1beta1().ComputeDomains(newCD.Namespace).UpdateStatus(ctx, newCD, metav1.UpdateOptions{}) | ||
| if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Connecting dots: we noticed this before; that's the only unresolved comment on #651:
jgehrcke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Of course this is what we want, for various reasons.
However, I still need to convince myself if this patch alone fixes a well-understood bug.
I am slowly catching up with the Slack thread that sparked reviewing this code section (and making the change).
Here, we make a change in removeNodeFromComputeDomain(). I wonder how that part of the business logic relates to the scenario in the user's environment. It appears as if that scenario is about cleanly growing a CD over many nodes (and hence node removal from that CD should not be part of that scenario).
In any case, let's get this in. And then worry about classification of this patch later when we have learned/understood more.
|
Not sure about the bug this fixes, but let's certainly land this. |
|
/cherry-pick release-25.8 |
|
🤖 Backport PR created for |
No description provided.