Feat: ResourceTracker new architecture #2849
Conversation
Somefive
requested review from
FogDong,
hongchaodeng,
leejanee,
wangyikewxgm and
wonderflow
as code owners
Somefive
force-pushed
the
new_rt
branch
2 times, most recently
from
3672d07
to
2ce4320
Compare
Codecov Report
@@ Coverage Diff @@
## master #2849 +/- ##
==========================================
+ Coverage 56.83% 59.85% +3.01%
==========================================
Files 229 238 +9
Lines 23739 24204 +465
==========================================
+ Hits 13492 14487 +995
+ Misses 8546 7979 -567
- Partials 1701 1738 +37
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Somefive
force-pushed
the
new_rt
branch
21 times, most recently
from
88766e3
to
5cfaf6d
Compare
Signed-off-by: Yin Da <yd219913@alibaba-inc.com>
| // DeletedManifestInResourceTracker marks resources as deleted in resourcetracker, if remove is true, resources will be removed from resourcetracker | ||
| func DeletedManifestInResourceTracker(ctx context.Context, cli client.Client, rt *v1beta1.ResourceTracker, manifest *unstructured.Unstructured, remove bool) error { | ||
| rt.DeleteManagedResource(manifest, remove) | ||
| return cli.Update(ctx, rt) |
There was a problem hiding this comment.
where do you delete the ownerRef in resourceTracker for compatibility？
There was a problem hiding this comment.
The compatibility code has not been added yet. Will be added together with upgrade tests.
| ) | ||
|
|
||
| // StateKeep run this function to keep resources up-to-date | ||
| func (h *resourceKeeper) StateKeep(ctx context.Context) error { |
There was a problem hiding this comment.
you should write a user facing doc to explain this policy and recommand the best practice.
| GenericFunc: func(genericEvent ctrlEvent.GenericEvent, limitingInterface workqueue.RateLimitingInterface) { | ||
| handleResourceTracker(genericEvent.Object, limitingInterface) | ||
| }, | ||
| }). |
There was a problem hiding this comment.
what are these handler for?
There was a problem hiding this comment.
These handlers are added for application controller to response ResourceTracker change events. Designed by @leejanee .
There was a problem hiding this comment.
What's the real affect? Who will change RT besides app controller?
There was a problem hiding this comment.
In the mode of KeepLegacyResource, history versioned ResourceTrackers will not be automatically deleted by application controller. Instead, users have the control to select which history version to keep or discard. In this case, user will delete RT.
There was a problem hiding this comment.
I got it. That means app controller is also the controller of RT
| if err := handler.resourceKeeper.StateKeep(ctx); err != nil { | ||
| logCtx.Error(err, "Failed to run prevent-configuration-drift") | ||
| r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedStateKeep, err)) | ||
| return r.endWithNegativeCondition(logCtx, app, condition.ReconcileError(err), phase) |
There was a problem hiding this comment.
What will happend In this case? I think the application is already running well here,and the health check should report error first if not?
So , does it necessary to report an error? But a event even change the condition is necessary
There was a problem hiding this comment.
If StateKeep failed (failed to get/update target resource and is not caused by not found or conflict, such as cluster disconnect), it will trigger the next reconcile of StateKeep due to the returned error.
There was a problem hiding this comment.
I think at least a warning should be reported for users, since a failure is encountered during ensuring application managed resources and bringing state back.
There was a problem hiding this comment.
Yes， it deserves a warning. But return error just cause a new reconcile which doesn't help in this case.
There was a problem hiding this comment.
In some cases, it is worth a retry. For example, if the child cluster apiserver is busy temporarily, then the resource check might timeout which will cause a StateKeep error here. However, the child cluster apiserver might recover by itself later, then a retry will help controller do the StateKeep. Generally, it would be better to have more details for the returned error and handle it more elegantly. I agree that currently we do not necessarily need an error to be returned here. Will fix it.
Signed-off-by: Yin Da <yd219913@alibaba-inc.com>
| @@ -476,6 +481,48 @@ const ( | |||
| WorkflowResourceCreator ResourceCreatorRole = "workflow" | |||
| ) | |||
|
|
|||
| // OAMObjectReference defines the object reference for an oam resource | |||
| type OAMObjectReference struct { | |||
| Component string `json:"component,omitempty"` | |||
There was a problem hiding this comment.
Is it based on one application--one component--one trait module?
There was a problem hiding this comment.
Not exactly. This reference is for one target resource, for example, a deployment or a service. Each resource is always belonging to 1 app - 1 comp (- 1 trait, optional).
|
|
||
| // Equal check if two references are equal | ||
| func (in OAMObjectReference) Equal(r OAMObjectReference) bool { | ||
| return in.Component == r.Component && in.Trait == r.Trait && in.Env == r.Env |
There was a problem hiding this comment.
Can reflect.DeepEqual do the comparison?
There was a problem hiding this comment.
Yes. It can. It is also possible to do the equality check with reflect.DeepEqual.
|
I noticed there are some changes on |
Yes, there will be compatibility problems. ResourceTracker will face an upgrade concerning existing KubeVela system. Some compatibility code and tests will be added. |
This PR is proposed to rework ResourceTracker and upgrade the current resource management architecture for KubeVela controller.
Major Changes
a. The former one allows difference resources have various life-cycle. Usually, resources are deleted as soon as the latest application does not use them anymore. Some resources need to be kept until the application is gone. Some might need be kept even if the application is gone.
b. The latter one allows us to prevent configuration-drift by running StateKeep after workflow suspended/terminated/succeed.
Minor Changes
Deprecation
Implementation
pkg/resourcekeeper/where resource recording, garbage collecting and state keeping are supported. It is recommended for code reviewers to review from here, since other changes are all about making this package work properly.I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.