-
Notifications
You must be signed in to change notification settings - Fork 666
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
add support for MCLAG #453
Conversation
Signed-off-by: shine.chen <shine.chen@nephosinc.com>
Retest this please |
Signed-off-by: shine.chen <shine.chen@nephosinc.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BRCM team has completed the review, we have no further request or comment to add, please proceed to the next steps.
Signed-off-by: shine.chen <shine.chen@mediatek.com>
Signed-off-by: shine.chen <shine.chen@mediatek.com>
retest this please |
retest this please |
1 similar comment
retest this please |
retest this please |
Signed-off-by: shine.chen <shine.chen@mediatek.com>
optional_services_dict = config_db.get_table('FEATURE') | ||
if not optional_services_dict: | ||
return None | ||
return optional_services_dict.keys() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this function could return the entire dictionary, then the consumers below can determine whether or not to stop/start the service based on whether it is enabled or disabled in the DB, in conjunction with the result of running systemctl is-enabled
. Theoretically, the two states should match, but I'm trying to consider the corner case just in case they are out-of-sync.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If out of sync I prefer the result from systemctl is-enable
. In general this out of sync should be caused by user behavior such as directly running systemctl enable xxx
. This is the reason I choose systemctl is-enabled
instead of config-db.
Anyway I am open for this. Giving priority to config-db is acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jleveque any comment? If no could you approve this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this approach. I will approve the PR, but I would also like to get @lguohan's approval before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
retest this please |
Retest this please |
retest this please |
config_db.connect() | ||
optional_services_dict = config_db.get_table('FEATURE') | ||
if not optional_services_dict: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shine4chen please fix a bug:
TASK [execute cli "config load_minigraph -y" to apply new minigraph] ***********
Tuesday 25 February 2020 19:54:51 +0000 (0:00:00.067) 0:00:18.526 ******
fatal: [vlab-01]: FAILED! => {"changed": true, "cmd": "config load_minigraph -y", "delta": "0:00:55.103499", "end": "2020-02-25 19:55:46.147827", "msg": "non-zero return code", "rc": 1, "start": "2020-02-25 19:54:51.044328", "stderr": "Traceback (most recent call last):\n File \"/usr/bin/config\", line 12, in <module>\n sys.exit(config())\n File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 722, in __call__\n return self.main(*args, **kwargs)\n File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 697, in main\n rv = self.invoke(ctx)\n File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 1066, in invoke\n return _process_result(sub_ctx.command.invoke(sub_ctx))\n File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 895, in invoke\n return ctx.invoke(self.callback, **ctx.params)\n File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 535, in invoke\n return callback(*args, **kwargs)\n File \"/usr/lib/python2.7/dist-packages/config/main.py\", line 684, in load_minigraph\n _stop_services()\n File \"/usr/lib/python2.7/dist-packages/config/main.py\", line 451, in _stop_services\n for service in _get_optional_services():\nTypeError: 'NoneType' object is not iterable", "stderr_lines": ["Traceback (most recent call last):", " File \"/usr/bin/config\", line 12, in <module>", " sys.exit(config())", " File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 722, in __call__", " return self.main(*args, **kwargs)", " File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 697, in main", " rv = self.invoke(ctx)", " File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 1066, in invoke", " return _process_result(sub_ctx.command.invoke(sub_ctx))", " File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 895, in invoke", " return ctx.invoke(self.callback, **ctx.params)", " File \"/usr/lib/python2.7/dist-packages/click/core.py\", line 535, in invoke", " return callback(*args, **kwargs)", " File \"/usr/lib/python2.7/dist-packages/config/main.py\", line 684, in load_minigraph", " _stop_services()", " File \"/usr/lib/python2.7/dist-packages/config/main.py\", line 451, in _stop_services", " for service in _get_optional_services():", "TypeError: 'NoneType' object is not iterable"], "stdout": "Stopping service swss ...\nStopping service lldp ...\nStopping service pmon ...\nStopping service bgp ...\nStopping service hostcfgd ...\nStopping service nat ...", "stdout_lines": ["Stopping service swss ...", "Stopping service lldp ...", "Stopping service pmon ...", "Stopping service bgp ...", "Stopping service hostcfgd ...", "Stopping service nat ..."]}
@qiluo-msft / @yxieca FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems github website has some error that we can't open PRs now. I will fix it after github service recover.
the return value of _get_optional_services() must be iterable This bug is imported from PR #453 Co-authored-by: shine.chen <shine.chen@mediatek.com>
* add support for MCLAG Signed-off-by: shine.chen <shine.chen@nephosinc.com> * add warm-reboot support for ICCPd Signed-off-by: shine.chen <shine.chen@nephosinc.com> * ensure iccpd is there before stop iccpd Signed-off-by: shine.chen <shine.chen@mediatek.com> * fix service function * remove unused comment * refactor code according to feature management mechanism Signed-off-by: shine.chen <shine.chen@mediatek.com>
the return value of _get_optional_services() must be iterable This bug is imported from PR sonic-net#453 Co-authored-by: shine.chen <shine.chen@mediatek.com>
This reverts commit 8aea564.
Signed-off-by: shine.chen shine.chen@nephosinc.com
- What I did
add support for MCLAG
- How I did it
Add iccpd(mclag daemon) item on restart_service and stop service.
issue docker kill iccpd when fast-reboot
- How to verify it
Tested on nephos lab
- 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)
-->