Allow NApp-to-NApp func calls with a minimal lightweight structure

[viniarck]

Goal: This is to remove the request loop back request and its complexity in favor of allowing function calls between NApps as mentioned on this issue discussion.

See #462 (comment)

Requirements on kytos core

• Add a new method get_napp_instance(username: str, name: str) -> KytosNApp on Controller
  • It should return the KytosNApp instance from Controller's self.napps
  • It must raise a custom exception if a NApp isn't installed
  • It must raise a custom exception if a NApp isn't enabled
  • This method docstring should also briefly document how its expected to be used: although the NApp Thread instance is returned, only managers/ from that NApp is supposed to be used, we won't have callable module inspections during runtime, we'll keep it light and follow the Zen of Python. Each NApp managers/ and controllers/ will be reused by the owning NApp and its API and events, and other client NApps are now encouraged to call managers/*.

Requirements on a NApp

• NApps which use a DB collection should have its DB controller on controllers/*
• Only managers/* are expected to be called from other NApps to be common entry points, just so business logic, validations (reuse them from OpenAPI schema), custom exceptions and the inner controllers can be used from there, including eventually to have granular internal concurrency control parametrizing other collection locks and so on, and API routes and event handlers as the server NApp can reuse the managers and map the custom exceptions to API exceptions or event handler errors. Bottom line: We'd be forcing one layer of indirection on demand with managers/ to lightly separate the concerns for reusability and code reorganization. A manager doesn't have to support every CRUD operation out of the gate, but as those are needed by other NApps it can be done gradually, and then it gets refactored in the server NApp to be reused in API endpoints and events too.
• Payload validations: if the payload needs to validated it's expected to be reused from openapi schema and their expected endpoints routes. This can be done with openapi-schema-validator which is already a sub-dependency of openapi-core https://openapi-schema-validator.readthedocs.io/en/latest/references.html. We don't want double validations like validating in the API and then the manager validating the payload again, so the manager methods allow it to pass a validate=callable, which should be preset with schema=schema, and registry=registry and then the manager would call validate() in a try except, that way it can be used easily from either a direct func call, or from the main.py API endpoint.
• NApps requirements should include the NApp installable too (and we should ideally avoid circular NApp dependencies)
• The client NApp is also expected to create a method such as get_napp_<napp>_<user>() where it should get the controller.get_napp_instance('user', 'napp'), and then when the NApp instance is needed it would call this method, this is to make it easily to mock the other NApp managers side effects. This is a similar pattern that we use when mocking DB controller methods.

Currently as of Jan 2025, controllers only have blocking IO methods. As an async ones are introduced in the future, the manager is also expected to expose/map the async corresponding method too.

This manager validation with validate parametrized is also nice since event handlers payload validation can finally start to benefit from it too gradually, this has been lacking for many years in certain event handlers, however since they are generated from trusted force and it's rare to have an invalid payload it's not a big problem

For more information about code module organization and their responsibilities (this will be augmented to describe the prior paragraphs about how other NApps can call other NApps too):

https://kytos-ng.github.io/napps/mongodb.html#napps-modules

[Ktmi]

Hmm, one thing I'd like to see added to this is context which can share share information about the call, such as a DB transaction, as well as providing the ability to schedule things to occur once the original call completes. For example, I'd like to do something like the following:

class PathDeploymentManager:
    """
    Manager for deploying paths.
    """
    def deploy_static_path(self, context, path_spec):
        session = context.get_session()
        vlan_reservations =  self.get_vlan_reservations(context, path_spec)
        flows = self.generate_flows(context, path_spec["cookie"], vlan_reservations)
        context.call("FlowManager", "install_flows", flows)
        deployed_path = {
            "path_spec": path_spec,
            "reserved_vlans": reserved_vlans,
        }
        db.deployed_paths.insert_one(deployed_path, session = session)
        context.call_on_success("EventManager", "push", "path_installed", deployed_path)

    def get_vlan_reservations(self, context, path_spec):
        vlan_reservations = {}
        for link in path_spec["links"]:
            vlan = context.call("LinkManager", "reserve_vlan", link)
            vlan_reservations[link] = vlan
        return vlan_reservations

    def generate_flows(self, context, cookie, vlan_reservations):
        ...

[viniarck]

@Ktmi yes good feedback to explicitly cover, we'll cover that, I'll reply in new comments below to facilitate following up since I'll break it down the answer in multiple parts

[viniarck]

I'd like to see added to this is context which can share share information about the call, such as a DB transaction,

Yes, we should pass either a **kwargs or context: dict to allow to chain down a DB transaction and to also be lightly extensible for future needs.

Nevertheless, to keep in mind:

ClientSession instances are not thread-safe or fork-safe. They can only be used by one thread or process at a time. A single ClientSession cannot be used to run multiple operations concurrently.

Although we'll have cases where it'll be beneficial, let's only use MongoDB transactions sparingly at the application level where it's absolutely needed, and then expose a global lock to be passed in the kwargs/context too. MongoDB tends to punish when requiring too much transactions, and there are certain production constraints to be aware, certain times that's unavoidable and reasonable to use. Ultimately, it's up to us to also review from time to time to design and maintain our collections and models to where MongoDB is strong and shines like individual documents (with embedding) and bulk ops in same collection to leverage its engine parallelism execution too.

In the short-term we should keep a similar pattern in terms of collections writes and reads. But, as we manage to successfully plan carefully and offload to the DB certain resources when it makes sense and when it's safe to do so, then gradually we can se how much else can be deffered and then for certain operations try to also use transactions, for instance to ensure the topology interface vlans, but again, we always gotta find a balance and be conservative about it, ultimately, MongoDB isn't a relational database at its core, it has bolted on multi docs ACID relational capabilities, but gotta find a balance.

[viniarck]

as well as providing the ability to schedule things to occur once the original call completes

Any sort sort of pub sub notification should use what we currently have with events.

Now, regarding when it completes, we'll get this "for free" in the composability since it'll be just a direct callable, NApps will be able to call the instance Manager* methods, so whatever it returns it'll be available either it's a def f1() or a async def f2():

class PathManager:
    """
    This Manager doesn't exist yet, needs to be implemented
    """

    def __init__(self):
        ...

    def find_paths(self, *args, **kwargs):
        ...

class FlowManager:
    """
    This Manager doesn't exist yet, needs to be implemented
    """

    def __init__(self):
      ...

    def add_flows(self, payload: dict, validate=True, **kwargs):
      validate and do_validate(payload, schema, registry=registry)
      db_session = kwargs.get('ctx_db_session')
      ...
      return self._install_flows(
          "add",
          flows_dict,
          switches,
          reraise_conn=not force,
          session=db_session,
      )

    def add_flows_by_switch(self, payload: dict, validate=True, **kwargs):
      validate and do_validate(payload, schema, registry=registry)
      db_session = kwargs.get('ctx_db_session')
      ...
      return self._install_flows(
          "add",
          flows_dict,
          switches,
          reraise_conn=not force,
          session=db_session,
      )

Usage on mef_eline for instance:

pfndr = self.get_napp_pathfinder() # wrapping `self.controller.get_napp_instance("kytos", "pathfinder")` for testability
fmngr = self.get_napp_flow_manager()

_ = pfndr.path_manager.find_paths(payload)
_ = fmngr.flow_manager.add_flows_by_switch(payload)

Usage on mef_eline for instance when DB transaction is needed (in the example it's not actually needed, but just for the sake of it, the async def version would be the same, except with async defs and async locks when we also have an async DB client):

pfndr = self.get_napp_pathfinder()
fmngr = self.get_napp_flow_manager()

with self.controller.get_db_lock():
  with self.db_client.start_session() as session:
      with session.start_transaction():
        _ = pfndr.path_manager.find_paths(payload)
        _ = fmngr.flow_manager.install_flows(payload, session=session)

What do you think? Let me know if you have anything else in mind in terms of production usage that hasn't been covered yet. Essentially, it's just extracting a layer, and every manager should be composable and ensuring concurrency safety. Any DB controller or manager that doesn't expose a functionality, then we keep as we've always been augmenting incrementally as needed.

[Ktmi]

Not exactly what I meant. What I in reality want is some exit stack shared by the entire call chain. In the case something interrupts an operation, I want each NApp to be able to provide specific instructions on how to undo their changes. Certain problems with consistency can only really be solved within Kytos, and I think this is an important tool, more important than transactions.

Consider the case of trying to reserve vlans for an EVC. Either all the vlans in the path are reserved or none. Transactions adhere to ACID Isolation, so you need a mechanism outside the DB to negotiate which vlan is assigned when multiple reservations are happening simultaneously. That in turn requires locks, and a local cache of the expected DB state. Now what happens if something interrupts this process, for example, one of your interfaces has run out of vlans to assign? If you are in a transaction, the DB will return to a consistent state, however, the local cache will be out of sync with the remote. That means we would need to start setting the state back into proper order on the local side.

For returning the local state, we can either have EVC manager try to return all the vlans. Or the vlan manager can push onto the exit stack a function for returning the vlans when an error occurs. Doing this with the exit stack doesn't make these mutually exclusive options. What do you think?

[viniarck]

That in turn requires locks, and a local cache of the expected DB state. Now what happens if something interrupts this process, for example, one of your interfaces has run out of vlans to assign?

yes, vlans path allocation has to be all or nothing, if it fails, the encapsulating should undo since at that point all the args.

The returning value should be what's best for testability, if encapsulating methods and inner methods are cleaning them up them it's even nicer. Now think about two cases 1) and 2) below:

1. Single EVC:

• One path, with 5 links
  • Looping over allocating, if at second link it can't allocate, then this choose_vlans method, takes care of making available the pre-allocation, and raise the exc to let the calller know that it couldn't do it but the vlan resources were managed correctly, and the controller methods lock the individual resource.

1.1) Single EVC (simulating another late failure but when pushing flows):

• One path, with 5 links
  • Looping over allocating, it suceeds
  • But fails when pushing flows, then it "undo" here by removing the path https://github.com/kytos-ng/mef_eline/blob/master/models/evc.py#L959, which will release the vlans too.
  • If this last removal fails (let's say http server overloaded), or in the extreme case DB isn't writable, at least in the runtime the state should indicate that and then it can also auto recover by trying periodic redeploys or eventually a network operator reploys.

Note: we do have an existing bug on the choose_vlans method kytos-ng/mef_eline#620, we need to improve this part

2. Multiple bulk EVCs (that we don't have yet):

• If it fails at the third EVC due to a resource that's been exausted or not available
  • It undeploys all related EVCs (first and second) if any allocation fails, assuming all of nothing when creating bulk EVCs one day.
    • and this also would be the undo part of the EVCs iterable
      • Now if the undeploy of an EVC also fail, of even the DB write isn't available due to partial cluster outage, then the runtime at least should keep the EVC in deactivated state in an errored out state, and eventually it tries to auto recover periodically, or an operator try to redeploy later

Doing this with the exit stack doesn't make these mutually exclusive options. What do you think?

I agree with you, we need to have all or nothing for vlan allocations over a path, I believe the allocations that work on an iterable should do the context management properly to simplify for callers. The simplest and easiest to maintain that's solid the better, what I want us to avoid and having to bubble up too much specific resources that could've been freed/released by the responsible provisioning method that's being called, but still raising the exception for a nice error and specific clear error message. For vlans, I don't think we're far away to have proper clean up if we get the encapsulation right.

Let me know if you have another example where this isn't sufficient and we'll get it covered too. Let's make this dead simple to maintain while still coherent.

I want each NApp to be able to provide specific instructions on how to undo their changes

That's a very broad concept, and that can also blur the responsibilities who should be freeing what (or even have to have multiple attempts at freeing/releasing resources), which can also put a lot of extra code in the top caller to have to loop over so many related resources but if we get the encapsulation and allocations right at the methods which are allocating, kinda following RAII-ish at the method level, and before bailing out of loops clean up the rest locally we can nail it, and then the top level only undo what it has done successfully mid iteration on what it manages at a higher level and knows how to undo by undeploying or deleting depending on what's desired.

For returning the local state, we can either have EVC manager try to return all the vlans.

Methods which are returning None, agreed that it's worth returning the vlans, but goes to that prior point, if we find ourselves bubbling up to much lower level allocated dependent resources over multiple calls, that can turn into a slippery slope. mef_eline either finds a new path and allocate all the vlans before pushing the flows or nothing, and similarly if it fails to push the flows, it should release the vlans too (which it's expected to do, except for issue 620), structuring that encapsulation correctly can simplify for us.