Ordering of modules

[jbemmel]

It looks like modules are currently processed in the order declared. For example, from routing sr-mpls-bgp example:

nodes:
  e1:
    device: csr
    module: [ sr, isis, bgp ]
    bgp:
      as: 65000

'sr' depends on 'isis', but this order of deployment is currently not enforced(?)

Also, there can be a case where 'sr' modifies 'bgp' configuration - there would seem to be a preferred execution order:

1. isis (or ospf)
2. bgp (first ebgp, then ibgp)
3. sr

(in my case I tried to configure a BGP shortcut for SR, which fails because BGP AS is not configured)

[ipspace]

Another can of worms ;)

I would love to keep the simple "modules are configured in the order they're listed" approach, same for config and plugin parameters. In most cases that works well, and even in the "SR with IS-IS" case the order of configuration does not matter.

Of course you might have a case where a module configuration depends on another module. We could add configure_after attribute to check ordering of modules. Whether we'd report an error or reorder them (with a warning) is another decision to be discussed.

Regardless, if you want to go for SR-with-BGP, we would need a Python module for SR that would set dependencies based on configured parameters in its initialization phase because it doesn't make sense to make SR dependent on BGP if in reality it runs perfectly fine without it as long as you don't use BGP-related SR features.

[jbemmel]

My suggestion would be to make the order more deterministic, as follows:

1. If module B depends on module A, always configure A before B (and vice versa)
2. If there is no direct dependency, configure the module with less dependencies first
3. If they have the same number of dependencies, configure in the order as requested by the user

I solved my "bgp before sr" issue by including the sr-related config as part of the bgp module ("if 'sr' in module { ... }")

[ipspace]

If module B depends on module A, always configure A before B (and vice versa)

Makes sense. In all other cases (including #2) I'll keep the order as requested by the user. That will result in minimum number of rearrangements (and thus minimal amount of surprise ;).

[ipspace]

Implemented in 5dd57b1. Turns out it was easier to do reordering along the lines of the original idea by @jbemmel , so sr, isis, bgp turns into isis, bgp, sr and not into minimally-reordered isis, sr, bgp.

[jbemmel]

I was thinking something like this (untested):

def sort_module_list(mods: list, mod_params: Box) -> list:

    def mod_sort(m1,m2) -> int:
       requires_m1 = mod_params[m1].get('requires',[])
       if m2 in requires_m1:
           return -1
       requires_m2 = mod_params[m2].get('requires',[])
       if m1 in requires_m2:
           return +1
       return len(requires_m1)-len(requires_m2) # or 0 if you prefer to keep user's order

    from functools import cmp_to_key
    return mods.sort( key=cmp_to_key(mod_sort) )

[ipspace]

Thanks for the idea. It doesn't work because we're dealing with a topological sort not a linear sort.

Looks like I intuitively wrote a variation of Kahn's algorithm ;)

[jbemmel]

Right, it could be made to work but it would have to impose a total ordering. Instead of returning 0, it should always return +1 or -1. And the tie breaker could be the user’s provided order, i.e. index(m1) - index(m2) in the provided list

…

Sent from my iPhone

On Dec 25, 2021, at 2:01 AM, Ivan Pepelnjak ***@***.***> wrote:  Thanks for the idea. It doesn't work because we're dealing with a topological sort not a linear sort. Looks like I intuitively wrote a variation of Kahn's algorithm ;) — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

[ipspace]

Right, it could be made to work but it would have to impose a total ordering. That’s the problem — some items are not comparable. Imagine the “sr, bgp, isis” order and a simple swap sort. Comparison #1: sr vs bgp. No need to reorder, sr is not dependent on BGP Comparison #2: bgp vs isis. No need to reorder. End result: “sr, bgp, isis”. Tested with real code :( The tie breaker could have been the number of dependencies. That would probably work. Instead of returning 0, it should always return +1 or -1. And the tie breaker could be the user’s provided order, i.e. index(m1) - index(m2) in the provided list

Sent from my iPhone

On Dec 25, 2021, at 2:01 AM, Ivan Pepelnjak ***@***.***> wrote:    Thanks for the idea. It doesn't work because we're dealing with a topological sort not a linear sort.   Looks like I intuitively wrote a variation of Kahn's algorithm ;)   — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.  You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.  You are receiving this because you commented.

[ipspace]

Implemented in 5dd57b1