Refactor recording client management #20175
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nwnt The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 52 files with indirect coverage changes @@ Coverage Diff @@
## main #20175 +/- ##
==========================================
+ Coverage 69.27% 69.68% +0.40%
==========================================
Files 413 398 -15
Lines 34364 33941 -423
==========================================
- Hits 23805 23651 -154
+ Misses 9160 8888 -272
- Partials 1399 1402 +3 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
06331aa to
a9821d2
Compare
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
a9821d2 to
df159e9
Compare
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
df159e9 to
f0731f2
Compare
| if err != nil { | ||
| lg.Fatal("Failed empty database at start check", zap.Error(err)) | ||
| } | ||
| trafficReports := []report.ClientReport{r} | ||
| // TODO use clientSet in all operations requiring c client creation |
There was a problem hiding this comment.
I had to introduce another struct called MultClientSet on top of ClientSet. The reason for that is because there's a cross-channel dependency between the traffic simulation and watch operation where the latter needs the max revision from the traffic simulation. Hope that's ok.
Signed-off-by: Nont <nont@duck.com>
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
f0731f2 to
76c980e
Compare
|
@serathius sorry was away for a few days. How does the latest commit look? |
|
@nwnt: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| @@ -93,12 +92,11 @@ func main() { | |||
|
|
|||
| func runTraffic(ctx context.Context, lg *zap.Logger, tf traffic.Traffic, hosts []string, baseTime time.Time, duration time.Duration) ([]report.ClientReport, error) { | |||
| ids := identity.NewIDProvider() | |||
| r, err := traffic.CheckEmptyDatabaseAtStart(ctx, lg, hosts, ids, baseTime) | |||
| multiClientSet := client.NewMultiSet(ids, baseTime) | |||
There was a problem hiding this comment.
Why multi set? I don't follow the reason we need a multiple sets, I should suffice.
There was a problem hiding this comment.
During traffic simulation, the reports have to be collected early to find the max revision, which will then be fed to the channel for the watch operation. If we have only one set, collecting the report would also close the set, correct?
trafficReports = slices.Concat(trafficReports, simulateTraffic(ctx, tf, hosts, ids, baseTime, duration))
maxRevision := report.OperationsMaxRevision(trafficReports)
maxRevisionChan <- maxRevision
lg.Info("Finished simulating Traffic", zap.Int64("max-revision", maxRevision))There was a problem hiding this comment.
Ok, that makes sense. The problem is that we want to first finish traffic generation and get the report, but still wait for watch requests. Using a single ClientSet would result in traffic generation closing the set and impacting watch.
There was a problem hiding this comment.
While MultiSet tackles the generic issue, I think we should avoid over-designing here.
Could you just have one Set for traffic + CheckEmptyDatabaseAtStart and have a separate set for watch?
Fix #19893