Skip to content

balloons/agent: fix/rewrite/simplify extended resource handling#714

Open
askervin wants to merge 3 commits into
containers:mainfrom
askervin:5i1-balloons-ext-cleanup
Open

balloons/agent: fix/rewrite/simplify extended resource handling#714
askervin wants to merge 3 commits into
containers:mainfrom
askervin:5i1-balloons-ext-cleanup

Conversation

@askervin

@askervin askervin commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

This change abandons earlier idea of always removing published extended resources on container exit and reconfiguration. Exit on "helm uninstall" is especially challenging because helm may remove serviceaccount and permissions before the policy can patch the node. On the other hand, successfully clean-up on exit is not critical, and clean-up on reconfiguration is completely unnecessary when adopting reconciliation approach implemented here: update resources if necessary.

This change extends GetExtendedResources() in policy interface to communicate extended resource domains/patterns owned by the policy. This enables the agent to remove extended resources owned by a policy, even if exact resource names depended on previous (already lost) policy configurations.

Adding/updating/removing extended resources is switched to MergePatchType to make it more robust against failures that could be caused by delayed earlier resource removals, for instance.

@askervin askervin requested review from kad and marquiz July 8, 2026 14:04
@askervin askervin force-pushed the 5i1-balloons-ext-cleanup branch 2 times, most recently from cae4bdb to 5ef2b6f Compare July 9, 2026 05:59

@marquiz marquiz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @askervin for the PR. On a quick look a definite improvement over the previous functionality.

I started to think should/could we come up with stricter and even better-defined scheme around policy-specific ERs.

  1. Policies own all and only *.<policy-name>.nri.io/* ERs. This could be handled/enforced on the generic policy infra. At startup all of these would be cleaned-up (removed).
  2. Agent shutdown could be simplified. Don't try to cleanup on exit. Instead, use Helm hooks for cleanup (e.g. run agent in special cleanup mode, haven't thought what options we would have here).
  3. The reconcile could be a loop (re-try a few times)

Just a quick idea, I haven't thought through the details and implications/problems.

Actually, now that I think of it, the agent cleaning up the ERs on exit is probably a bad idea. E.g. on updates or some other restarts it would transitionally/temporarily remove ERs which might cause unexpected/unwanted ripple effects in scheduling etc.

Thoughts?

@askervin askervin force-pushed the 5i1-balloons-ext-cleanup branch 2 times, most recently from 1f2e267 to 1222353 Compare July 10, 2026 11:30
@askervin

Copy link
Copy Markdown
Collaborator Author

Thanks @marquiz!

  • Agent shutdown simplified: no more extended resources clean-up trial with unpredictable results.
  • Documentation updated accordingly. A note on this added to publishExtendedResources option.
  • Proper reconciliation loop added: extended resources are managed even if running containers would not change.

Helm hooks for uninstall clean-up, including more general node state reset (frequencies, governors, CPU affinities, SST CLOSes....), is left for next phase.

My first thoughts regarding framework-managed extended resource names (..nri.io/) are mixed. I cannot see immediate issues, but I can imagine nasty conflicts (NRI plugins outside nri-plugins project) and limitations in exotic trials (multipolicy setups) that unconfigurable resource names might cause. But these are all more or less imaginary issues at the moment.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR reworks node extended-resource handling in the resource-policy agent to use reconciliation instead of “publish/clear on exit,” enabling robust cleanup of stale/orphaned extended resources across reconfigurations and restarts without relying on shutdown-time permissions.

Changes:

  • Extend the policy interface for extended resources to express both “published” resources and “owned-but-not-published” resources via nil values and wildcard ownership patterns.
  • Replace ad-hoc JSONPatch + local tracking with a single-goroutine reconciliation loop that GETs current node capacity and applies MergePatch updates/removals.
  • Update balloons policy behavior, docs, and e2e coverage to validate reconciliation (including in-process helm upgrade and orphan injection).

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/e2e/policies.test-suite/balloons/n4c16/test19-pct/code.var.sh Adds Phase 1.5 e2e coverage for extended-resource reconciliation and introduces helpers for polling node capacity.
test/e2e/policies.test-suite/balloons/n4c16/test19-pct/balloons-pct-nopublish.cfg Adds a non-publishing config used to validate reconciliation cleanup behavior.
pkg/resmgr/resource-manager.go Updates commentary to reflect reconciliation semantics for node extended resources.
pkg/resmgr/policy/policy.go Changes the policy interface contract for extended resources to include ownership patterns and nil-valued keys.
pkg/agent/node-extended-resources.go Replaces last-writer JSONPatch publishing/clearing with a reconciler loop using MergePatch and ownership patterns.
pkg/agent/agent.go Adds agent state and goroutine startup for the extended-resource reconciler; removes shutdown/config-loss cleanup calls.
docs/resource-policy/policy/howto/balloons-pct-example-manual.md Updates guidance: extended resources aren’t cleared on uninstall; reconciliation removes stale resources when configured non-publishing.
docs/resource-policy/policy/howto/balloons-pct-example-auto.md Same documentation update for the automated example.
docs/resource-policy/policy/balloons.md Notes extended resources aren’t cleaned on stop/uninstall; reconciliation removes them when not published.
cmd/plugins/topology-aware/policy/topology-aware-policy.go Adapts to the new GetExtendedResources return type.
cmd/plugins/template/policy/template-policy.go Adapts to the new GetExtendedResources return type.
cmd/plugins/balloons/policy/balloons-policy.go Implements domain ownership via glob pattern and publishes per-class PCT capacity using pointer quantities.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/agent/node-extended-resources.go Outdated
Comment thread pkg/agent/node-extended-resources.go
Comment thread pkg/agent/node-extended-resources.go Outdated
@askervin askervin force-pushed the 5i1-balloons-ext-cleanup branch 2 times, most recently from 3b0ef24 to 6b834ad Compare July 10, 2026 15:20
askervin added 3 commits July 13, 2026 09:18
This change abandons earlier idea of always removing published
extended resources on container exit and reconfiguration. Exit on
"helm uninstall" is especially challenging because helm may remove
serviceaccount and permissions before the policy can patch the
node. On the other hand, successful clean-up on exit is not
critical, and clean-up on reconfiguration is completely unnecessary
when adopting reconciliation approach implemented here: update
resources if necessary.

This change extends GetExtendedResources() in policy interface to
communicate extended resource domains/patterns owned by the
policy. This enables the agent to remove extended resources owned by a
policy, even if exact resource names depended on previous (already
lost) policy configurations.

Adding/updating/removing extended resources is switched to idempotent
MergePatchType to make it more robust against failures that could be
caused by delayed earlier resource removals, for instance. Updates are
done in an independent reconciliation loop that takes care handling
API failures and external drift between desired state and node status.

Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
Deleting all effective BalloonsPolicies is not expected to clean up
extended resources. Clean-up takes place in reconciliation and on
exit. Without a config there is nothing to reconcile (no expected
state), and the policy is not about to exit. It is merely waiting for
a new configuration.

Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
@askervin askervin force-pushed the 5i1-balloons-ext-cleanup branch from 6b834ad to 9a14813 Compare July 13, 2026 07:02
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