Skip to content

Conversation

@klihub
Copy link
Member

@klihub klihub commented Feb 22, 2023

When adjusting CPU or memory resources, check for each field whether it is set (IOW adjusted by a plugin) before updating. The field getters return the corresponding golang zero value for unset fields. Therefore unconditionally adjusting unset Spec fields resets the fields instead. This manifests itself by all container CPU resources getting cleared when NRI is enabled but no plugins are running or none of the running plugins are adjust resources.

When adjusting CPU or memory resources, check for each field
whether it is set (IOW adjusted by a plugin) before updating.
The field getters return the corresponding golang zero value
for unset fields. Therefore unconditionally adjusting unset
Spec fields resets the fields instead. This manifests itself
by all container CPU resources getting cleared when NRI is
enabled but no plugins are running or none of the running
plugins are adjust resources.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the fixes/resource-adjustment branch from 0a80942 to b21fd1d Compare February 22, 2023 20:16
@codecov-commenter
Copy link

Codecov Report

Base: 69.75% // Head: 63.33% // Decreases project coverage by -6.43% ⚠️

Coverage data is based on head (b97b0b9) compared to base (2dfff71).
Patch coverage: 71.42% of modified lines in pull request are covered.

❗ Current head b97b0b9 differs from pull request most recent head b21fd1d. Consider uploading reports for the commit b21fd1d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
- Coverage   69.75%   63.33%   -6.43%     
==========================================
  Files           7        9       +2     
  Lines        1531     1781     +250     
==========================================
+ Hits         1068     1128      +60     
- Misses        333      502     +169     
- Partials      130      151      +21     
Impacted Files Coverage Δ
pkg/runtime-tools/generate/generate.go 27.85% <71.42%> (ø)
pkg/net/multiplex/mux.go 67.52% <0.00%> (-0.52%) ⬇️
pkg/runtime-tools/generate/helpers_linux.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@klihub klihub force-pushed the fixes/resource-adjustment branch from b21fd1d to 85fdf6a Compare February 22, 2023 20:58
@klihub klihub requested a review from fuweid February 22, 2023 21:09
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

return spec
}

func Int64(v int64) *int64 {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use generic here, like

func ToPtr[T any](v T) *T {
    return &v
}

Just share a idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks ! Good point and I'll include that change in a subsequent PR when adding more test cases to the suite.

@fuweid fuweid merged commit f6a7cac into containerd:main Feb 23, 2023
@klihub klihub deleted the fixes/resource-adjustment branch February 23, 2023 08:32
@klihub
Copy link
Member Author

klihub commented Feb 23, 2023

@fuweid Could we tag the current HEAD as v0.3.0 ? Then I could update cobtsinerd #8140 to point at it.

I might have one more fix that we could try to sqeeze in, but it should not break backward compatibility, so I think that wouldn't require a tag (and we could always tag it as v0.3.1 if we prefer releases to be tag-aligned).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants