Skip to content

Commit e3b5100

Browse files
Fei Chenfacebook-github-bot
authored andcommitted
Add separate counter for server ID 0 in katran TPR
Summary: This diff introduces a new counter `error_server_id_zero` to separately track server ID 0 cases in katran's TPR (TCP Packet Router) module, distinguishing them from other types of bad server IDs. ## Background Currently, server ID 0 is treated as a "bad ID" and counted under `error_bad_id`. However, server ID 0 represents a special case that requires different debugging and handling compared to actual corrupted/invalid server IDs. ## Changes Made 1. **New Counter Added**: `error_server_id_zero` added to: - `tcp_router_stats` struct in `TPRTypes.h` - BPF `stats` struct in `tcp_pkt_router_structs.h` - Output formatting in `operator<<` 2. **Stats Collection Updated**: - `TPRStatsPoller.cpp` - Generic stats collection - `MetaTcpPktRouter.cpp` - Meta-specific stats with timeseries - `tpr_app.cpp` - App-level diff calculation 3. **BPF Logic Updated**: Replaced `stat->error_bad_id++` with `stat->error_server_id_zero++` for **server ID 0 specific cases only**: - **Active handler**: - Parse header option where server_id == 0 (line 50) - Storage failure sending 0 to passive (line 105) - No server_id received in established connection (line 142) - **Passive handler**: - Received server_id == 0 from peer (line 58) 4. **Preserved existing `error_bad_id` behavior**: Still tracks legitimate bad ID cases including: - ID mismatches between header and storage (non-zero values) - Wrong server_id during packet routing (misrouted packets) - Storage inconsistencies with non-zero IDs 5. **Test Updates**: Updated `TcpPktRouterIntegrationTest.cpp` to reflect new counter behavior: - Rogue sock_ops scenarios now expect `error_server_id_zero > 0` - Normal scenarios expect both counters to be 0 - All 27 tests now pass ## Key Distinction - **`error_server_id_zero`**: Counts cases where server_id is actually 0 (special case for debugging) - **`error_bad_id`**: Counts cases where server_id is corrupted, mismatched, or invalid (actual errors) ## Benefits - **Better observability**: Distinguish expected server ID 0 cases from unexpected bad IDs - **Improved debugging**: Separate counters for different error conditions - **Backward compatibility**: Existing `error_bad_id` counter remains functional ## Testing - Built successfully: `fbcode//katran/tpr:proxy_tpr_lib` and `fbcode//katran/facebook/cpr/tpr:meta_katran_tpr_lib` - **All 27 integration tests pass**: `fbcode//katran/facebook/cpr/tpr/test:tcp_pkt_router_test` - All code properly formatted and linted - Both counters will be exported to monitoring systems for comprehensive tracking --- > Generated by [Confucius Code Assist (CCA)](https://www.internalfb.com/wiki/Confucius/Analect/Shared_Analects/Confucius_Code_Assist_(CCA)/) [Session](https://www.internalfb.com/confucius?session_id=4cd8f5ba-81f2-11f0-b5ce-fd1edeb5e779&tab=Chat), [Trace](https://www.internalfb.com/confucius?session_id=4cd8f5ba-81f2-11f0-b5ce-fd1edeb5e779&tab=Trace) Reviewed By: pdubovitsky Differential Revision: D80974570 fbshipit-source-id: 86e733d386c15f1fa461a5331c2f5f6eba0e7377
1 parent 1e4cd50 commit e3b5100

5 files changed

Lines changed: 9 additions & 4 deletions

File tree

katran/tpr/TPRStatsPoller.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ void TPRStatsPoller::setStatsCounters(const tcp_router_stats& stats) {
3636
setCounter("conns_skipped", stats.conns_skipped);
3737
setCounter("no_tcp_opt_hdr", stats.no_tcp_opt_hdr);
3838
setCounter("error_bad_id", stats.error_bad_id);
39+
setCounter("error_server_id_zero", stats.error_server_id_zero);
3940
setCounter("error_write_opt", stats.error_write_opt);
4041
setCounter("error_sys_calls", stats.error_sys_calls);
4142
setCounter("ignoring_due_to_kde", stats.ignoring_due_to_kde);
@@ -154,6 +155,7 @@ TPRStatsPoller::collectTPRStats(int numCpus) {
154155
aggregateStats.conns_skipped += stat.conns_skipped;
155156
aggregateStats.no_tcp_opt_hdr += stat.no_tcp_opt_hdr;
156157
aggregateStats.error_bad_id += stat.error_bad_id;
158+
aggregateStats.error_server_id_zero += stat.error_server_id_zero;
157159
aggregateStats.error_write_opt += stat.error_write_opt;
158160
aggregateStats.error_sys_calls += stat.error_sys_calls;
159161
aggregateStats.ignoring_due_to_kde += stat.ignoring_due_to_kde;

katran/tpr/TPRTypes.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ struct tcp_router_stats {
4242
uint64_t conns_skipped{0};
4343
uint64_t no_tcp_opt_hdr{0};
4444
uint64_t error_bad_id{0};
45+
uint64_t error_server_id_zero{0};
4546
uint64_t error_write_opt{0};
4647
uint64_t error_sys_calls{0};
4748
uint64_t ignoring_due_to_kde{0};
@@ -92,6 +93,7 @@ inline std::ostream& operator<<(std::ostream& os, const tcp_router_stats& s) {
9293
<< " no_tcp_opt_hdr=" << s.no_tcp_opt_hdr
9394
<< " ignoring_kde=" << s.ignoring_due_to_kde
9495
<< " error_bad_id=" << s.error_bad_id
96+
<< " error_server_id_zero=" << s.error_server_id_zero
9597
<< " error_write_opt=" << s.error_write_opt
9698
<< " error_sys_calls=" << s.error_sys_calls
9799
<< " legacy_server_opt=" << s.legacy_server_opt

katran/tpr/bpf/tcp_pkt_router_active_hdlr.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ static inline int handle_active_parse_hdr(
4747
return err;
4848
}
4949
if (!hdr_opt.server_id) {
50-
stat->error_bad_id++;
50+
stat->error_server_id_zero++;
5151
TPR_PRINT(skops, "active parsed empty server id from the option");
5252
return PASS;
5353
}
@@ -102,7 +102,7 @@ static int handle_active_write_hdr_opt(
102102
// while storing 'hdr_opt.server_id'. Send id == 0 so that the passive
103103
// side can try sending back proper id in the next round.
104104
hdr_opt.server_id = 0;
105-
stat->error_bad_id++;
105+
stat->error_server_id_zero++;
106106
TPR_PRINT(skops, "active failed to read server id from storage");
107107
} else {
108108
hdr_opt.server_id = *existing_id;
@@ -139,7 +139,7 @@ static inline int handle_active_estab(
139139
}
140140
if (_UNLIKELY(!hdr_opt.server_id)) {
141141
// Received tcp-hdr-opt but no server_id recv'ed from peer.
142-
stat->error_bad_id++;
142+
stat->error_server_id_zero++;
143143
// write a server_id 0 out to tell passive side to resend its server_id
144144
set_write_hdr_cb_flags(skops, stat);
145145
// try to read in the next round

katran/tpr/bpf/tcp_pkt_router_passive_hdlr.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ static inline int handle_passive_parse_hdr(
5555
}
5656
if (!hdr_opt.server_id) {
5757
// no server_id received from peer.
58-
stat->error_bad_id++;
58+
stat->error_server_id_zero++;
5959
TPR_PRINT(skops, "passive received 0 server id");
6060
return PASS;
6161
}

katran/tpr/bpf/tcp_pkt_router_structs.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ struct stats {
3737
__u64 conns_skipped;
3838
__u64 no_tcp_opt_hdr;
3939
__u64 error_bad_id;
40+
__u64 error_server_id_zero;
4041
__u64 error_write_opt;
4142
__u64 error_sys_calls;
4243
__u64 ignoring_due_to_kde;

0 commit comments

Comments
 (0)