Repository navigation
update: remove ondelete not used for cleanup steps - #1621
Merged
Merged
Conversation
zdtsw
requested review from
bartoszmajsak
and removed request for
asanzgom and
carlkyrillos
February 6, 2025 11:40
Member
Author
|
/test all |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1621 +/- ##
==========================================
- Coverage 20.28% 20.14% -0.14%
==========================================
Files 163 162 -1
Lines 11137 11089 -48
==========================================
- Hits 2259 2234 -25
+ Misses 8638 8621 -17
+ Partials 240 234 -6 ☔ View full report in Codecov by Sentry. |
bartoszmajsak
suggested changes
Feb 7, 2025
- we do not unpatch auth in smcp since we do not support unmanaged case Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Member
Author
|
/test opendatahub-operator-e2e |
5 tasks
bartoszmajsak
approved these changes
Feb 12, 2025
bartoszmajsak
left a comment
bartoszmajsak
left a comment
Contributor
There was a problem hiding this comment.
With no hard feelings...
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bartoszmajsak The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
zdtsw
added a commit
to zdtsw-forking/opendatahub-operator
that referenced
this pull request
Feb 14, 2025
* 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 <wenzhou@redhat.com>
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 <wenzhou@redhat.com> * 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 <ykaliuta@redhat.com> * 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 <ykaliuta@redhat.com> * 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 <ykaliuta@redhat.com> * 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 <ykaliuta@redhat.com> --------- Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> * 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 <wenzhou@redhat.com> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> Co-authored-by: Luca Burgazzoli <lburgazzoli@users.noreply.github.com> Co-authored-by: Yauheni Kaliuta <ykaliuta@redhat.com>
This file contains hidden or 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
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
How Has This Been Tested?
Screenshot or short clip
Merge criteria