stantheman0128 commented on PR #1131: URL: https://github.com/apache/yunikorn-core/pull/1131#issuecomment-5840676116
I read the fuzzer against master. A few comments, mostly about replaying a failure and about what CI actually shows. **1. Replaying a failure** (`partition_property_test.go` L51-55, L77, L281-282, L485, L723-724) The header comment at L51-55 says the scheduler can break ties differently between runs, so the state a seed reaches can differ. I think the operation sequence can differ too, because the number of values an op draws from `f.rng` depends on partition state. `release()` returns at L443-445 without drawing when `f.allocated` is empty, and `f.allocated` is filled from whatever the scheduling cycle returned (L433-436). Once one cycle places a different allocation, the rest of the rng stream can shift. One tie source I found is `filterOnPendingResources` in `objects/sorters.go`, which builds the app slice by ranging over a map before the stable sort. So the comments at L77, L281-282, L485 and L723-724, which promise a stable or identical replay, claim more than the test can deliver. Two suggestions: - Print the full history on failure instead of the last 25 ops (L61, L497-500). It is only printed when the test fails, it is at most 800 lines, and it is the only reliable way to rebuild the sequence. - Reword L51-52 so it no longer says the seed fixes the generated operations. **2. Coverage counters are not visible in CI** (L480-482) The per-seed coverage is written with `t.Logf`, and Go only prints that for a passing test when `-v` is set. `make test` runs `go test ./... $(RACE) -tags deadlock ...` without `-v` (Makefile L208), so if a later change stops the fuzzer from reaching reservations, nobody will notice. Could `run` return its stats so `TestPartitionProperty` asserts a floor after the loop? Summing over all seeds and checking something like `node removed with allocations` > 0 and `tryReservedAllocate placed an allocation` > 0 should not flake from tie breaking. **3. YUNIKORN-3379 and YUNIKORN-3380 are resolved now** This follows up on Manikandan's comment on the JIRA. The 3380 trigger is a release with an empty allocation key, which takes the release-all branch in `processAllocationRelease`. `release()` always sets `AllocationKey` (L460), so that branch is never reached. A release-all op would need to drop every model entry of that app from `f.allocated`. It also needs a termination type other than `SCHEDULING_FAILED_ON_RM`, because `removeAllocation` sends that type to `rollbackAllocation` before the empty key check is reached. For 3379, `PLACEHOLDER_REPLACED` is excluded from `propTerminationTypes` (L119-128). With the fix in, what should the model expect when that type arrives for a non-placeholder allocation? The branch sits on 41ae1cc (Aug 28), so it needs a rebase to pick up either fix. **4. Runtime with the CI flags** (question) The description says about 4s. Was that measured with `-race -tags deadlock` and `DEADLOCK_DETECTION_ENABLED=true`, the way `make test` runs it (Makefile L201-208)? The run is five seeds of 800 steps, with the invariants checked again after every operation that runs, and that could be a lot slower once the race detector and the deadlock wrapper are on. **5. Nit** (L649) `if reserved := app.NodeReservedForAsk(key)` shadows the `reserved` counter from L620 with a string. It works, but a different name such as `reservedOn` would be easier to read. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
