Add VRRP checks for if instance is an owner. #37
Conversation
VRRP instance is an owner if it holds a virtual IP address as it's parent interface. Currently being checked during virtual-address configuration and when the primary interface has change in addresses (addition / deletion) which may affect the is_owner state. Signed-off-by: Paul Wekesa <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #37 +/- ##
==========================================
+ Coverage 57.90% 58.37% +0.47%
==========================================
Files 201 200 -1
Lines 50537 50135 -402
Branches 50537 50135 -402
==========================================
+ Hits 29264 29267 +3
+ Misses 19379 19132 -247
+ Partials 1894 1736 -158 ☔ View full report in Codecov by Sentry. |
holo-vrrp/src/instance.rs
Outdated
| .virtual_addresses | ||
| .iter() | ||
| .any(|addr| interface_sys.addresses.contains(addr)); | ||
| false |
There was a problem hiding this comment.
You should return the result of the last expression instead of always returning false
holo-vrrp/src/instance.rs
Outdated
| @@ -403,6 +412,16 @@ impl Instance { | |||
| let _ = net.net_tx_packetp.send(msg); | |||
| } | |||
| } | |||
|
|
|||
| /// an instance is an owner if the ip address of the parent interface is | |||
There was a problem hiding this comment.
Comments should start with an uppercase for consistency with the rest of the codebase
holo-vrrp/src/instance.rs
Outdated
| @@ -403,6 +412,16 @@ impl Instance { | |||
| let _ = net.net_tx_packetp.send(msg); | |||
| } | |||
| } | |||
|
|
|||
| /// an instance is an owner if the ip address of the parent interface is | |||
| /// part of the virutal addresses held by the instance. | |||
| @@ -234,6 +238,9 @@ impl Provider for Interface { | |||
| ); | |||
| instance.timer_set(&interface); | |||
| } | |||
| // owner status may change when virtual address is added | |||
There was a problem hiding this comment.
Owner status can also change when an IP address is added to or removed from the interface.
For simplicity, we could compute the owner status on demand whenever necessary, instead of caching and continuously updating it. There's only a few places where we need to consult the owner status, and computing it is a cheap operation.
There was a problem hiding this comment.
@rwestphal You're right, the owner status can also change when an IP address is added or removed from an interface.
I will add the check_is_owner() to only be called on demand. This will also mean removing the is_owner field from Instance.
My thought process was I wanted to cover cases where we have access to Instance but no access to InterfaceSys(which is needed for checking owner status). Seeing how the VRRP codebase is structured, that was a pretty terrible choice since we always fetch the Instance from the Interface and will almost always have access to InterfaceSys, either via the View or the Interface directly.
Update vrrp's is_owner checks based on reviews: - call check_is_owner() on demand. - Comment Capitalization. - spelling checks General changes: - have the check_is_owner check be positive for all virtual IP addresses instead of any of them Signed-off-by: Paul Wekesa <[email protected]>
|
@rwestphal I believe the failing clippy check is a false negative. |
VRRP instance is an owner if it holds a virtual IP address as it's parent interface. Currently checked during virtual-address configuration and when the primary interface has change in addresses (addition / deletion) which may affect the is_owner state.
Adding owner logic to FSM is work in progress