-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Descriptions for dividing subscription should be generated after related localizations was synced. #772
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #772 +/- ##
==========================================
+ Coverage 13.60% 14.12% +0.52%
==========================================
Files 66 79 +13
Lines 7492 9070 +1578
==========================================
+ Hits 1019 1281 +262
- Misses 6391 7692 +1301
- Partials 82 97 +15 ☔ View full report in Codecov by Sentry. |
// If the sub associated with base is a dividing strategy, check if there are derived loc resources, if not, do not create desc | ||
if sub, _ := deployer.subsController.FindSubByUID(baseCopy.Labels[known.ConfigSubscriptionUIDLabel]); sub != nil { | ||
if sub.Spec.SchedulingStrategy == appsapi.DividingSchedulingStrategyType { | ||
if locs, _ := deployer.locLister.Localizations(baseCopy.Namespace).List(labels.SelectorFromSet(labels.Set{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need a way to ensure these localizations are up to date. cc @abstractmj for a look.
For sure, we can use the count len(locs)
for new base objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. Further consideration needs to be given here to adjust the number of replicas and how to handle resource updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can associate the resourceversion of loc and sub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can associate the resourceversion of loc and sub
There are some cases that those updates are triggered by manifests/feeds.
I think we should add hash labels for these generated localizations (for replica diving case). These hash labels should indicate whether manifest/feed and finv are up to date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your idea, because the current logic is that manifests will be directly synchronized to description after updating the resource size, which is not reasonable.
By the way, in the case of dividing, can we just give up using localization for replicas settings? At the same time, you can consider putting the latest feedinventory version in the sub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The essential reason is the logic of controller-manager is independent of the logic of scheduler
After offline discussions with @abstractmj, we've got some clues that may help us find a way out of this dilemma. This dilemma only exists in dividing scheduling. This is the context for the below content. The goal is to avoid updating twice, no matter it is due to the replicas or other fields changing. In Clusternet, Manifest, FeedInventory, Subscription, Localization are closely related.
Below fields could be added.
|
What type of PR is this?
bugfix
What this PR does / why we need it:
Descriptions for dividing subscription should be generated after related localizations was synced.
Which issue(s) this PR fixes:
Fixes #688
Special notes for your reviewer: