Bug: Generic Config Updater - Race Condition

[bhouse-nexthop]

Is it platform specific

generic

Importance or Severity

High

Description of the bug

There is a race condition in Generic Config Updater between pushing configuration changes and consuming them.

Some configuration changes may depend on other changes, so they must be executed in order. Some will also generate "extended" changes when deemed necessary, so may not be obvious to an end user that such a change is ever occurring. For instance, a user might request a change like:

PORT_QOS_MAP|Ethernet0/pfc_enable = 1

But the PORT_QOS_MAP requires the port to be in an admin_status=down state, so if the port is up, the actual lists of changes automatically generated and performed looks like this:

PORT|Ethernet0/admin_status=down
PORT_QOS_MAP|Ethernet0/pfc_enable=1
PORT|Ethernet1/admin_status=up

The problem comes up because the change is written directly to the database (its not using the producertable/consumertable helpers for configdb ... but if it would, it would still have issues). That change is then detected because various daemons like portmgrd and buffermgrd are waiting on changes to their requested tables as per:

https://github.com/sonic-net/sonic-swss/blob/fe9b89e435d42c2140dbdcd422066de3ee15200b/orchagent/orch.cpp#L913

which does a simple psubscribe:

https://github.com/sonic-net/sonic-swss-common/blob/c82811f753b4b14c0818eda5d9f3cc924b7e24ae/common/subscriberstatetable.cpp#L24

Then when a key is detected as changed, it has to go query the database to get the new value as per:

https://github.com/sonic-net/sonic-swss-common/blob/c82811f753b4b14c0818eda5d9f3cc924b7e24ae/common/subscriberstatetable.cpp#L150

Because it has to query for a new value, its not something that is able to be seen as part of the subscription notification, we may never read the "down" state.

So in this case, there's no order guarantee that buffermgrd will actually read its update in-between those 2 updates to the port admin_status, and since you can't make QOS changes to a port that is in admin_status=up it will fail. Therefore this is a race condition that needs to be addressed.

NOTE: this is just one specific example, there are many other possibilities. We need to ensure changes are applied before moving to the next change.

This was found during development of #22254 and sonic-net/sonic-utilities#3831 .

There is a chunk of code here:

https://github.com/sonic-net/sonic-utilities/blob/ebb17ca86839635275698fff42c38f1575eac838/generic_config_updater/change_applier.py#L150-L155

which pulls the configdb for verification purposes after writing it. Since its holding a lock and mutating the data in a known state, its impossible for this to fail. This takes up a lot of CPU and adds an 0.5s-1.0s delay, likely hiding this race condition ... but if the control plane is under high load it could still be hit.

This can be seen by removing the aforementioned chunk of code and running through the sonic-mgmt test suite for generic_config_updater ... however the tests don't actually fail, but additional warnings are output in the logs. In my PRs listed above, this aforementioned chunk is replaced by time.sleep(1) rather than attempting to address the underlying cause of the issue. Plus there are other instances in GCU that use sleep to avoid race conditions so this appears to be an acceptable practice:

https://github.com/sonic-net/sonic-utilities/blob/ebb17ca86839635275698fff42c38f1575eac838/generic_config_updater/services_validator.py#L100

The proper solution would be to create a shadow table, similar to how ProducerTable/ConsumerTable currently do. The differences would be:

1. We would still want to write the data to config_db direct, the shadow table should just record the new op, key and value (if replace / add).
2. The shadow table should only be drained when consumption is complete. Currently the consumer table implementation drains it before they are processed which can itself be a race condition.
3. The producertable should wait until the shadow table is drained before continuing
4. Some masking would be required for tables that are only read at startup, e.g. dhcp-relay, as we wouldn't use the shadow tables for those.

Steps to Reproduce

See above

Actual Behavior and Expected Behavior

See above

Relevant log output

Output of show version, show techsupport

Attach files (if any)

No response

[prabhataravind]

@qiluo-msft to review the findings and do initial investigation.

[qiluo-msft]

@bhouse-nexthop Could you repro these steps with some plain config command, but not using config apply-patch?

I believe if human run some plain config command to manipulate ConfigDB in the same sequence, it will lead to exactly same swss behavior. Then we need to improve the plain config implementation to wait for some config consumed, then we can implement the same protective logic inside Generic Config Updater.

[bradh352]

Correct, if the 'config' command was executed very fast back to back it could in theory cause the same issue. That said, I haven't tried it. Its possible the additional overhead of python startup and teardown between commands, additional redis connections, etc, it might make it hard to reproduce.