lf_interop_ping_plotter.py: fix crashes, retry transient API failures#297
Merged
Merged
Conversation
Adds log_endpoint_status_change(), called from get_results() for every endpoint on each poll, which records a timestamp/resource_id/ endpoint_name/status row to endpoint_status_log.csv whenever an endpoint's status actually changes (including first-seen), rather than dumping full state every poll. Resource id is resolved from self.sta_list, branching on real vs. virtual naming since GenCXProfile.create() names them differently (full eid for real clients, port segment only for virtual). The CSV is created at startup (works for both CLI and WebUI runs) and moved into the report directory once generate_report()/generate_report_robo() establish it. Also dedupes the "missing endpoints" warning added earlier: each endpoint is now only logged once when it first goes missing, tracked via self.reported_missing_endpoints, instead of repeating the same warning on every poll for the rest of the test. get_results() now returns immediately when the manager gives no response, instead of crashing on results.keys(). Both polling loops (monitor() and the equivalent inline loop in main()) handle that None return by pacing with the loop's normal time.sleep(1) and loop_timer advance before continuing, so a transient API outage skips a poll instead of crashing the test or busy-looping. Signed-off-by: Narayana-CT <narayana.pinapatruni@candelatech.com>
…salignment Adds build_device_summary(), a shared helper that derives the per-device summary fields (packets sent/received/dropped, mac/ip/ bssid/ssid, channel, mode, packet loss %, min/avg/max RTT, report name) used by every table and graph in the report. The inline loops in generate_report() and generate_report_robo() previously accessed device_data fields directly with no fallback, so a single device with an empty ping_stats, a missing key, or an rtts dict made up entirely of the 0.11 "no data" placeholder would raise (including a latent ZeroDivisionError on sum()/len() of an emptied rtts list) and abort report generation for every device, not just the bad one. build_device_summary() now falls back to safe defaults (0 / 'NA' / 'Unknown') per field instead, keeping every device's contribution positionally aligned with self.result_json's iteration order, which generate_uptime_graph() depends on for per-device color mapping. Also drops a leftover debug print() from the generate_report_robo() copy of this loop. Fixes get_pass_fail_list(): GET /adb/ returning None was logged but then dereferenced anyway (adb_response["devices"]), crashing the whole report; it's now checked before use, matching the equivalent fix already applied elsewhere. Separately, and more importantly, res_list only appended an entry for an Android client when the /adb/ lookup found a match, so any unresolved Android device silently shifted every later entry in res_list/test_input_list one position out of alignment with self.report_names/self.packets_sent - producing either a pandas length-mismatch crash when the mismatched test_input_list was assigned onto individual_report_df, or worse, a PASS/FAIL verdict silently attributed to the wrong device. res_list now always appends exactly one entry per client (falling back to the client's own name when unresolved), and pass_fail_list is indexed by range(len(self.report_names)) instead of range(len(test_input_list)). Also hoists the /adb/ call out of the loop instead of re-fetching it once per Android client. Fixes generate_dataframe(): GET /adb/ returning None was logged but then dereferenced anyway, same crash as above - now returns None early. Also fixes an 'android' vs 'Android' case mismatch in the group-membership check: every place in this file that actually sets a device's os field uses the capitalized string "Android", so the lowercase comparison here could never match, meaning Android devices always fell through to the slower per-username /adb/ lookup branch instead of the direct name-match branch. Signed-off-by: Narayana-CT <narayana.pinapatruni@candelatech.com>
…loss When every generic endpoint disappears from a poll response (all 'UNKNOWN' or no valid entries), both polling loops (monitor() and the equivalent inline loop in main()) previously called exit(0) immediately - killing the process before report generation ever ran, even if the loss was a transient LANforge hiccup and even though minutes or hours of prior polling data was already sitting in result_json/coordinate_json. Adds has_valid_endpoint_data(), extracted from the duplicated isinstance/UNKNOWN-filtering check in both loops, and wait_for_valid_endpoints(timeout=40), which retries get_results() every 2s for up to 40s before giving up. If valid endpoint data reappears during that window, the loop resumes normal polling; if the timeout elapses, the loop now breaks instead of exiting, falling through to the existing stop_generic() / report-generation / cleanup sequence in both call sites. Since report generation can now be reached with partial or even zero collected data (a device that never once returned data still never gets a result_json entry), generate_report() and generate_report_robo() each gained a guard at the top that logs and returns early if self.result_json / self.coordinate_json is empty, rather than building an empty or broken report. Signed-off-by: Narayana-CT <narayana.pinapatruni@candelatech.com>
Three call sites fetched GET /ports/all/ and dereferenced the response (ports_response['interfaces']) without checking for None first - a transient failure would raise TypeError. Two of the three built a ports_data dict that was never actually read afterward: the per-coordinate fetch in perform_robo(), and the one-time initial fetch in main() before the polling loop (its result was always superseded by the in-loop rebuild under `if args.virtual:` before ever being read, even on the very first iteration). Both were dead code as well as a crash risk, so removed outright rather than just guarded. The third site - the in-loop rebuild under `if args.virtual:` - is the one that matters: ports_data[station] is read a few lines later in the same iteration, so a transient failure there would abort the whole test run. Adds wait_for_ports_response(timeout=40), mirroring wait_for_valid_endpoints() added for the endpoint-loss case: retries GET /ports/all/ every 2s for up to 40s, and if it still hasn't recovered, stops the test with exit(1) rather than crashing on the None dereference. Signed-off-by: Narayana-CT <narayana.pinapatruni@candelatech.com>
…ponse Adds wait_for_adb_response(timeout=40), mirroring wait_for_valid_endpoints()/wait_for_ports_response(): retries GET /adb/ every 2s for up to 40s when it returns no response, in case the failure is transient, before falling back to whatever the caller already did for a missing response. Unlike get_results()/ports/all/, /adb/ is only used as enrichment - matching Android clients to CSV pass/fail thresholds (get_pass_fail_list()) or filtering devices into group tables (generate_dataframe()) - so neither caller's fallback changes: an unresolved Android client in get_pass_fail_list() still falls back to its own name (already fixed in the res_list alignment work), and generate_dataframe() still returns None for that one group's table on a caller loop that already treats None as "skip this group, keep going" - confirmed by tracing group_device_map.items() in both generate_report() and generate_report_robo(), where the result is only consumed inside `if dataframe:`. So this only adds a wait before either fallback kicks in, without changing what happens after. Verified CLI : python3 lf_interop_ping_plotter.py --mgr 192.168.207.75 --real --ping_interval 1 --ping_duration 1m --target www.google.com --use_default_config Signed-off-by: Narayana-CT <narayana.pinapatruni@candelatech.com>
haricharan-candela
requested review from
a-gavin and
haricharan-candela
and removed request for
a-gavin and
haricharan-candela
July 17, 2026 07:15
goyalsaurabh06
approved these changes
Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug-fix batch for
lf_interop_ping_plotter.pyaddressing several crashespaths and one data-correctness bug found during a review of the report
generation and the LANforge API polling loops.
Report-generation crashes on bad device data. The per-device
summary loop in
generate_report()/generate_report_robo()accessedfields directly with no fallback, so a single device with empty
ping_stats, a missing key, or anrttsdict made up entirely of the0.11"no data" placeholder (a latentZeroDivisionError) would abortreport generation for every device. Replaced with a shared
build_device_summary()helper that falls back to safe defaults(
0/'NA'/'Unknown') per field.NoneAPI responses logged but dereferenced anyway.GET /adb/and
GET /ports/all/responses were checked forNoneand logged,then immediately dereferenced regardless, raising
TypeErroron atransient failure — in
get_pass_fail_list(),generate_dataframe(),and three
/ports/all/call sites (two of which built a value thatwas never actually read afterward, so removed outright; the third,
inside the main per-poll loop, now retries).
Retry-before-giving-up instead of instant
exit(0). TotalGeneric-endpoint loss previously killed the process immediately, even
on a transient LANforge hiccup and even with hours of prior polling
data already collected.
has_valid_endpoint_data()+wait_for_valid_endpoints(timeout=40)now retry for up to 40s beforeThe loop
breaks (instead of exiting) into the existingstop/report/cleanup sequence. The same retry-then-fallback pattern was
added for
/ports/all/(wait_for_ports_response(), falls back toexit(1)if still unresolved) and/adb/.(
wait_for_adb_response(), falls back to each caller's existinggraceful degradation — unresolved Android clients fall back to their
own name; a group table is skipped for that one group).
generate_report()/generate_report_robo()now guard againstresult_json/coordinate_jsonbeing empty instead of building aBroken report of zero data.
Data-misalignment bug in
get_pass_fail_list(). An Android clientThat didn't resolve via
/adb/left a gap inres_list, silentlyshifting every later PASS/FAIL verdict onto the wrong device (or
raising a pandas length-mismatch error). Fixed so every client always
contributes exactly one aligned entry.
Case-mismatch bug in
generate_dataframe(). Group-membershipmatching compared against lowercase
'android', but every place thatactually sets a device's OS uses
'Android'— so the condition couldnever match, and Android devices always fall through to the slower
Per-username
/adb/lookup instead of the direct name-match branch.Endpoint status logging. Adds
endpoint_status_log.csv, recordinga row per endpoint whenever its status actually changes (not every
poll), and dedupes the "missing endpoints" warning to fire once per
endpoint instead of repeating every poll for the rest of the test.