[Sync] changes from ODH to rhoai- improve + cleanup code + refactor #1666
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
* Improve gc service/action * Remove useless lock * Fix typos * Improve error handling
* 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]>
…opendatahub-io#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]>
zdtsw
requested review from
lburgazzoli
and removed request for
AjayJagan and
asanzgom
…hub-io#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)
…endatahub-io#1628) (cherry picked from commit a774767)
lburgazzoli
approved these changes
Feb 14, 2025
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lburgazzoli 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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rhoai #1666 +/- ##
==========================================
+ Coverage 20.32% 20.40% +0.08%
==========================================
Files 159 159
Lines 10870 10881 +11
==========================================
+ Hits 2209 2220 +11
- Misses 8425 8437 +12
+ Partials 236 224 -12 ☔ View full report in Codecov by Sentry. |
openshift-merge-bot
bot
merged commit 2f253f2
into
opendatahub-io:rhoai
10 checks passed
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
#1656
#1621
#1563
#1628
#1646
#1580
How Has This Been Tested?
Screenshot or short clip
Merge criteria