epic_mef_eline_consistency_v2 Scope Refinement

[Ktmi]

There are a few processes we would like to rework in regards to consistency check:

• EVCs modified without holding lock, may lead to race conditions #512 - Protect EVCs during the consistency check process. It's possible that an EVC may be modified during consistency check, as the EVC lock isn't maintained throughout the process.

• Replace consistency check with routine for checking traces #437 - Perform traces on all deployed (active) EVCs. Currently traces are only performed on inactive EVCs to check if they are deployed, in order to set them to active. However with the current implementation of mef_eline, all deployed EVCs are set as active, and maintain the active state between restarts.

• consistency: redeploy EVCs that are enabled and inactive and now have paths to be deployed #395 - Periodically try to deploy EVCs that are inactive. There could be cases where a path is available for an EVC, but the EVC wasn't deployed due to some unexpected factor.

• Consistency check: remove orphan flows from an EVC #444 - Clearing orphan flows. EVCs may not have properly deleted flows from a previous path.

• Optimize consistency check routine to avoid trying to activate EVCs unnecessarily  #247 - Prevent EVCs from trying to redeploy too often. This is mainly a constraint on trying to deploy inactive EVCs.

• Annoying log messages about fail to establish a failover path every 60s #488 - Reduce annoying log messages about problems that operators can not address.

Of these issues, the main problematic one I'm seeing is #444. Identifying orphan flows is a non trivial issue, being that we must identify flows pertaining to the EVC, that we might not know if they exist, or where they may exist.

Edit: New issue just dropped

• Multiple link downs can lead to race condition with old_path, resulting in old_path not being properly cleared. #517 - Multiple link down events can cause a race condition in regards to handling old_path. This can cause the old paths to not be cleared, and leave there flows hanging.

[Alopalao]

Reviewing mef_eline, it already takes action from changes in the topology and modifies EVCs accordingly. These changes could be link up, link down, maintenance, redeployments, and more. The cases where we need a consistent check are (correct if I am wrong):

1. Check that flows are correct. These could be created from old_path not deleting due to kytos restart and path creation malfunctions not detected.
2. Detect EVCs not deployed that missed a link_up. It reminds me of issue bug: after shutting down failover upstream interface + upstream current_path interface EVC remained active #503 but with link_down.
3. Check EVCs correctness from any change while kytos was down.

What if we handle these cases in three different consistency checks:

1. Consistency for flows: Get flows with a time so we can discard every EVC that has updated after getting the flows. Acquire the lock for every EVC.
2. Consistency check for link changes: Do not trigger when a link_down or link_up has not happened for a while.
3. Consistency start up: Trigger at kytos start and add a request so that the user can trigger this too.

Some enhancements to mef_eline that I think can help with the development of consistency check but that are not necessary:

• Separate sync: There are EVC attributes that do not affect if they are modified under a lock like name and owner. These also could not affect evc.updated_at.
• Acquire lock for any modification of an EVC only the modified attribute requires redeployment. This is related to the previous enhancement.
• Enforce evc.updated_at to be used when changes occur in attributes that require redeployment. For example here does not update the timer and the flows for this failover_path should be deleted.
• Add a lock for consistency checks so they do not run simultaneously.

These are wild ideas discard them all if you see best.

[Ktmi]

In terms of what I think is doable from the issues listed, #437, #395, and #488 seem immediately doable. These three are fairly simple changes, all things considered. I even written a potential implementation for it already:

    def execute_consistency(self):
       """Execute consistency routine."""
       # Stages:
       # 1. For all enabled and not deployed circuits, try to deploy them
       # 2. For all deployed circuits, make sure they have a failover path ready.
       # 3. For all deployed circuits, check their traces are good, and if not, redeploy them.
       circuits_to_trace = []
       circuits_to_deploy = []
       with contextlib.ExitStack() as stack:
           for circuit in self.get_evcs_by_svc_level(enable_filter=False):
               # If a circuit is active, then it has an implementation
               if circuit.lock.acquire(blocking=False):
                   if circuit.is_recent_updated() or circuit.has_recent_removed_flow():
                       circuit.lock.release()
                       continue
                   stack.push(circuit.lock)
                   circuit.try_setup_failover_path()
                   if circuit.is_active():
                       circuits_to_trace.append(circuit)
                   elif circuit.is_enabled():
                       circuits_to_deploy.append(circuit)

           # Passively try to deploy circuits that are enabled, but deactivated
           for circuit in circuits_to_deploy:
               circuit.deploy()

           # Check the trace of active circuits
           # Maybe we should check enabled but deactivated circuits as well? They could have
           circuits_checked = EVCDeploy.check_list_traces(circuits_to_trace)
           for circuit in circuits_to_trace:
               is_checked = circuits_checked.get(circuit.id)
               if is_checked:
                   log.info(f"{circuit} enabled, active, and trace good!")
                   circuit.execution_rounds = 0
               else:
                   circuit.execution_rounds += 1
                   log.info(f"{circuit} enabled, active, but trace failed {circuit.execution_rounds} times.")
                   if circuit.execution_rounds > settings.WAIT_FOR_OLD_PATH:
                       log.info(f"{circuit} enabled, active, but trace failed too many times - redeploying")
                       circuit.deploy()

However, I don't think doing them as soon as possible is the best decision. I think instead the effort should first be focused on resolving race conditions within mef_eline, from issues such as #517 and #512. In addition I think #517 and #444 are going to require the biggest changes to mef_eline in order to properly implement.

[Ktmi]

In order to implement #517 and #444, we need a way to keep track of past paths. There are few things we can do to help with this:

1. Every path deployment for an EVC should have a unique cookie. This makes it easier to delete flows on switches we remember affecting.
2. Store deployed paths separate from the EVC, and have the EVC keep references to the current and failover path. That way we can keep a history of what paths have been deployed, know which paths should remain, and know which paths should be cleaned up.

[Alopalao]

These changes will help to compare flows to paths instead of creating flows from paths and comparing field/values. However having paths accessible this way could lead to new race conditions, which can complicate maintenance.
What if we use part of the cookie itself and create id's for the paths inside the EVC. Currently we have 16 hex characters, the first two are 0xAA and the rest is the EVC id. If we divide the second part like 7/7 hex characters so we also have the path ID as well. For example:

• Current:

   "aa"      +      "69fe7a649dce4b"
  prefix                EVC ID

• New cookie from evc.id = uuid4().hex[:7] and evc.path.id = uuid4().hex[:7]:

    "aa"      +        "69fe7a6"         +       "49dce4b"
   prefix               EVC ID                    path ID

Having the EVC ID reduced to just 7 hex characters reduces its of different values to 268435455. This number looks enough for our cases. Or maybe moving the space as 10/3. It should not be an issue if we have same path ID in different EVCs.
Update:
If the repeated IDs for EVC becomes a concern, we can check for existing IDs with the dictionary of circuits in mef_eline and not let EVC create its ID blindingly.

[Alopalao]

Now, thinking more, maybe just using 1 hex character for the path is enough since there are two possibilities, current and failover.
prefix(2)+evc_id(13)+path(1)

[Ktmi]

@Alopalao thanks for the feedback.

These changes will help to compare flows to paths instead of creating flows from paths and comparing field/values. However having paths accessible this way could lead to new race conditions, which can complicate maintenance.

What kinda race conditions are we thinking can happen here? If we move checks and updates to pure DB operations, or mostly pure DB operations, we should be able to considerably reduce race conditions. For example, what we could do for checking if a link down affects a path is this:

path_collection.update_many(
    {"link_dependencies": down_link, "state": "deployed"},
    {"$set": {"state": "invalidated"}
)
invalidated_paths = path_collection.find({"state": {"$ne": "deployed"}})
invalidated_path_ids = [path['id'] for path in invalidated_paths]
# This following part could probably be done better, with some conditional updating based updating and an aggregation pipeline to reduce it to a single call.
evc_collection.update_many(
    {"current_path": {"$in": invalidated_path_ids}},
    {"$rename": {"failover_path": "current_path"}}
)
evc_collection.update_many(
    {"current_path": {"$in": invalidated_path_ids}},
    {"$unset": {"current_path": ""}}
)

Which removes the need for a lock for every path, and then with a single handler thread for the old_paths, we can delete the flows and mark them as cleared.

What if we use part of the cookie itself and create id's for the paths inside the EVC. Currently we have 16 hex characters, the first two are 0xAA and the rest is the EVC id. If we divide the second part like 7/7 hex characters so we also have the path ID as well. For example:

* Current:

   "aa"      +      "69fe7a649dce4b"
  prefix                EVC ID

* New cookie from `evc.id = uuid4().hex[:7]` and `evc.path.id = uuid4().hex[:7]`:

    "aa"      +        "69fe7a6"         +       "49dce4b"
   prefix               EVC ID                    path ID

Having the EVC ID reduced to just 7 hex characters reduces its of different values to 268435455. This number looks enough for our cases. Or maybe moving the space as 10/3. It should not be an issue if we have same path ID in different EVCs. Update: If the repeated IDs for EVC becomes a concern, we can check for existing IDs with the dictionary of circuits in mef_eline and not let EVC create its ID blindingly.

This is pretty close to the idea I had regarding giving each path of the EVC a unique cookie. However it alone isn't sufficent for deleting extra paths, primarily due to the lack of ability to match all the old_path flows, while ignoring the current_path and failover_path flows.

[Ktmi]

This is mainly a comment on the changes to paths that I have discussed, and my plans on how we may implement it.

There are a few goals with these changes:

1. Never put the system into a state where something important can be forgotten, such as what happens with Multiple link downs can lead to race condition with old_path, resulting in old_path not being properly cleared. #517.
2. Make it easier to differentiate flows for different paths on the same EVC, this is to help resolve Consistency check: remove orphan flows from an EVC #444.
3. Prevent overlapping changes to EVC state that may lead to incomplete actions getting sync'd to the DB like with EVCs modified without holding lock, may lead to race conditions #512, unless there is a clear method for undoing the incomplete changes.

To achieve these changes, I propose a few things:

1. Persistent data about deployed paths needs stricter lifetime guarantees, up until we can certain that the flows for that path is removed from stored flows. To do so, we should maintain persistence of path data separate from the EVC.
2. Give each path a unique cookie value, so that paths can be torn down individually, instead of as a component of the EVC. They can still be a subcookie of the EVC, but it should remain unique from path to path.
3. Move more of the logic to the DB, reducing cases of overlapping/incomplete changes.

These changes will affect how telemetry_int interacts with mef_eline. Instead of listening for when the EVC deploys/redeploy/uninstall a path, it instead will have to listen for when a path that associates itself with the EVC is deployed or removed. The main change with this is that telemetry_int will have to be aware of the cookie of each individual path when modifying them.

Going forward I'm going to be working on two things, outlining new/updated DB models, and prototyping moving logic to the DB. From there we can get a clearer picture of what the path ahead looks like.

[Ktmi]

Created a few issues for the changes:

• Move logic for updating EVCs to DB #534
• Separate persistent data on path deployments from EVCs to ensure flows needing to be removed aren't forgotten #535
• Make each path deployment use a unique cookie #536

[jab1982]

Thanks @Ktmi for the detailed documentation and new issues. @italovalcy @viniarck @Alopalao Do you see any conflicts with the suggestions above?

[viniarck]

Great ideas and lots of good stuff here. Two main points to be addressed, I'll keep it short (in the next two threaded comments):

[viniarck]

1. Regarding race conditions and data model changes:

The proposed change to fully decouple paths from EVCs is a massive data model change, although it's a good idea and has potential for other path reusabilibility optimizations in the future, and it's great to also have path status about its flow installation|removal state.

For 2024.2 version, I'll recommend that you try to find and propose a more incremental and specific solution(s) that's still close to the current data model while trying to minimize significant changes to ensure the existing code stability and maintenance, while still ensuring that converge still be on par with current performance, and laser focus on ensuring that old_path doesn't get overwritten/lost (and consider storing it) or flows get leaked when handling link down. And if there's any other potential leak, let's map and see how to solve it with consistency check or after a controller restart.

[viniarck]

Quick comment: handle_link_down handler is single threaded, would it help if handle_cleanup_evcs_old_path were to be executed on handle_link_down before it finishes? handle_cleanup_evcs_old_path used to make requests for each EVC that's why it got offloaded to event based to keep that tight hot path fast in terms of other IO dependencies. On version 2024.1 the deletions are event based via flow manager, so executing on handle_link_down shouldn't be a problem in terms of poor IO performance. And if a stronger gurantee is needed that the actual flow removals get executed before handling the next event then you can potentially use the new flow_manager bulk deletion endpoint that supports in a single request n switches with different flows that @Alopalao is developing on kytos-ng/flow_manager#205. Anyway, something to think about, see if it helps or try to solve the old_path issue and races in a simpler way that requires less siginificant refactoring.

[viniarck]

2. Regarding the consistency check

Still needs to be specified how the active consistency check will ensure no false positives and if it'll be able to run sdntrace_cp in bulk. Will it perform well with prod-like AmLight scalability 400 ish EVCs? And, what mef_eline should do when a trace fails? Try to redeploy? and keep trying to redeploy for how many attempts?

It's also important to discuss if the consistency will only rely on sdntrace_cp or if checking flows might also needed (so let's ensure we don't have room for other flows leaks, and if there is let's discuss which cases, and try our best not to have to check individual flows avoiding redundant consistencies, considering that flow_manager already performs flows consistency checks), if we can only rely on sdntrace_cp even better and less effort/cost to maintain. sdntrace_cp will only cover the current path but if that's good enough and reliable then it'll already be sufficient to never allow the application stays in a inconsistent black hole-ish state (like switching to a broken path and staying there)

[viniarck]

Adjacent comment: With 400 EVCs with INT with a single bulk request, this issue has been hit sometime ago kytos-ng/sdntrace_cp#120. Depending on how frequent and (if and) for how long EVCs locks are held, the few seconds latency that can add up and result in significant slowdowns in certain cases.