Second opinion on script

ChrisVanMeer · Thu Sep 08, 2016 8:27 am

Hi guys,

I made the script below, that is in production.
Basically what it does is check the routing table for routes in a given subnet, if no routes are available, make the vrrp-instance go to backup-state.

Could you guys look it over and see if there is any optimilization possible, or maybe something more effecient?

# Set route counter to zero before checking
:global routeCounter 0

# Specify the BGP advertised subnet to check for
:global subnet "10.2.0.0/20"

# Specify the name of the VRRP interface to work with
:global vrrpInterface1 "vrrp-v4"
:global vrrpInterface2 "vrrp-v6"

# Specify the priority the $vrrpInterface should get once the BGP routes are gone
:global newPrio 50

# Loop through all routes and check for BGP routes with given subnet
:foreach route in=[/ip route find where dst-address in $subnet and bgp] do={
  # Increment the value of $routeCounter with every found route
  :set $routeCounter ($routeCounter+1)
}

# When $routeCounter is zero, that means that there are no BGP routes to given subnet
if ($routeCounter = 0) do={

  # Get the current VRRP priority of given VRRP interface
  :global curPrio [/interface vrrp get [find name=$vrrpInterface1] priority]

  # If the current VRRP priority is different then the $newPrio
  if ($curPrio != $newPrio) do={

  # Produce logging
    :log error "================ WARNING ================"
    :log error "No BGP routes to $subnet found."
    :log error "Setting $vrrpInterface1 priority from $curPrio to $newPrio"
    :log error "Setting $vrrpInterface2 priority from $curPrio to $newPrio"
    :log error "========================================="

    # Set the new priority of given VRRP interface to force a Master/Backup switch
    :put [/interface vrrp set [find name=$vrrpInterface1] priority=$newPrio]
    :put [/interface vrrp set [find name=$vrrpInterface2] priority=$newPrio]
  }
}

Deantwo · Thu Sep 08, 2016 4:29 pm

{
# Specify the BGP advertised subnet to check for
    :local subnet "10.2.0.0/20"
    
# Specify the name of the VRRP interface to work with
    :local vrrpInterface1 "vrrp-v4"
    :local vrrpInterface2 "vrrp-v6"
    
# Specify the priority the $vrrpInterface should get once the BGP routes are gone
    :local newPrio 50
    
# When $routeCounter is zero, that means that there are no BGP routes to given subnet
    :if ([:len [/ip route find disabled=no && dst-address in $subnet && bgp]] = 0) do={
        
# Get the current VRRP priority of given VRRP interface
        :local curPrio [/interface vrrp get [find name=$vrrpInterface1] priority]
        
# If the current VRRP priority is different then the $newPrio
        :if ($curPrio != $newPrio) do={
                
# Produce logging
            :log warning "================ WARNING ================"
            :log warning "No BGP routes to $subnet found."
            :log warning "Setting $vrrpInterface1 priority from $curPrio to $newPrio"
            :log warning "Setting $vrrpInterface2 priority from $curPrio to $newPrio"
            :log warning "========================================="
            
# Set the new priority of given VRRP interface to force a Master/Backup switch
            :put [/interface vrrp set [find name=$vrrpInterface1] priority=$newPrio]
            :put [/interface vrrp set [find name=$vrrpInterface2] priority=$newPrio]
        }
    }
}

Made a few small changes.

First of all I removed the foreach loop. It is much simpler to just to just check the length of the returned list.

[:len [find whatever]] = 0

I also made it exclude disabled routes.

Using local variables is also a good idea. Unless you actually need the specific variable in another script or the next time the script is run. If you use a global variable with the same name in two scripts at the same time, they could in theory change each other's variable if they run at the exact same time.

The ":if" commands were missing the colon.

Lastly, I like ":log warning" better than error. There are a lot fewer logs that use the warning color, plus you actually wrote warning in your log, so it is not really an error, right?

ChrisVanMeer · Thu Sep 08, 2016 8:57 pm

Wow thanks, actually all those points make total sense.
Strange is, the if statement without the : did work.
I have changed the script accordingly and all is still working as expected.

Thanks a lot!

Second opinion on script

Second opinion on script

Re: Second opinion on script

Re: Second opinion on script