>> Another issue: If the node is Elected as BSR and I add another
>> Cand-RP, it triggers bsr_stop() and bsr_start() what causes it to
>> lose Elected state and return to Pending. Although this is not
>> wrong, it would be better to just keep the state (the stop()
>> causes Cand-RP-ADV with zero holdtime, what changes the state also
>> in remote peers). I was wondering about writing an update method
>> that apply the actions in stop() and start() only in the changed
>> zones. What do you think?
>
> Accidentally, couple of days ago I was thinking about the same when I
> noticed the bsr_stop()/bsr_start().
>
> If I remember correctly, I used bsr_stop()/bsr_start() because there
> was lots of complexity when I tried to do the incremental update.
> Though, I don't remember whether this applied to the first version
> of the BSR implementation or to the (almost complete) rewrite I had
> to do after I discovered major issues with the first version.
The code in CVS is the rewritten code?
> Another reason for the bsr_stop()/bsr_start() was because I wanted
> atomic update (e.g., transaction like), otherwise during
> reconfiguration there will be lots of flux with potentially
> dangerous results (remember that consistent Cand-RP set across
> all PIM-SM routers is critical).
> At that time there was no easy way to apply some transaction-based
> mechanism to achieve the atomic update, so the
> bsr_stop()/bsr_start() was the simple work-around (e.g., note that
> it is in the etc/templates/pimsm*.tp template files).
>
> If you can find a simple way to do the update atomically without
> bsr_stop()/bsr_start(), then give it a try and see how well it
> works.
> Otherwise, please submit a Bugzilla entry about the issue.
I played around a bit with this.
I've tried to keep as close as possible to the stop()/start() approach.
My idea was:
- Add a boolean parameter to bsr_start()/bsr_stop that is true during restarts
- Create a bsr_restart() method that calls bsr_stop(true) and then
bsr_start(true)
- In stop(true), instead deleting the whole _active_bsr_zone_list delete only
the non-elected zones. For the elected zones, delete all groupprefix (which
contains the rps).
- In start(true) it will reuse the existing elected zone and add all configured
groupprefix (and rps)
- In start(true), check if any elected zone was deleted from config and vanish
them.
- After that, I put the elected zones back in the PENDING state and expire the
bsr timer. The timer will put it back in the ELECTED state, compute the rp-set
and send BSR Messages with the new rp-set.
In my tests it seems to work fine.
I'm sending the patch attached.
Can you have a look on it and check if I've forgotten something?
Regards,
- Samuel
>From 5b0a9a21b06a5f2c743ae53edb68717f0165fe23 Mon Sep 17 00:00:00 2001
From: Samuel Lucas Vaz de Mello <[EMAIL PROTECTED]>
Date: Wed, 29 Oct 2008 11:25:30 -0200
Subject: [PATCH] Support BSR restart without losing BSR Elected state
---
etc/templates/pimsm4.tp | 6 +--
pim/pim_bsr.cc | 120 ++++++++++++++++++++++++++++++++++++++++----
pim/pim_bsr.hh | 11 +++-
pim/pim_config.cc | 8 ++--
pim/pim_node.hh | 7 +++
pim/xrl_pim_node.cc | 23 +++++++++
pim/xrl_pim_node.hh | 3 +
xrl/interfaces/pim.xif | 3 +-
xrl/interfaces/pim_xif.cc | 38 ++++++++++++++
xrl/interfaces/pim_xif.hh | 15 +++++-
xrl/targets/pim.xrls | 4 +-
xrl/targets/pim_base.cc | 29 +++++++++++
xrl/targets/pim_base.hh | 6 ++-
13 files changed, 248 insertions(+), 25 deletions(-)
diff --git a/etc/templates/pimsm4.tp b/etc/templates/pimsm4.tp
index 4c0cbb2..d430efd 100644
--- a/etc/templates/pimsm4.tp
+++ b/etc/templates/pimsm4.tp
@@ -207,10 +207,8 @@ protocols {
bootstrap {
%help: short "Configure the IPv4 Bootstrap mechanism";
- %activate: xrl "$(pimsm4.targetname)/pim/0.1/stop_bsr";
- %activate: xrl "$(pimsm4.targetname)/pim/0.1/start_bsr";
- %update: xrl "$(pimsm4.targetname)/pim/0.1/stop_bsr";
- %update: xrl "$(pimsm4.targetname)/pim/0.1/start_bsr";
+ %activate: xrl "$(pimsm4.targetname)/pim/0.1/restart_bsr";
+ %update: xrl "$(pimsm4.targetname)/pim/0.1/restart_bsr";
%delete: xrl "$(pimsm4.targetname)/pim/0.1/stop_bsr";
disable {
diff --git a/pim/pim_bsr.cc b/pim/pim_bsr.cc
index bd55709..4280ec6 100644
--- a/pim/pim_bsr.cc
+++ b/pim/pim_bsr.cc
@@ -113,21 +113,56 @@ PimBsr::clear()
* Return value: %XORP_OK on success, otherwize %XORP_ERROR.
**/
int
-PimBsr::start()
+PimBsr::start(bool restarting)
{
if (! is_enabled())
return (XORP_OK);
- if (is_up() || is_pending_up())
+ if ((!restarting) && (is_up() || is_pending_up()))
return (XORP_OK);
if (ProtoUnit::start() != XORP_OK)
return (XORP_ERROR);
+ // if restarting, clean up any deleted zone
+ list<BsrZone *>::iterator iter;
+ if (restarting){
+ list<BsrZone*> del_list;
+ for (iter = _active_bsr_zone_list.begin();
+ iter != _active_bsr_zone_list.end();
+ ++iter){
+ BsrZone *active_bsr_zone = *iter;
+
+ if (active_bsr_zone->bsr_zone_state() == BsrZone::STATE_ELECTED_BSR){
+
+ // zone removed from config
+ BsrZone *config_bsr_zone = find_config_bsr_zone(active_bsr_zone->zone_id());
+ if (config_bsr_zone == NULL){
+ del_list.push_back(active_bsr_zone);
+ continue;
+ }
+
+ // zone is no longer BSR candidate (i.e., removed from cand-bsr but
+ // still have any cand-rp)
+ // Remove it too, as the cand-rps are re-added below
+ if (!config_bsr_zone->i_am_candidate_bsr()){
+ del_list.push_back(active_bsr_zone);
+ continue;
+ }
+ }
+ }
+
+ // delete the vanished items
+ for (iter = del_list.begin(); iter != del_list.end(); ++iter){
+ BsrZone *active_bsr_zone = *iter;
+ _active_bsr_zone_list.remove(active_bsr_zone);
+ }
+ delete_pointers_list(del_list);
+ }
+
//
// Activate all configured BSR zones
//
- list<BsrZone *>::iterator iter;
for (iter = _config_bsr_zone_list.begin();
iter != _config_bsr_zone_list.end();
++iter) {
@@ -146,7 +181,34 @@ PimBsr::start()
config_bsr_zone->start_candidate_rp_advertise_timer();
}
- XLOG_INFO("Bootstrap mechanism started");
+ // When restarting we want to keep any previously enabled BSR.
+ // Set it to PENDING and expire the bsr_timer immediately.
+ // As soon as the timer runs, it puts the zone back in the ELECTED
+ // state and run the needed actions (calculate rp-set and send a
+ // BSR Message with the new rp-set)
+ if (restarting){
+ for (iter = _active_bsr_zone_list.begin();
+ iter != _active_bsr_zone_list.end();
+ ++iter){
+ BsrZone *active_bsr_zone = *iter;
+
+ if (active_bsr_zone->bsr_zone_state() == BsrZone::STATE_ELECTED_BSR){
+
+ if (active_bsr_zone->i_am_candidate_bsr()){
+ // zone was not changed
+ active_bsr_zone->set_bsr_zone_state(BsrZone::STATE_PENDING_BSR);
+ }else{
+ // zone is no longer Cand-BSR, but contain Cand-RPs
+ active_bsr_zone->set_bsr_zone_state(BsrZone::STATE_ACCEPT_ANY);
+ }
+ active_bsr_zone->expire_bsr_timer();
+ }
+ }
+
+ }
+
+
+ XLOG_INFO("Bootstrap mechanism started %s", restarting?"(restarting)":"");
return (XORP_OK);
}
@@ -160,7 +222,7 @@ PimBsr::start()
* Return value: %XORP_OK on success, otherwise %XORP_ERROR.
**/
int
-PimBsr::stop()
+PimBsr::stop(bool restarting)
{
PimVif* pim_vif;
string dummy_error_msg;
@@ -271,7 +333,32 @@ PimBsr::stop()
}
// Remove the lists of active and expiring BsrZone entries
- delete_pointers_list(_active_bsr_zone_list);
+ if (!restarting){
+ delete_pointers_list(_active_bsr_zone_list);
+ }else{
+ list<BsrZone*> del_list;
+ // when restarting, preserve any elected BSR zone.
+ for (list<BsrZone*>::iterator iter = _active_bsr_zone_list.begin();
+ iter != _active_bsr_zone_list.end();
+ iter++){
+ BsrZone *tmp_zone = (*iter);
+ // if the bsr_zone is not elected BSR, just remove it
+ if (tmp_zone->bsr_zone_state() != BsrZone::STATE_ELECTED_BSR){
+ del_list.push_back(tmp_zone);
+ }else{
+ // it's an Elected BSR. we will keep the bsr_zone in the list
+ // but remove all cand-rps. Some of them may have been deleted.
+ // The cand-rps that were not deleted will be added again in start()
+ delete_pointers_list(tmp_zone->bsr_group_prefix_list());
+ }
+ }
+ for (list<BsrZone*>::iterator iter = del_list.begin(); iter != del_list.end(); ++iter){
+ _active_bsr_zone_list.remove(*iter);
+ }
+ delete_pointers_list(del_list);
+ }
+
+ // clean up expire zone
delete_pointers_list(_expire_bsr_zone_list);
// Cancel unwanted timers
@@ -283,11 +370,24 @@ PimBsr::stop()
// synchronize internal state.
//
- XLOG_INFO("Bootstrap mechanism stopped");
+ XLOG_INFO("Bootstrap mechanism stopped %s", restarting?"(restarting)":"");
return (XORP_OK);
}
+int
+PimBsr::restart()
+{
+ if (stop(true) != XORP_OK)
+ return (XORP_ERROR);
+
+ if (start(true) != XORP_OK)
+ return (XORP_ERROR);
+
+ return (XORP_OK);
+}
+
+
/**
* Enable the node operation.
*
@@ -883,8 +983,7 @@ PimBsr::add_vif_addr(uint32_t vif_index, const IPvX& vif_addr)
// If the BSR is running, and we found a matching entry, then restart
//
if (found && old_is_up) {
- stop();
- start();
+ restart();
}
}
@@ -950,8 +1049,7 @@ PimBsr::delete_vif_addr(uint32_t vif_index, const IPvX& vif_addr)
// If the BSR is running, and we found a matching entry, then restart
//
if (found && old_is_up) {
- stop();
- start();
+ restart();
}
}
diff --git a/pim/pim_bsr.hh b/pim/pim_bsr.hh
index e42e241..c08ae7e 100644
--- a/pim/pim_bsr.hh
+++ b/pim/pim_bsr.hh
@@ -69,14 +69,21 @@ public:
*
* @return XORP_OK on success, otherwise XORP_ERROR.
*/
- int start();
+ int start(bool restarting=false);
/**
* Stop operation.
*
* @return XORP_OK on success, otherwise XORP_ERROR.
*/
- int stop();
+ int stop(bool restarting=false);
+
+ /**
+ * Restart operation, preserving Elected BSR state
+ *
+ * @return XORP_OK on success, otherwise XORP_ERROR.
+ */
+ int restart();
/**
* Enable operation.
diff --git a/pim/pim_config.cc b/pim/pim_config.cc
index 48bfa27..8b86975 100644
--- a/pim/pim_config.cc
+++ b/pim/pim_config.cc
@@ -1284,7 +1284,7 @@ PimNode::delete_config_cand_bsr(const IPvXNet& scope_zone_id,
// Stop the BSR, delete the BSR zone, and restart the BSR if necessary
//
is_up = pim_bsr().is_up();
- pim_bsr().stop();
+ pim_bsr().stop(true);
if (bsr_zone->bsr_group_prefix_list().empty()) {
// No Cand-RP, therefore delete the zone.
@@ -1297,7 +1297,7 @@ PimNode::delete_config_cand_bsr(const IPvXNet& scope_zone_id,
}
if (is_up)
- pim_bsr().start(); // XXX: restart the BSR
+ pim_bsr().start(true); // XXX: restart the BSR
if (end_config(error_msg) != XORP_OK)
return (XORP_ERROR);
@@ -1514,7 +1514,7 @@ PimNode::delete_config_cand_rp(const IPvXNet& group_prefix,
// Stop the BSR, delete the RP zone, and restart the BSR if necessary
//
is_up = pim_bsr().is_up();
- pim_bsr().stop();
+ pim_bsr().stop(true);
bsr_group_prefix->delete_rp(bsr_rp);
bsr_rp = NULL;
@@ -1531,7 +1531,7 @@ PimNode::delete_config_cand_rp(const IPvXNet& group_prefix,
}
if (is_up)
- pim_bsr().start(); // XXX: restart the BSR
+ pim_bsr().start(true); // XXX: restart the BSR
if (end_config(error_msg) != XORP_OK)
return (XORP_ERROR);
diff --git a/pim/pim_node.hh b/pim/pim_node.hh
index 6ae99b8..79671c6 100644
--- a/pim/pim_node.hh
+++ b/pim/pim_node.hh
@@ -872,6 +872,13 @@ public:
* @return XORP_OK on success, otherwise XORP_ERROR.
*/
int stop_bsr() { return (pim_bsr().stop()); }
+
+ /**
+ * Restart the Bootstrap mechanism, preserving Elected BSR state
+ *
+ * @return XORP_OK on success, otherwise XORP_ERROR.
+ */
+ int restart_bsr() { return (pim_bsr().restart()); }
//
// Configuration methods
diff --git a/pim/xrl_pim_node.cc b/pim/xrl_pim_node.cc
index ee1f9c7..80c5ece 100644
--- a/pim/xrl_pim_node.cc
+++ b/pim/xrl_pim_node.cc
@@ -226,6 +226,16 @@ XrlPimNode::stop_bsr()
return (XORP_OK);
}
+int
+XrlPimNode::restart_bsr()
+{
+ if (PimNode::restart_bsr() != XORP_OK)
+ return (XORP_ERROR);
+
+ return (XORP_OK);
+}
+
+
//
// Finder-related events
//
@@ -3930,6 +3940,19 @@ XrlPimNode::pim_0_1_stop_bsr()
return XrlCmdError::OKAY();
}
+XrlCmdError
+XrlPimNode::pim_0_1_restart_bsr()
+{
+ string error_msg;
+
+ if (restart_bsr() != XORP_OK) {
+ error_msg = c_format("Failed to restart PIM BSR");
+ return XrlCmdError::COMMAND_FAILED(error_msg);
+ }
+
+ return XrlCmdError::OKAY();
+}
+
//
// PIM configuration
//
diff --git a/pim/xrl_pim_node.hh b/pim/xrl_pim_node.hh
index ed9424a..f4b4f49 100644
--- a/pim/xrl_pim_node.hh
+++ b/pim/xrl_pim_node.hh
@@ -113,6 +113,7 @@ public:
int disable_bsr();
int start_bsr();
int stop_bsr();
+ int restart_bsr();
// XrlTask relatedMethods that need to be public
void send_register_unregister_interest();
@@ -707,6 +708,8 @@ protected:
XrlCmdError pim_0_1_start_bsr();
XrlCmdError pim_0_1_stop_bsr();
+
+ XrlCmdError pim_0_1_restart_bsr();
/**
* Add/delete scope zone.
diff --git a/xrl/interfaces/pim.xif b/xrl/interfaces/pim.xif
index ba018a3..2cbc218 100644
--- a/xrl/interfaces/pim.xif
+++ b/xrl/interfaces/pim.xif
@@ -49,7 +49,7 @@ interface pim/0.1 {
stop_cli
/**
- * Enable/disable/start/stop BSR.
+ * Enable/disable/start/stop/restart BSR.
*
* @param enable if true, then enable the BSR, otherwise
* disable it.
@@ -57,6 +57,7 @@ interface pim/0.1 {
enable_bsr ? enable:bool
start_bsr
stop_bsr
+ restart_bsr
/**
* Add/delete scope zone.
diff --git a/xrl/interfaces/pim_xif.cc b/xrl/interfaces/pim_xif.cc
index 2fdb927..1c2eb3c 100644
--- a/xrl/interfaces/pim_xif.cc
+++ b/xrl/interfaces/pim_xif.cc
@@ -607,6 +607,44 @@ XrlPimV0p1Client::unmarshall_stop_bsr(
}
bool
+XrlPimV0p1Client::send_restart_bsr(
+ const char* dst_xrl_target_name,
+ const RestartBsrCB& cb
+)
+{
+ static Xrl* x = NULL;
+
+ if (!x) {
+ x = new Xrl(dst_xrl_target_name, "pim/0.1/restart_bsr");
+ }
+
+ x->set_target(dst_xrl_target_name);
+
+
+ return _sender->send(*x, callback(this, &XrlPimV0p1Client::unmarshall_restart_bsr, cb));
+}
+
+
+/* Unmarshall restart_bsr */
+void
+XrlPimV0p1Client::unmarshall_restart_bsr(
+ const XrlError& e,
+ XrlArgs* a,
+ RestartBsrCB cb
+)
+{
+ if (e != XrlError::OKAY()) {
+ cb->dispatch(e);
+ return;
+ } else if (a && a->size() != 0) {
+ XLOG_ERROR("Wrong number of arguments (%u != %u)", XORP_UINT_CAST(a->size()), XORP_UINT_CAST(0));
+ cb->dispatch(XrlError::BAD_ARGS());
+ return;
+ }
+ cb->dispatch(e);
+}
+
+bool
XrlPimV0p1Client::send_add_config_scope_zone_by_vif_name4(
const char* dst_xrl_target_name,
const IPv4Net& scope_zone_id,
diff --git a/xrl/interfaces/pim_xif.hh b/xrl/interfaces/pim_xif.hh
index 7705745..a9fbaf4 100644
--- a/xrl/interfaces/pim_xif.hh
+++ b/xrl/interfaces/pim_xif.hh
@@ -160,7 +160,7 @@ public:
/**
* Send Xrl intended to:
*
- * Enable/disable/start/stop BSR.
+ * Enable/disable/start/stop/restart BSR.
*
* @param dst_xrl_target_name the Xrl target name of the destination.
*
@@ -186,6 +186,13 @@ public:
const StopBsrCB& cb
);
+ typedef XorpCallback1<void, const XrlError&>::RefPtr RestartBsrCB;
+
+ bool send_restart_bsr(
+ const char* dst_xrl_target_name,
+ const RestartBsrCB& cb
+ );
+
typedef XorpCallback1<void, const XrlError&>::RefPtr AddConfigScopeZoneByVifName4CB;
/**
* Send Xrl intended to:
@@ -2205,6 +2212,12 @@ private:
StopBsrCB cb
);
+ void unmarshall_restart_bsr(
+ const XrlError& e,
+ XrlArgs* a,
+ RestartBsrCB cb
+ );
+
void unmarshall_add_config_scope_zone_by_vif_name4(
const XrlError& e,
XrlArgs* a,
diff --git a/xrl/targets/pim.xrls b/xrl/targets/pim.xrls
index d0b8a05..16aa713 100644
--- a/xrl/targets/pim.xrls
+++ b/xrl/targets/pim.xrls
@@ -385,7 +385,7 @@ finder://pim/pim/0.1/start_cli
finder://pim/pim/0.1/stop_cli
/**
- * Enable/disable/start/stop BSR.
+ * Enable/disable/start/stop/restart BSR.
*
* @param enable if true, then enable the BSR, otherwise disable it.
*/
@@ -395,6 +395,8 @@ finder://pim/pim/0.1/start_bsr
finder://pim/pim/0.1/stop_bsr
+finder://pim/pim/0.1/restart_bsr
+
/**
* Add/delete scope zone.
*
diff --git a/xrl/targets/pim_base.cc b/xrl/targets/pim_base.cc
index 5be5b30..58ed0be 100644
--- a/xrl/targets/pim_base.cc
+++ b/xrl/targets/pim_base.cc
@@ -1343,6 +1343,30 @@ XrlPimTargetBase::handle_pim_0_1_stop_bsr(const XrlArgs& xa_inputs, XrlArgs* /*
}
const XrlCmdError
+XrlPimTargetBase::handle_pim_0_1_restart_bsr(const XrlArgs& xa_inputs, XrlArgs* /* pxa_outputs */)
+{
+ if (xa_inputs.size() != 0) {
+ XLOG_ERROR("Wrong number of arguments (%u != %u) handling %s",
+ XORP_UINT_CAST(0), XORP_UINT_CAST(xa_inputs.size()), "pim/0.1/restart_bsr");
+ return XrlCmdError::BAD_ARGS();
+ }
+
+ /* Return value declarations */
+ try {
+ XrlCmdError e = pim_0_1_restart_bsr();
+ if (e != XrlCmdError::OKAY()) {
+ XLOG_WARNING("Handling method for %s failed: %s",
+ "pim/0.1/restart_bsr", e.str().c_str());
+ return e;
+ }
+ } catch (const XrlArgs::BadArgs& e) {
+ XLOG_ERROR("Error decoding the arguments: %s", e.str().c_str());
+ return XrlCmdError::BAD_ARGS(e.str());
+ }
+ return XrlCmdError::OKAY();
+}
+
+const XrlCmdError
XrlPimTargetBase::handle_pim_0_1_add_config_scope_zone_by_vif_name4(const XrlArgs& xa_inputs, XrlArgs* /* pxa_outputs */)
{
if (xa_inputs.size() != 2) {
@@ -8766,6 +8790,10 @@ XrlPimTargetBase::add_handlers()
callback(this, &XrlPimTargetBase::handle_pim_0_1_stop_bsr)) == false) {
XLOG_ERROR("Failed to xrl handler finder://%s/%s", "pim", "pim/0.1/stop_bsr");
}
+ if (_cmds->add_handler("pim/0.1/restart_bsr",
+ callback(this, &XrlPimTargetBase::handle_pim_0_1_restart_bsr)) == false) {
+ XLOG_ERROR("Failed to xrl handler finder://%s/%s", "pim", "pim/0.1/restart_bsr");
+ }
if (_cmds->add_handler("pim/0.1/add_config_scope_zone_by_vif_name4",
callback(this, &XrlPimTargetBase::handle_pim_0_1_add_config_scope_zone_by_vif_name4)) == false) {
XLOG_ERROR("Failed to xrl handler finder://%s/%s", "pim", "pim/0.1/add_config_scope_zone_by_vif_name4");
@@ -9644,6 +9672,7 @@ XrlPimTargetBase::remove_handlers()
_cmds->remove_handler("pim/0.1/enable_bsr");
_cmds->remove_handler("pim/0.1/start_bsr");
_cmds->remove_handler("pim/0.1/stop_bsr");
+ _cmds->remove_handler("pim/0.1/restart_bsr");
_cmds->remove_handler("pim/0.1/add_config_scope_zone_by_vif_name4");
_cmds->remove_handler("pim/0.1/add_config_scope_zone_by_vif_name6");
_cmds->remove_handler("pim/0.1/add_config_scope_zone_by_vif_addr4");
diff --git a/xrl/targets/pim_base.hh b/xrl/targets/pim_base.hh
index 646bc90..20bc4bf 100644
--- a/xrl/targets/pim_base.hh
+++ b/xrl/targets/pim_base.hh
@@ -690,7 +690,7 @@ protected:
/**
* Pure-virtual function that needs to be implemented to:
*
- * Enable/disable/start/stop BSR.
+ * Enable/disable/start/stop/restart BSR.
*
* @param enable if true, then enable the BSR, otherwise disable it.
*/
@@ -702,6 +702,8 @@ protected:
virtual XrlCmdError pim_0_1_stop_bsr() = 0;
+ virtual XrlCmdError pim_0_1_restart_bsr() = 0;
+
/**
* Pure-virtual function that needs to be implemented to:
*
@@ -2174,6 +2176,8 @@ private:
const XrlCmdError handle_pim_0_1_stop_bsr(const XrlArgs& in, XrlArgs* out);
+ const XrlCmdError handle_pim_0_1_restart_bsr(const XrlArgs& in, XrlArgs* out);
+
const XrlCmdError handle_pim_0_1_add_config_scope_zone_by_vif_name4(const XrlArgs& in, XrlArgs* out);
const XrlCmdError handle_pim_0_1_add_config_scope_zone_by_vif_name6(const XrlArgs& in, XrlArgs* out);
--
1.5.4.3
_______________________________________________
Xorp-hackers mailing list
[email protected]
http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers
