extract memory and CPU resource helpers

[ErikJiang]

Refactored resource processing logic by extracting adjustMemoryResource and adjustCPUResource helper functions to eliminate code duplication.

• Added adjustMemoryResource function for memory resource operations
• Added adjustCPUResource function for CPU resource operations
• Updated adjustResources and updateResources to use new helpers

[chrishenzie]

nit: I think I'm partial to the previous "adjust" method naming, which implies it's making adjustments to the result container and reply

[chrishenzie]

nit: mem is also the same type as the following two arguments. Should we remove this redundant LinuxMemory?

[chrishenzie]

Without context, feeding in an empty struct that's unused here feels like an anti-pattern. What if we were to split up processMemoryResource() so that it only updates targetContainer, and then separately we can deep copy the results to reply? Thoughts on this approach?

[ErikJiang]

Thanks for the review!
I've addressed your suggestions in the latest commit.
Please let me know if there are any further adjustments needed. 😊

[klihub]

Those proto.Clone()s are not a good idea (see my other, more detailed comments). Please revert to your original approach where you properly collected the requested changes only in the reply adjustment.

[klihub]

@ErikJiang I think these blind deep copies are not a good idea. Please don't do these. They are altering behavior compared to the original implementation, even if that does not show currently up in the end result.

The original implementation deliberately collected only the requested changes in the reply adjustment. With these changes now if there is any change requested to any of the CPU or memory resources of a container which has non-empty resources, the reply will incorrectly claim that the plugin requested changing every single parameter with the unrequested ones being copies of the original ones. You can see this for yourself easily, if you change the tests so that we don't start with a container with empty resources, for instance using this commit:

commit 38c50ea6bc12092ee6b9fe8cf53c09ca6a96884c (HEAD)
Author: Krisztian Litkey <krisztian.litkey@intel.com>
Date:   Tue Sep 23 14:57:52 2025 +0300

    WiP: adaptation: demo altered semantics of collected resource adjustments.
    
    Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

diff --git a/pkg/adaptation/adaptation_suite_test.go b/pkg/adaptation/adaptation_suite_test.go
index 4a19c7a..e093ded 100644
--- a/pkg/adaptation/adaptation_suite_test.go
+++ b/pkg/adaptation/adaptation_suite_test.go
@@ -628,6 +628,19 @@ var _ = Describe("Plugin container creation adjustments", func() {
                                                        "argument",
                                                        "list",
                                                },
+                                               Linux: &api.LinuxContainer{
+                                                       Resources: &api.LinuxResources{
+                                                               Cpu: &api.LinuxCPU{
+                                                                       Shares: api.UInt64(111),
+                                                                       Quota:  api.Int64(222),
+                                                                       Period: api.UInt64(333),
+                                                               },
+                                                               Memory: &api.LinuxMemory{
+                                                                       Limit: api.Int64(11111),
+                                                                       Swap:  api.Int64(22222),
+                                                               },
+                                                       },
+                                               },
                                        }
                                )

make test (or alternatively ginkgo run ./pkg/adaptation) should now fail with this extra commit and the changes of this PR in place, while it passes without this PR.

I am pretty sure this is not intentional on your part. It is just a side effect of your latest changes (in response to a previous review comment) where you now proto.Clone()-deep-copy resources from the adjusted container in the request, instead of properly collecting them in the reply. Even if this would not have unintended side-effects on the final adjusted container itself now, it might have in the future if we change semantics, and it already might cause false positives now with custom validators and break stuff.

Please switch your PR back to your original approach where this aspect was not changed.

[klihub]

We shouldn't alter the semantics of the collected adjustment. Your original implementation was correct in that sense and we should switch back to that or a functionally equivalent one.

[klihub]

@ErikJiang I filed #231 as minimal protection to prevent such unintentional changes to the semantics in the future.

[chrishenzie]

@klihub @ErikJiang If we're going with the prior approach, can we leave a comment at the callsites briefly explaining why we're passing an empty reply?

[klihub]

@klihub @ErikJiang If we're going with the prior approach, can we leave a comment at the callsites briefly explaining why we're passing an empty reply?

I think instead of an empty reply, we could change the function so that if we pass in a nil instead, it's checked for and skipped, and we'd use a nil instead of a temporary variable which is otherwise unused. It'd seem much clearer that way to me.

[ErikJiang]

@klihub @chrishenzie Thanks for the feedback. I've removed the proto.Clone() and reverted to selectively updating the reply to avoid the side effects you pointed out. The new commit is up for review.

[chrishenzie]

LGTM, thanks!

[klihub]

@ErikJiang LGTM. Thank you !