Skip to content
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

balloons: add "preserve" option to match containers whose pinning must not be modified #368

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

askervin
Copy link
Collaborator

Some containers must be able to run with their resources unmanaged by a resource policy. Currently this can be achieved only by annotating pods. However, in certain environments it is more convenient to change a node-specific policy configuration rather than change annotations of workloads.

This PR adds such an option to the balloons policy. Once the option takes its final shape, we can add similar option to the topology-aware policy, too.

@klihub klihub self-requested a review October 1, 2024 07:33
Copy link
Collaborator

@klihub klihub left a comment

Choose a reason for hiding this comment

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

LGTM, but I wouldn't call this ignored but preserved, since it basically has the same effect as annotating a container preserved.

The preserve option enables specifying containers whose resource
pinning must not be modified in policy configuration. The meaning is
the same as with the preserve annotation for both CPU and memory.

Signed-off-by: Antti Kervinen <[email protected]>
@askervin
Copy link
Collaborator Author

askervin commented Oct 1, 2024

Renamed to preserve. Now it's aligned with the preserve annotation.

Thanks for spotting the typos! I'd prefer fixing typos in resmgr.Expression documentation (in pkg/apis/resmgr/v1alpha1/types.go) in a separate PR. That's because including those fixes into this patch series would only make it more difficult to understand, just by checking modified files in the PR, that which exact source files needed a change in order to add a new option+implementation+test+documentation. Sort of minimizing the blast radius. What do you think, @fmuyassarov?

Because the fix modifies generated code files in this and another option, filing that PR would cause conflicts before this PR is merged. So I could file it after merging.

@askervin askervin changed the title This patch enables ignoring special containers in the resource management balloons: add "preserve" option to match containers whose pinning must not be modified Oct 1, 2024
@fmuyassarov
Copy link
Collaborator

Thanks for spotting the typos! I'd prefer fixing typos in resmgr.Expression documentation (in pkg/apis/resmgr/v1alpha1/types.go) in a separate PR. That's because including those fixes into this patch series would only make it more difficult to understand, just by checking modified files in the PR, that which exact source files needed a change in order to add a new option+implementation+test+documentation. Sort of minimizing the blast radius. What do you think, @fmuyassarov?

Yes, sure. As you prefer.

@klihub
Copy link
Collaborator

klihub commented Oct 1, 2024

Renamed to preserve. Now it's aligned with the preserve annotation.

Thanks for spotting the typos! I'd prefer fixing typos in resmgr.Expression documentation (in pkg/apis/resmgr/v1alpha1/types.go) in a separate PR. That's because including those fixes into this patch series would only make it more difficult to understand, just by checking modified files in the PR, that which exact source files needed a change in order to add a new option+implementation+test+documentation. Sort of minimizing the blast radius. What do you think, @fmuyassarov?

Because the fix modifies generated code files in this and another option, filing that PR would cause conflicts before this PR is merged. So I could file it after merging.

And I hope we could get #356 merged soon (once libmem is in) and that will bring in a bunch of spelling fixes and make it easier to avoid them in the future.

Copy link
Collaborator

@fmuyassarov fmuyassarov left a comment

Choose a reason for hiding this comment

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

LGTM

@fmuyassarov fmuyassarov merged commit 4752b6c into containers:main Oct 1, 2024
3 checks passed
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.

3 participants