Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add VRRP checks for if instance is an owner. #37

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add VRRP checks for if instance is an owner. #37

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a31c2a6
vrrp: add owner checks
Paul-weqe Dec 11, 2024
db00218
vrrp: remove TODO comment
Paul-weqe Dec 11, 2024
023a835
vrrp: update is-owner checks
Paul-weqe Dec 12, 2024
cb07449
chore: fix extract_if method signature
Paul-weqe Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions holo-vrrp/src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ pub struct InstanceState {
pub timer: VrrpTimer,
pub last_adv_src: Option<Ipv4Addr>,
pub statistics: Statistics,
pub is_owner: bool,
}

#[derive(Debug)]
Expand Down Expand Up @@ -165,7 +166,8 @@ impl Instance {
match InstanceNet::new(interface, &self.mvlan) {
Ok(net) => {
self.net = Some(net);
if self.config.priority == 255 {
self.state.is_owner = self.check_is_owner(interface.system);
if self.config.priority == 255 || self.state.is_owner {
let src_ip =
interface.system.addresses.first().unwrap().ip();
self.send_vrrp_advertisement(src_ip);
Expand Down Expand Up @@ -314,11 +316,18 @@ impl Instance {
ip_addresses.push(addr.ip());
}

// RFC 3768 -> 5.3.4. Priority
let priority = if self.state.is_owner {
255
} else {
self.config.priority
};

let mut packet = VrrpHdr {
version: 2,
hdr_type: 1,
vrid: self.vrid,
priority: self.config.priority,
priority,
count_ip: self.config.virtual_addresses.len() as u8,
auth_type: 0,
adver_int: self.config.advertise_interval,
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should start with an uppercase for consistency with the rest of the codebase

/// part of the virutal addresses held by the instance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo (s/virutal/virtual/)

pub(crate) fn check_is_owner(&self, interface_sys: &InterfaceSys) -> bool {
self.config
.virtual_addresses
.iter()
.any(|addr| interface_sys.addresses.contains(addr));
false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should return the result of the last expression instead of always returning false

}
}

impl Drop for Instance {
Expand Down
7 changes: 7 additions & 0 deletions holo-vrrp/src/northbound/configuration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,10 @@ impl Provider for Interface {
);
instance.timer_set(&interface);
}

// owner status may change when virtual address is added
instance.state.is_owner =
instance.check_is_owner(interface.system);
}
Event::VirtualAddressDelete { vrid, addr } => {
let (interface, instance) = self.get_instance(vrid).unwrap();
Expand All @@ -234,6 +238,9 @@ impl Provider for Interface {
);
instance.timer_set(&interface);
}
// owner status may change when virtual address is added
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@Paul-weqe Paul-weqe Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

instance.state.is_owner =
instance.check_is_owner(interface.system);
}
Event::ResetTimer { vrid } => {
let (_, instance) = self.get_instance(vrid).unwrap();
Expand Down
3 changes: 1 addition & 2 deletions holo-vrrp/src/northbound/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@ fn load_callbacks() -> Callbacks<Interface> {
Box::new(VrrpInstance {
vrid: *vrid,
state: Some(instance.state.state.to_yang()),
// TODO
is_owner: None,
is_owner: Some(instance.state.is_owner),
last_adv_source: instance.state.last_adv_src.map(std::convert::Into::into).map(Cow::Owned).ignore_in_testing(),
up_datetime: instance.state.up_time.as_ref().map(Cow::Borrowed).ignore_in_testing(),
master_down_interval: instance.state.timer.as_master_down_timer().map(|task| task.remaining().as_millis() as u32 / 10).ignore_in_testing(),
Expand Down
Loading