Fix phc2sys source failover test to handle multiple log matches#231
Fix phc2sys source failover test to handle multiple log matches#231gtannous-spec wants to merge 1 commit into
Conversation
7582cff to
83a1adb
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the phc2sys log parsing logic in test/conformance/serial/ptp.go to accept multiple log matches and select the most recent one instead of the first. It also adds additional logging for the selected interface. The reviewer suggests refactoring the duplicated interface extraction logic into a helper function to improve maintainability.
| lastMatch := logMatches[len(logMatches)-1] | ||
| logrus.Infof("phc2sys log matching line: %v", lastMatch[0]) | ||
| selectedInterface = lastMatch[1] | ||
| logrus.Infof("selected interface: %s", selectedInterface) |
There was a problem hiding this comment.
The logic for extracting the interface from the phc2sys logs is duplicated in this file. Consider refactoring this into a helper function to improve maintainability and reduce code duplication.
There was a problem hiding this comment.
Optional: Will modify that after we validate all ci-tests pass.
|
/assign @nocturnalastro |
| phc2sysLogPattern, false, pkg.TimeoutIn1Minute) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(len(logMatches)).To(BeNumerically("==", 1), "Could not identify which interface phc2sys is using") | ||
| Expect(len(logMatches)).To(BeNumerically(">=", 1), "Could not identify which interface phc2sys is using") |
There was a problem hiding this comment.
Why should we see more than 1?
Last time it was more than 1 we had a bug that was duplicating lines.
The other reason would be it is switching master then switching again which would also be odd.
Do you have an example of this happening?
There was a problem hiding this comment.
This test is causing this #223 PR to fail on this test (after enabling PTP authentication), I added there a CI negative test to verify that slaves don't sync to master in case we have keys mismatch.
while we change SPP from 1 to 0 on the GM config and then from 0 to 1 each change causes the daemon to restart the ptp4l/phc2sys processes inside the same pod.
phc2sys re-selects its interface each time and logs "selecting". By the time the DualNICBCHA test ("DualNICBCHA phc2sys switches to secondary ptp4l when primary interface fails") runs, the pod has 3 accumulated entries: 1 from initial startup + 1 from spp change + 1 from spp restore.
I was not tracking the bug you talked about following the fact that "was duplicating lines", if this could lead to another issue then I can try a different approach to "get rid" of the 3 log entries.
There was a problem hiding this comment.
I fixed that PR, now all tests pass. I guess problem was replacing the pod before it was in LOCKED state.
which leads the operator to reconcile causing another process to restart so pod ends up selecting multiple entries.
Will close this one.
|
Flaky Ci-test failures were fixed for PR#223 , |
3 participants
Summary
The phc2sys source failover test assumed exactly one log match for the
source interface pattern, causing false failures when phc2sys logs
multiple source selection lines (e.g., during restarts or re-syncs).
== 1to>= 1to accept multiple matchesrecent source interface selection