aggregate desc status to subscription and workload #360
Conversation
aven-ai
force-pushed
the
pr_AggregateStatus_hub
branch
from
b909ecf
to
4b60b88
Compare
Codecov Report
@@ Coverage Diff @@
## main #360 +/- ##
==========================================
+ Coverage 13.49% 13.62% +0.12%
==========================================
Files 60 60
Lines 6563 6568 +5
==========================================
+ Hits 886 895 +9
+ Misses 5608 5605 -3
+ Partials 69 68 -1
Continue to review full report at Codecov.
|
| @@ -31,6 +31,8 @@ func trimCommonMetadata(result *unstructured.Unstructured) { | |||
| unstructured.RemoveNestedField(result.Object, "metadata", "managedFields") | |||
| unstructured.RemoveNestedField(result.Object, "metadata", "resourceVersion") | |||
| unstructured.RemoveNestedField(result.Object, "metadata", "selfLink") | |||
| unstructured.RemoveNestedField(result.Object, "metadata", "generation") | |||
There was a problem hiding this comment.
I don't think we should modify this file.
There was a problem hiding this comment.
It is to remove the generation and observedGeneration fields in the object. Keeping them will cause the manifest to change, and re-ApplyDescription.
Like: manifest change -> description.spec.raw -> ApplyDescription to child -> workload status.observedGeneration -> description.status.observedGeneration -> workload.status changed -> manifest change.
Do you have any other suggestions to this problem?
| syncHandlerFunc SyncHandlerFunc | ||
| } | ||
|
|
||
| func NewController(clusternetClient clusternetClientSet.Interface, |
There was a problem hiding this comment.
We're aggregating status from Description to Subscription, then why not adding this Subscription controller?
There was a problem hiding this comment.
We're aggregating status from
DescriptiontoSubscription, then why not adding thisSubscriptioncontroller?
The logic of subscription-controller is to watch the events of addSubscription, updateSubscription, deleteSubscription, and deleteBase to re-ApplyDescription.
The logic of aggregatestatus-controller is to watch Description.Status update, deleteDescription, and synchronize the status of Description to subs and workload.
I don't think it's a better solution to combine them together, leading to logical intersections and more logical conditional judgments, especially considering more exception scenarios.
aven-ai
force-pushed
the
pr_AggregateStatus_hub
branch
from
4b60b88
to
6a8b283
Compare
pkg/hub/deployer/deployer.go
Outdated
| @@ -308,6 +334,198 @@ func (deployer *Deployer) handleSubscription(sub *appsapi.Subscription) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (deployer *Deployer) handleAggregateStatus(sub *appsapi.Subscription) error { | |||
There was a problem hiding this comment.
I think we could move this to aggregatestatus.go, since we can get all the status from the listers.
The aggregatestatus controller can be invoked in deployer.
There was a problem hiding this comment.
I think we could move this to
aggregatestatus.go, since we can get all the status from the listers.The aggregatestatus controller can be invoked in deployer.
Yes, it can be done in code implementation. In fact, putting handleAggregateStatus in aggregatestatus.go or deployer.go has the same effect. Considering that there will be more iterations and more code in the subsequent status aggregation, we can try to put it in aggregatestatus.go.
aven-ai
force-pushed
the
pr_AggregateStatus_hub
branch
from
6a8b283
to
907e23b
Compare
Signed-off-by: aven-ai <as1919@qq.com>
aven-ai
force-pushed
the
pr_AggregateStatus_hub
branch
from
907e23b
to
b9f502f
Compare
What type of PR is this?
Kind/feature
What this PR does / why we need it:
aggregate desc status to subscription and workload
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer: