UCT/TCP: added reachability mode conf var to tcp_iface #10983
Conversation
|
@shasson5 please look |
WalkthroughAdds a configurable TCP interface reachability mode enum and wires it through the public config, initialization, and the reachability check so the iface can treat addresses as routability-checked or universally reachable. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as reachability caller
participant IFACE as TCP_IFACE\n(config.reachability_mode)
participant SCOPE as ScopeChecker
participant ROUTE as RoutabilityChecker
Caller->>IFACE: uct_tcp_iface_is_reachable_v2(addr)
alt config == ALL
IFACE->>SCOPE: uct_iface_scope_is_reachable(addr)
SCOPE-->>IFACE: reachable / not reachable
IFACE-->>Caller: return result
else config == ROUTE
IFACE->>ROUTE: check address routability (if_nametoindex / name-index)
ROUTE-->>IFACE: routable / not routable
IFACE-->>Caller: return result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/uct/tcp/tcp.h (1)
232-240: Consider naming consistency with other TCP iface types.The enum is named
uct_tcp_reachability_mode_t, but given that this is specific to the TCP interface configuration and following the pattern of other types likeuct_tcp_iface_config_tanduct_tcp_iface_addr_t, the nameuct_tcp_iface_reachability_mode_twould be more consistent with the codebase conventions.Apply this diff to align with naming conventions:
-typedef enum { +typedef enum uct_tcp_iface_reachability_mode { UCT_TCP_REACHABILITY_MODE_ROUTE = 0, UCT_TCP_REACHABILITY_MODE_ALL = 1, UCT_TCP_REACHABILITY_MODE_LAST -} uct_tcp_reachability_mode_t; +} uct_tcp_iface_reachability_mode_t;Also update the corresponding field declarations on lines 435-436 and 474, and the array declaration in
src/uct/tcp/tcp_iface.c.src/uct/tcp/tcp_iface.c (1)
257-265: Add comment explaining the reachability bypass logic.The conditional logic on lines 257-260 is somewhat complex. When
reachability_modeis set toALL, the function delegates touct_iface_scope_is_reachable, which returns 1 forNETWORKscope (the default). This effectively treats all addresses as reachable without performing route verification. The assertion on lines 264-265 is good defensive programming to ensure onlyROUTEmode reaches the route checking code.Consider adding a clarifying comment before the conditional:
+ /* For DEVICE scope or ALL reachability mode, delegate to base function + * which bypasses route checking and treats all addresses as reachable */ if (((params->field_mask & UCT_IFACE_IS_REACHABLE_FIELD_SCOPE) && (params->scope == UCT_IFACE_REACHABILITY_SCOPE_DEVICE)) || (iface->config.reachability_mode == UCT_TCP_REACHABILITY_MODE_ALL)) { return uct_iface_scope_is_reachable(tl_iface, params); } /* Check if the remote address is routable */ ucs_assert(iface->config.reachability_mode == UCT_TCP_REACHABILITY_MODE_ROUTE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/uct/tcp/tcp.h(3 hunks)src/uct/tcp/tcp_iface.c(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/uct/tcp/tcp_iface.c (1)
src/uct/base/uct_iface.c (1)
uct_iface_scope_is_reachable(303-322)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: UCX PR (Codestyle ctags check)
- GitHub Check: UCX PR (Codestyle commit title)
- GitHub Check: UCX PR (Codestyle format code)
- GitHub Check: UCX PR (Codestyle codespell check)
- GitHub Check: UCX PR (Codestyle AUTHORS file update check)
- GitHub Check: UCX release DRP (Prepare CheckRelease)
- GitHub Check: UCX release (Prepare CheckRelease)
- GitHub Check: UCX snapshot (Prepare Check)
🔇 Additional comments (5)
src/uct/tcp/tcp.h (2)
435-436: Verify external declaration for the modes array.The
reachability_modefield is correctly added to the config structure. However, I don't see anexterndeclaration for theuct_tcp_reachability_modes[]array that is defined insrc/uct/tcp/tcp_iface.c. This array is referenced in the config table viaUCS_CONFIG_TYPE_ENUM()macro.Please verify whether an extern declaration should be added near the other external declarations (around line 518-522) like:
extern const char *uct_tcp_reachability_modes[];This would make the array properly visible to code that includes this header file.
474-474: Configuration field looks good.The
reachability_modefield is correctly added to the configuration structure, maintaining consistency with the runtime config structure.src/uct/tcp/tcp_iface.c (3)
25-29: Array definition is correct.The reachability modes array is properly defined with correct enum indices and NULL terminator for the LAST element.
127-132: Configuration entry documentation is clear.The configuration entry properly documents both modes and their behavior. The default value "route" maintains backward compatibility.
782-782: Configuration propagation is correct.The
reachability_modevalue is properly propagated from the config to the runtime iface structure.
|
BTW why is WIP label set? |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/uct/tcp/tcp.h (3)
232-240: Add documentation for the enum and its values.The enum lacks documentation comments explaining what each reachability mode does. Based on the PR objectives, ROUTE performs route checking (default behavior), while ALL allows all addresses without route checking.
Apply this diff to add documentation:
+/** + * TCP Reachability mode. + * Determines how reachability checks are performed for TCP interfaces. + */ typedef enum { + UCT_TCP_REACHABILITY_MODE_ROUTE = 0, /* Check reachability via routing table (default) */ + UCT_TCP_REACHABILITY_MODE_ALL = 1, /* Allow all addresses without route checking */ - UCT_TCP_REACHABILITY_MODE_ROUTE = 0, - UCT_TCP_REACHABILITY_MODE_ALL = 1, UCT_TCP_REACHABILITY_MODE_LAST } uct_tcp_reachability_mode_t;
435-435: Add documentation for the reachability_mode field.The
reachability_modefield lacks a comment explaining its purpose. Consider adding an inline comment similar to other fields in the config struct.Apply this diff:
- uct_tcp_reachability_mode_t reachability_mode; /* Mode used for performing reachability check */ + uct_tcp_reachability_mode_t reachability_mode; /* Reachability checking mode: ROUTE (default, via routing table) or ALL (skip route checks) */
473-473: Consider adding documentation for the reachability_mode field.Similar to other config fields, consider adding a brief comment for clarity, though the type name is self-documenting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/uct/tcp/tcp.h(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: UCX PR (Codestyle ctags check)
- GitHub Check: UCX PR (Codestyle codespell check)
- GitHub Check: UCX PR (Codestyle commit title)
- GitHub Check: UCX PR (Codestyle format code)
- GitHub Check: UCX PR (Codestyle AUTHORS file update check)
- GitHub Check: UCX release DRP (Prepare CheckRelease)
- GitHub Check: UCX release DRP (Prepare CheckRelease)
- GitHub Check: UCX release (Prepare CheckRelease)
- GitHub Check: UCX release (Prepare CheckRelease)
- GitHub Check: UCX snapshot (Prepare Check)
- GitHub Check: UCX snapshot (Prepare Check)
🔇 Additional comments (2)
src/uct/tcp/tcp.h (2)
378-443: LGTM: Struct alignment addresses previous review feedback.The alignment changes to
uct_tcp_iface_taddress the clang false positive issue mentioned in the previous review comments. The substantive change is the addition of thereachability_modefield at line 435.Based on learnings.
516-520: The review comment appears to be based on an incorrect assumption about the intended scope of the array.Based on verification, the array
uct_tcp_reachability_modes[]is defined and used exclusively withinsrc/uct/tcp/tcp_iface.c(defined at line 25, used at line 132). The script found zero references to this array outside of that file. For internal-only usage within a single compilation unit, an extern declaration in the header is not required. The similar pattern with other declared arrays in the header (likeuct_tcp_address_type_names[]) reflects those being part of the public TCP component API, whereas this reachability modes array appears to be intentionally scoped to internal configuration handling.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/uct/tcp/tcp.h (2)
232-240: Consider documenting the enum values.The enum values
UCT_TCP_REACHABILITY_MODE_ROUTEandUCT_TCP_REACHABILITY_MODE_ALLwould benefit from inline documentation explaining their behavior. Based on the PR objectives, ROUTE performs route checking (default) while ALL skips route checking and allows all addresses.Consider applying this documentation:
/** - * TCP Reachability mode. + * TCP Reachability mode for determining interface reachability. */ typedef enum { - UCT_TCP_REACHABILITY_MODE_ROUTE = 0, - UCT_TCP_REACHABILITY_MODE_ALL = 1, + UCT_TCP_REACHABILITY_MODE_ROUTE = 0, /* Perform route checking (default) */ + UCT_TCP_REACHABILITY_MODE_ALL = 1, /* Skip route checking, allow all addresses */ UCT_TCP_REACHABILITY_MODE_LAST } uct_tcp_reachability_mode_t;
473-473: Consider adding an inline comment for consistency.The corresponding field in
uct_tcp_iface_t(line 435) has an inline comment explaining its purpose. Adding a similar comment here would improve consistency.- uct_tcp_reachability_mode_t reachability_mode; + uct_tcp_reachability_mode_t reachability_mode; /* Mode used for performing reachability check */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/uct/tcp/tcp.h(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: UCX PR (Static_check Static checks)
- GitHub Check: UCX PR (Codestyle codespell check)
- GitHub Check: UCX PR (Codestyle format code)
- GitHub Check: UCX PR (Codestyle AUTHORS file update check)
- GitHub Check: UCX PR (Codestyle ctags check)
- GitHub Check: UCX PR (Codestyle commit title)
- GitHub Check: UCX release DRP (Prepare CheckRelease)
- GitHub Check: UCX release (Prepare CheckRelease)
- GitHub Check: UCX snapshot (Prepare Check)
🔇 Additional comments (2)
src/uct/tcp/tcp.h (2)
435-435: LGTM!The
reachability_modefield is correctly added to the interface config structure with appropriate typing and documentation.
346-368: Alignment changes address previous review feedback.The extensive alignment changes across the struct definitions appropriately address the concerns raised in previous reviews about clang false positives with struct definitions.
Also applies to: 377-443, 449-474
amastbaum
force-pushed
the
configure_reachability_mode_in_tcp_iface
branch
from
e7aa4df to
1ad0201
Compare
3 participants
What?
Added
UCX_TCP_REACHABILITY_MODEconfig (default:route) to optionally allow all addresses without route checking when set toall.Solving bug: https://redmine.mellanox.com/issues/4659780
Summary by CodeRabbit