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

[sonic-host-services]: Support GCU and reload #1

Merged
merged 9 commits into from
Nov 18, 2022
Merged

[sonic-host-services]: Support GCU and reload #1

merged 9 commits into from
Nov 18, 2022

Conversation

ganglyu
Copy link
Collaborator

@ganglyu ganglyu commented Jul 13, 2022

Signed-off-by: Gang Lv ganglv@microsoft.com

Why I did it
GNMI needs to use host service to invoke generic_config_updater and config reload.

How I did it
Add host_modules for generic_config_updater and config reload.
Add unit test for host modules.

How to verify it
Run unit test for sonic-host-services.

config apply-patch
config checkpoint
config delete checkpoint
config reload
@ganglyu ganglyu requested a review from qiluo-msft July 13, 2022 03:07
@ganglyu ganglyu changed the title Add dbus support for below commands: [sonic-host-services]: Support GCU and reload Jul 13, 2022
@sonic-net sonic-net deleted a comment from azure-pipelines bot Jul 13, 2022
@sonic-net sonic-net deleted a comment from mssonicbld Jul 13, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 8, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ganglyu / name: ganglv (86eb51f)

@wen587
Copy link

wen587 commented Aug 11, 2022

GCU part LGTM.

if 'Error' in line:
msg = line
break
return result.returncode, msg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Execute command and return error message code seems dupe in multiple places, suggest create a new method.

test_ret = 0
test_msg = b"Error: this is the test message\nHello world\n"
attrs = {"returncode": test_ret, "stderr": test_msg}
res_mock.configure_mock(**attrs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest create a new method for dupe code.

pytest.ini Show resolved Hide resolved
@qiluo-msft
Copy link
Contributor

@bhagatyj Could you or your team help review?

@lgtm-com
Copy link

lgtm-com bot commented Nov 6, 2022

This pull request introduces 1 alert when merging 8287786 into 6eac2d3 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 14, 2022

This pull request introduces 1 alert when merging be6b250 into 6eac2d3 - view on LGTM.com

new alerts:

  • 1 for Unused import

def reload(self, config_db_json):

cmd = ['/usr/local/bin/config', 'reload', '-y']
config_db_json = config_db_json.strip()
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strip

Why strip? Is it a valid user intention to provide the argument with extra blanks? #Closed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still have the question.

Copy link
Collaborator Author

@ganglyu ganglyu Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if config_db_json and len(config_db_json.strip()):

Now I use len(config_db_json.strip()) to detect string with only spaces, and config_db_json is still the same.

host_modules/gcu.py Outdated Show resolved Hide resolved
host_modules/gcu.py Outdated Show resolved Hide resolved
@ganglyu ganglyu requested a review from qiluo-msft November 14, 2022 07:07
@ganglyu ganglyu merged commit d3a4cf1 into sonic-net:master Nov 18, 2022
isabelmsft pushed a commit to isabelmsft/sonic-host-services that referenced this pull request Dec 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants