[GCU] Turning port admin down before some critical port changes

[ghooo]

What I did

Fixes #1930

Issue can be summarized into these 2 case:

• A patch can contain an operation to turn port admin up -- do at the end
• A patch that modifies config that needs the port to be down -- start by bringing the port down then do the update then bring it back up

How I did it

The configs that need the port to be down, we will call them critical-port configs.

• Added a move validator to validate critical-port configs, which does the following

  • If a port is up, make sure the move does not make changes to related critical-port configs
  • If the move is turning a port up, make sure there is no critical-port config are still left in the patch

• Added a move extender to critical-port changes:

  • If a port is up, bring down if there are critical-port changes
  • If the move is turning a port up, flip it to down if there are still critical-port configs left. In other words, do not turn the port back up until all critical-port changes are in done.

How to verify it

• Added AddRack unit-test to tests/generic_config_updater/files/patch_sorter_test_success.json
• Other unit-tests

Previous command output (if the output of a command-line utility has changed)

Check issue #1930 for old sorting order

New command output (if the output of a command-line utility has changed)

Check AddRack unit-test to tests/generic_config_updater/files/patch_sorter_test_success.json for new sorting-order

[qiluo-msft]

identify_admin_up_ports

def identify_admin_up_ports(self, config):

---

Could you try leverage JsonPath, which is a mature query language? For this function, you may query $.PORT.*[?(@.admin_status == 'up')].

ref: https://support.smartbear.com/alertsite/docs/monitors/api/endpoint/jsonpath.html
online evaluator
python lib: https://github.com/json-path/JsonPath#path-examples #Pending

[ghooo]

This is a JAVA lib, I think you meant: https://github.com/kennknowles/python-jsonpath-rw

[ghooo]

Check this reply

In a future PR, I can leverage JsonPath instead the Patterns. I can add get_values() to ConfigFilter class.

[qiluo-msft]

Future PR is good to me. Sorry for wrong library. I did not search for all the python libraries. python-jsonpath-rw seems a good candidate.

[ghooo]

#2039

[qiluo-msft]

PortCriticalMoveValidator

class PortCriticalMoveValidator:

---

I believe there will more cases other than port in future. The concept is similar to a lock. When a node is locked, the manipulation of the node subtree is banned.

Can we add a yang model extension to capture this behavior, and apply the extension to port as a first case? #Pending

[ghooo]

The idea of a locking a node is interesting. My thought was to name it TransitionalValueValidator later if we need to support more cases, and also move it to YANG. Added that as a TODO in line 390.

# TODO: port-critical fields are hard-coded for now, it should be moved to YANG models
#  NOTE: PortCritical validation logic can be generalized to be validation for any config that
#        requires another config to be of a specific value. Call the validator "TransitionalValueValidator"
#        For the case of port-critical validation the transitional value would be "admin_status=down".

Let me think about the lock idea.

[ghooo]

Sorry took me a bit to think about this, had other work items.

I think the idea of lock is similar to what we have today for the validation. If the validation fails we cannot do the update.

The concept is similar to a lock. When a node is locked, the manipulation of the node subtree is banned

I think this is the same as what we have for validation

if validation of a move that is changing a specific part of the config fails, the move is rejected.

We can also think about the lock in another way, let's think about the dependency between PORT and VLAN_MEMBER table

{
    "PORT": {
        "Ethernet0": {}
    },
    "VLAN_MEMBER": {
        "Vlan715|Ethernet0": {
            "tagging_mode": "untagged"
        }
    }
}

We cannot delete Ethernet0 unless we delete Vlan715|Ethernet0. hence the deletion cannot take place and the Ethernet0 deletion is locked. We already have that as a validation check which is FullConfigMoveValidator

We can also think about the lock for the CreateOnly configs, if a config is CreateOnly it cannot be modified hence it is modification locked. We have to delete the parent and add it back. Check CreateOnlyMoveValidator

I prefer another Validator maybed named RequiredTransitionalValueValidator or something over a lock. I think it will be confusing to have a lock and the validators at the same time. Thoughts?

[ghooo]

Updated to handle any config and named RequiredValueValidator

[qiluo-msft]

with minor comment in code comment.

[qiluo-msft]

There is a recent build fix on master branch. Could you rebase to latest master or merge it?

[ghooo]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).