Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[config]: Implement a process level lock #1590

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

praveen-li
Copy link

@praveen-li praveen-li commented May 4, 2021

  • What I did
    Implement a config process level lock.

  • How I did it
    Changes:
    1.) Implement a class, which uses hsetnx for lock.
    2.) lock is expired within timeout period or will be released by config python click process.
    3.) After -y prompt, lock is reacquired, because timer could have expired,
    before user enters yes.

Signed-off-by: Praveen Chaudhary pchaudhary@linkedin.com

Note: VS Test PR: I will raise it after confirmation###

Depends on: sonic-py-swsssdk PR which will be raised with below diff after confirmation

@@ -22,6 +22,7 @@
 from .dbconnector import SonicV2Connector
 
 PY3K = sys.version_info >= (3, 0)
+FILTER_TABLE = ['LOCK']
 
 class ConfigDBConnector(SonicV2Connector):
 
@@ -318,6 +319,8 @@
                 key = key.decode('utf-8')
             try:
                 (table_name, row) = key.split(self.TABLE_NAME_SEPARATOR, 1)
+                if table_name in FILTER_TABLE:
+                    continue
                 entry = self.__raw_to_typed(client.hgetall(key))
                 if entry != None:
                     data.setdefault(table_name, {})[self.deserialize_key(row)] = entry

Why lock is acquired at process level, not just when we interact with DB.

command such as port breakout or apply-patch interects with DB 3 or more times:
1.) Read Config DB,
---Processing---
2.) Delete Ports
---Processing---
3.) Add Ports.
Here we need to take the lock before step one and then have to release the lock after step three. If we take the lock only while interacting with DB then we have to take lock three times in above situation and that also doesn't solve the problem, because some process can update the DB in between Step 1 and step 2.

Similar command can be written in future with config validation in picture.

  • How to verify it

Testing: [With Debug Logs] [### Note: VS Test PR: ###]

Test 1: acquire a lock with command config. Run another config command in between to make sure lock is not acquired. Then say yes on the prompt of first command before timer expires.

Expectation: Second command should not acquire lock, first command should re-extend the timer.

Terminal 1:

admin@lnos-x1-a-fab01:~$ sudo config save
:::Lock Acquired:::
Existing file will be overwritten, continue? [y/N]: y
:::Lock Timer Extended:::
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json

Terminal 2:

admin@lnos-x1-a-fab01:~$ sudo config save
:::Can not acuire lock, Abort:::

Test 2: run first command, let the timer expires, run second command, then immediately say yes on the prompt of first command.

Expectation: Second command should acquire lock, first command should not be acquire lock after prompt.

Terminal 1:

admin@lnos-x1-a-fab01:~$ sudo config save
:::Lock Acquired:::
Existing file will be overwritten, continue? [y/N]: y
:::Can not acquire lock LOCK PID: 10903 and self.pid:10877:::
:::Lock PID: 10903 and self.pid:10877:::

Terminal 2:

admin@lnos-x1-a-fab01:~$ sudo config load
:::Lock Acquired:::
Load config from the file /etc/sonic/config_db.json? [y/N]: N
Aborted!

Test 3: run first command, let the timer expires, then say yes on the prompt of first command to reacquire the lock.

admin@lnos-x1-a-fab01:~$ sudo config save

:::Lock Acquired:::
Existing file will be overwritten, continue? [y/N]: y
:::Lock Reacquired:::
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json
admin@lnos-x1-a-fab01:~$
  • Previous command output (if the output of a command-line utility has changed)

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

@lgtm-com
Copy link

lgtm-com bot commented May 4, 2021

This pull request introduces 1 alert when merging 71c2a833e40604cfd6d4028d8ff1524f5d1a2bc6 into 9492eab - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

 Changes:
 1.) Implement a class, which uses hsetnx for lock.
 2.) lock is expired within timeout period or will be released by config python click process.
 3.) After -y prompt, lock is reacquired, because timer could have expired,
     before user enters yes.
@lgtm-com
Copy link

lgtm-com bot commented May 4, 2021

This pull request introduces 2 alerts when merging 63338bf into 9492eab - view on LGTM.com

new alerts:

  • 1 for Testing equality to None
  • 1 for Unused local variable

Signed-off-by: Praveen Chaudhary <pchaudhary@linkedin.com>
@lgtm-com
Copy link

lgtm-com bot commented May 5, 2021

This pull request introduces 1 alert when merging adda6d0 into 9492eab - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2021

This pull request introduces 1 alert when merging 9bbd4b7c5552361ea3f4229af0e567952c4f6e6a into 9a88cb6 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2021

This pull request introduces 1 alert when merging 49fc4b5445e3ba20bff6c4b6352c6d595f773e4f into 9a88cb6 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2021

This pull request introduces 1 alert when merging 1e29648f2396e9a140a8edad07ba0f485cd9198a into 9a88cb6 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2021

This pull request introduces 1 alert when merging f7f24591e7ee5983f28c5c60911c2982a6fa860d into 9a88cb6 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

@lgtm-com
Copy link

lgtm-com bot commented May 7, 2021

This pull request introduces 1 alert when merging 812d09f7ccd43d0bee031904d5db223ac7446720 into 8c2980a - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

Signed-off-by: Praveen Chaudhary <pchaudhary@linkedin.com>
@jleveque jleveque changed the title [main.py]: Implement a config process level lock. [config]: Implement a process level lock May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants