Improve gc service/action #1656
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1656 +/- ##
==========================================
- Coverage 20.33% 20.32% -0.02%
==========================================
Files 163 163
Lines 11164 11195 +31
==========================================
+ Hits 2270 2275 +5
- Misses 8657 8682 +25
- Partials 237 238 +1 ☔ View full report in Codecov by Sentry. |
|
I do not have comments about the change (mean, fine for me), but since I read the code, would clarify a couple of generic questions. You wrap []Resource and protect access with mutex, but it looks that Set() called only from Start and I do not see it's supposed to be called from different threads. Did I miss any scenario? Can you explain how SelfSubjectRulesReview works since it says about |
ykaliuta
reviewed
Feb 12, 2025
lburgazzoli
force-pushed
the
gc-enhancements
branch
from
a44d477 to
bf42cf4
Compare
Something carried over from the first iteration where the deletable resources where lazily initialized, removed with |
ykaliuta
approved these changes
Feb 12, 2025
Not that you pointed me to this, I think the code does not work as expected, mh ... |
Ok, now I think I remember:
|
ykaliuta
approved these changes
Feb 12, 2025
| case err != nil: | ||
| return nil, err | ||
| default: | ||
| return items.Items, nil |
There was a problem hiding this comment.
I remember something complained that there is no return if I do it just in default. But looks like not the case anymore
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ykaliuta The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 6a7cf01
into
opendatahub-io:main
10 checks passed
ykaliuta
reviewed
Feb 13, 2025
| r.items = resources | ||
| } | ||
| func (r *Resources) Get() []Resource { | ||
| r.lock.RLock() | ||
| defer r.lock.RUnlock() | ||
|
|
||
| return slices.Clone(r.items) |
There was a problem hiding this comment.
May be it's redundant if it's set once and then just read?
zdtsw
pushed a commit
to zdtsw-forking/opendatahub-operator
that referenced
this pull request
Feb 14, 2025
* Improve gc service/action * Remove useless lock * Fix typos * Improve error handling
5 tasks
openshift-merge-bot bot
pushed a commit
that referenced
this pull request
Feb 14, 2025
…1666) * Improve gc service/action (#1656) * Improve gc service/action * Remove useless lock * Fix typos * Improve error handling * update: remove ondelete not used for cleanup steps (#1621) * update: remove ondelete not used for cleanup steps - we do not unpatch auth in smcp since we do not support unmanaged case --------- Signed-off-by: Wen Zhou <[email protected]> * actions: refactor to move template and manifest rendering to a helper (#1563) * actions: abstract caching functionality into cacher The caching code for all caching Actions is pretty much the same 1) hash the key 2) if the hash is the same as the stored one, return stored resource 3) otherwise render the resource 4) store the key and the resource 5) return the resource Abstract this logic into Cacher[T] generic type, where T is the type of the resource, which is supposed to be included as a composition in on the higher levels. The core engine is implemented as Render method which accepts actual renderer (function) and return boolean value in addition to the resources indicating if it performed rendering. It supposed to be used in resource rendering actions for accounting. The renderer should return (error, T). Put renderer as the last argument to make more convenient to call with a closure. Copy CachingKeyFn type. In future it can be extented to accept context for possible debug logging. Define contract, CachingKeyFn should not return empty hash. (!!!) The code originally is taken pretty much from the existing kustomize rendering Action. But taking into account the contract, refactor the code to avoid comparation of the result (and nil assignments) since with generics to work with both nilable (maps, arrays, etc) and non-nilable (ints, strings) types it requires some extra handling of Zero values. Zero values are still required for return values in error case, so implement Zero[T] generic function. With the assumption make "empty key" a error condition, it removes need of extra comparation. Move actual rendering and cache update into separate function, it makes main Render function more clear without extra indentation. Possible usage supposed to be ``` type Action struct { cacher.Cacher[resources.UnstructuredList] // action data } type ActionOpts func(*Action) func WithCache() ActionOpts { return func(a *Action) { a.Cacher.SetKeyFn(types.Hash) // or own } } func (s *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error { //... res, acted, err := a.Cacher.Render(ctx, rr, s.render) if acted { /* may be do something */ } // use resources } func (s *Action) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { result := make(resources.UnstructuredList) // make some resources return result, nil } func NewAction(opts ...ActionOpts) actions.Fn { for _, opt := range opts { opt(&action) } return action.run } ``` with reconciler creation ``` func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { _, err := reconciler.ReconcilerFor( mgr, &componentApi.Type{}, ). // ... WithAction(foobar.NewAction( foobar.WithCache(), )) } ``` Pay attention to signature of render() and call to Cacher.Render() from run(). Signed-off-by: Yauheni Kaliuta <[email protected]> * actions: abstract resource caching into resourcecacher Make an abstract wrapper on the raw cacher.Cacher[T] to work on actions whose Renderer produces resources (resources.UnstructuredList) to be recorded into ReconciliationRequest.Manifests and later deployed (deleted) to the cluster. Performs common tasks: - invalidate caching if devflags enabled; - account resources if they were generated (and skip cached ones); - add resources to the ReconciliationRequest. Created constructor which accepts mandatory name. Since it is used in prometheus, it's recommended to make it lower case. At this moment this is the only part which interested in the origin of resources (cached or not) and it's supposed to be used in actions whose final goal is to add resources to the request (which is done by this helper), so change signature of its Render() method (comparing to cacher.Cacher) to return only error. The code is basically copied from manifest/template rendering actions run() method. Usage (changed run() comparing to cacher.Cacher): ``` type Action struct { resourcecacher.ResourceCacher // action data } type ActionOpts func(*Action) func WithCache() ActionOpts { return func(a *Action) { a.ResourceCacher.SetKeyFn(types.Hash) // or own } } func (s *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error { //... return a.ResourceCacher.Render(ctx, rr, s.render) } func (s *Action) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { result := make(resources.UnstructuredList) // make some resources return result, nil } func NewAction(opts ...ActionOpts) actions.Fn { action := Action{ ResourceCacher: resourcecacher.NewResourceCacher("mycoolaction"), } for _, opt := range opts { opt(&action) } return action.run } ``` Signed-off-by: Yauheni Kaliuta <[email protected]> * actions: use resourcecacher for manifests rendering All the run() functionality now implemented by the ResourceCacher, so just call its Render() in run(). Amend WithCache() as required by ResourceCacher to set its CachingKeyFn and remove CachingKeyFn initialization from NewAction. Change render() signature to return resources.UnstructuredList RendererEngine is used as a field initializer now, so no need to export, rename to rendererEngine. Signed-off-by: Yauheni Kaliuta <[email protected]> * actions: use resourcecacher for template rendering The same changes as to manifests rendering. Just keep `data` field initialization in NewAction, used in render(). Signed-off-by: Yauheni Kaliuta <[email protected]> --------- Signed-off-by: Yauheni Kaliuta <[email protected]> * chore: move Release strcuts from pkg/cluster to apis/common (#1580) Structs that are part of the public APIs should be in the apis packages for consistency and to avoid cyclic import issues (cherry picked from commit 1760dee) * feat(test): support for Succeed and MatchError in testf framework (#1628) (cherry picked from commit a774767) * chore(kserve): wrap error returned by FromUnstructured (#1646) (cherry picked from commit 9402f6a) --------- Signed-off-by: Wen Zhou <[email protected]> Signed-off-by: Yauheni Kaliuta <[email protected]> Co-authored-by: Luca Burgazzoli <[email protected]> Co-authored-by: Yauheni Kaliuta <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Extracted from #1605
How Has This Been Tested?
Screenshot or short clip
Merge criteria
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).