[config reload] Fixing config reload when timer based delayed services are disabled by dgsudharsan · Pull Request #1967 · sonic-net/sonic-utilities

dgsudharsan · 2021-12-10T20:51:40Z

Signed-off-by: Sudharsan Dhamal Gopalarathnam sudharsand@nvidia.com

What I did

When timer based delayed services like mgmt-framework, telemetry and snmp are disabled and config reload is execute it fails Failed to reset failed state of unit mgmt-framework.service: Unit mgmt-framework.service not loaded.
The reason is these services don't get masked like regular services and these are derived from timers. So when reset-failed is tried on these services it leads to exception.

How I did it

When the feature related to these services are disabled their timers would be masked and wouldn't be "enabled". So when deriving the services from timers the services which are not enabled will be skipped.

How to verify it

Disable services like mgmt-framework, snmp and telemetry and execute config reload. The config reload should execute without failure

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)

…abled Signed-off-by: Sudharsan Dhamal Gopalarathnam <sudharsand@nvidia.com>

Correcting indentation

…ies into config_rel_fix2

liat-grozovik · 2021-12-11T14:46:54Z

tests/config_test.py

            # Verify "systemctl reset-failed" is called for services under sonic-delayed.target 
            mock_run_command.assert_any_call('systemctl reset-failed snmp')
-            assert mock_run_command.call_count == 10
+            assert mock_run_command.call_count == 11


@dgsudharsan what this hard coded number stands for? should we count it in a better way than just a number?

I believe the logic is testing for number of calls to clicommon.run_command which is internal implementation. There is no way of knowing it in test script (even in the code). If tomorrow an additional command gets added this needs to be changed again. I agree this is not a deterministic method but its an existing code and I am not sure of the motivation.
For e.g below are the calls internally invoked.
sudo systemctl stop sonic.target --job-mode replace-irreversibly
/usr/local/bin/sonic-cfggen -H -m --write-to-db
config qos reload --no-dynamic-buffer
pfcwd start_default
systemctl list-dependencies --plain sonic.target | sed '1d'
systemctl list-dependencies --plain sonic-delayed.target | sed '1d'
systemctl is-enabled snmp.timer
systemctl reset-failed swss
systemctl reset-failed snmp
sudo systemctl restart sonic.target
sudo monit reload

qiluo-msft · 2021-12-15T00:46:00Z

config/main.py

 def _get_delayed_sonic_services():
    out = clicommon.run_command("systemctl list-dependencies --plain sonic-delayed.target | sed '1d'", return_cmd=True)
-    return (unit.strip().rstrip('.timer') for unit in out.splitlines())
+    timers = [unit for unit in out.splitlines() if(clicommon.run_command("systemctl is-enabled {}".format(unit),


systemctl

you can use one command to check all the units. You may use xargs to bypass the command line too long issue. #Pending

qiluo-msft · 2021-12-15T00:47:12Z

config/main.py

 def _get_delayed_sonic_services():
    out = clicommon.run_command("systemctl list-dependencies --plain sonic-delayed.target | sed '1d'", return_cmd=True)
-    return (unit.strip().rstrip('.timer') for unit in out.splitlines())
+    timers = [unit for unit in out.splitlines() if(clicommon.run_command("systemctl is-enabled {}".format(unit),


unit

This statement is too complex. Could you rewrite in traditional control flow, like for-loop or if-block? #Closed

qiluo-msft · 2021-12-17T05:24:52Z

config/main.py

+    timers = []
+    for unit in out.splitlines():
+        state = clicommon.run_command("systemctl is-enabled {}".format(unit), return_cmd=True)
+        if(state.strip() == "enabled"):


(

Add a blank after if, you can remove the outer (). #Closed

…_rel_fix2

liat-grozovik · 2021-12-20T15:48:52Z

@dgsudharsan is this also required for 202111? if so, i will add the label

liat-grozovik · 2021-12-28T08:24:32Z

/azp run Azure.sonic-utilities

azure-pipelines · 2021-12-28T08:24:43Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2022-01-02T16:01:55Z

/azp run Azure.sonic-utilities

azure-pipelines · 2022-01-02T16:02:06Z

Azure Pipelines successfully started running 1 pipeline(s).

dgsudharsan · 2022-01-05T17:34:35Z

/azpw run Azure.sonic-utilities (Test vstest)

mssonicbld · 2022-01-05T17:34:36Z

/AzurePipelines run Azure.sonic-utilities (Test vstest)

azure-pipelines · 2022-01-05T17:34:42Z

No pipelines are associated with this pull request.

dgsudharsan · 2022-01-05T18:51:27Z

/azpw run Azure.sonic-utilities

mssonicbld · 2022-01-05T18:51:29Z

/AzurePipelines run Azure.sonic-utilities

azure-pipelines · 2022-01-05T18:51:40Z

Azure Pipelines successfully started running 1 pipeline(s).

…s are disabled (#1967) #### What I did When timer based delayed services like mgmt-framework, telemetry and snmp are disabled and config reload is execute it fails Failed to reset failed state of unit mgmt-framework.service: Unit mgmt-framework.service not loaded. The reason is these services don't get masked like regular services and these are derived from timers. So when reset-failed is tried on these services it leads to exception. #### How I did it When the feature related to these services are disabled their timers would be masked and wouldn't be "enabled". So when deriving the services from timers the services which are not enabled will be skipped. #### How to verify it Disable services like mgmt-framework, snmp and telemetry and execute config reload. The config reload should execute without failure

… services are disabled (sonic-net#1967)" This reverts commit 055ed4f.

…s are disabled (#1967) #### What I did When timer based delayed services like mgmt-framework, telemetry and snmp are disabled and config reload is execute it fails Failed to reset failed state of unit mgmt-framework.service: Unit mgmt-framework.service not loaded. The reason is these services don't get masked like regular services and these are derived from timers. So when reset-failed is tried on these services it leads to exception. #### How I did it When the feature related to these services are disabled their timers would be masked and wouldn't be "enabled". So when deriving the services from timers the services which are not enabled will be skipped. #### How to verify it Disable services like mgmt-framework, snmp and telemetry and execute config reload. The config reload should execute without failure

c31a362 - 2021-11-18 : [202012][Mux orch] set default as standby, change mux orch priority (sonic-net#2015) [Prince Sunny] 9a9e8e6 - 2021-11-18 : [202012] Check VS test failure (sonic-net#2033) [Prince Sunny] 7eaabca - 2021-11-11 : [202012] Fix random failure in PR/CI build. (sonic-net#2016) [Shilong Liu] 85230fe - 2021-11-04 : [orchagent] Fix group name of port-buffer-drop in flexcounterorch.cpp (sonic-net#1967) [Junchao-Mellanox] a55c2ca - 2021-11-03 : [teammgrd]: Handle LAGs cleanup gracefully on Warm/Fast reboot. (sonic-net#1934) [Nazarii Hnydyn]

dgsudharsan and others added 4 commits December 9, 2021 02:16

[config reload]Fixing config reload when timer based services are dis…

497d9e0

…abled Signed-off-by: Sudharsan Dhamal Gopalarathnam <sudharsand@nvidia.com>

Update main.py

8730bf2

Correcting indentation

Updating UT

c48d6cc

Merge branch 'config_rel_fix2' of github.com:dgsudharsan/sonic-utilit…

ed68ac1

…ies into config_rel_fix2

dgsudharsan requested a review from Junchao-Mellanox December 10, 2021 20:51

liat-grozovik reviewed Dec 11, 2021

View reviewed changes

Junchao-Mellanox previously approved these changes Dec 13, 2021

View reviewed changes

qiluo-msft changed the title ~~[config reload]Fixing config reload when timer based delayed services are disabled~~ [config reload] Fixing config reload when timer based delayed services are disabled Dec 15, 2021

qiluo-msft reviewed Dec 15, 2021

View reviewed changes

Addressing code review comments

b1b9679

dgsudharsan dismissed Junchao-Mellanox’s stale review via b1b9679 December 17, 2021 05:20

qiluo-msft reviewed Dec 17, 2021

View reviewed changes

dgsudharsan added 2 commits December 17, 2021 06:11

Addressing code review comments

263f3cb

Addressing code review comments

e6f3ec0

qiluo-msft approved these changes Dec 17, 2021

View reviewed changes

dgsudharsan added 2 commits December 17, 2021 16:59

Merge remote-tracking branch 'upstream/master' into config_rel_fix2

7f0897a

Merge branch 'master' of github.com:Azure/sonic-utilities into config…

6bf1631

…_rel_fix2

liat-grozovik approved these changes Jan 2, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into config_rel_fix2

93c44e1

Merge remote-tracking branch 'upstream/master' into config_rel_fix2

3597962

dgsudharsan added Request for 202012 Branch Request for 202106 Branch Request for 202111 Branch labels Jan 6, 2022

qiluo-msft merged commit 055ed4f into sonic-net:master Jan 6, 2022

judyjoseph added the Included in 202111 Branch label Jan 9, 2022

qiluo-msft added the Included in 202012 Branch label Jan 10, 2022

qiluo-msft added a commit to qiluo-msft/sonic-utilities that referenced this pull request Jan 16, 2022

Revert "[config reload] Fixing config reload when timer based delayed…

d7e23fb

… services are disabled (sonic-net#1967)" This reverts commit 055ed4f.

qiluo-msft mentioned this pull request Jan 16, 2022

Revert "[config reload] Fixing config reload when timer based delayed… #2017

Closed

wenyiz2021 mentioned this pull request Feb 4, 2022

[202012] [submodule] Update sonic-utilities submodule sonic-net/sonic-buildimage#9918

Merged

5 tasks

arlakshm added the Included in 202106 Branch label Mar 1, 2022

dgsudharsan deleted the config_rel_fix2 branch March 9, 2023 02:06

dgsudharsan restored the config_rel_fix2 branch March 9, 2023 02:06

dgsudharsan deleted the config_rel_fix2 branch March 9, 2023 02:06

Conversation

dgsudharsan commented Dec 10, 2021

What I did

How I did it

How to verify it

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)

Uh oh!

liat-grozovik Dec 11, 2021

Choose a reason for hiding this comment

Uh oh!

dgsudharsan Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liat-grozovik commented Dec 20, 2021

Uh oh!

liat-grozovik commented Dec 28, 2021

Uh oh!

azure-pipelines bot commented Dec 28, 2021

Uh oh!

liat-grozovik commented Jan 2, 2022

Uh oh!

azure-pipelines bot commented Jan 2, 2022

Uh oh!

dgsudharsan commented Jan 5, 2022

Uh oh!

mssonicbld commented Jan 5, 2022

Uh oh!

azure-pipelines bot commented Jan 5, 2022

Uh oh!

dgsudharsan commented Jan 5, 2022

Uh oh!

mssonicbld commented Jan 5, 2022

Uh oh!

azure-pipelines bot commented Jan 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

qiluo-msft Dec 15, 2021 •

edited

Loading

qiluo-msft Dec 15, 2021 •

edited

Loading

qiluo-msft Dec 17, 2021 •

edited

Loading