Implementation for ImageService.List #206

hdwhdw · 2025-01-16T16:17:31Z

This PR contains implementation for List method in ImageService host module. The method will list the current, next and available SONiC image on the SONiC device.

It will be used in GNOI library for verifying OS installation.

mssonicbld · 2025-01-16T16:17:34Z

/azp run

azure-pipelines · 2025-01-16T16:17:45Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

host_modules/image_service.py:171

The error message should include the actual error output from the subprocess for better debugging. Suggestion: return errno.EIO, "Failed to list images: {}".format(e.output.decode())

return errno.EIO, "Failed to list images"

host_modules/image_service.py

mssonicbld · 2025-01-16T16:53:27Z

/azp run

azure-pipelines · 2025-01-16T16:53:38Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

xincunli-sonic · 2025-01-17T00:39:28Z

host_modules/image_service.py

+
+        except subprocess.CalledProcessError as e:
+            logger.error("Failed to list images: {}".format(e.output.decode()))
+            return errno.EIO, "Failed to list images"


return errno.EIO, "Failed to list images"

Use a consistent return pattern, such as wrapping the result in a tuple like (return_code, result)

Done. Use json string for success case instead. Such also follows examples from docker.list

xincunli-sonic · 2025-01-17T00:40:21Z

host_modules/image_service.py

+        try:
+            output = subprocess.check_output(
+            ["/usr/local/bin/sonic-installer", "list"],
+            stderr=subprocess.STDOUT,


nit: add indent. #Closed

xincunli-sonic · 2025-01-17T00:43:50Z

host_modules/image_service.py

+            available_images = []
+
+            for line in output:
+                if line.startswith("Current:"):


Have you checked all versions are this word? recommend use insensitive, like lower first then compare?

Yes and checked. Lower case isn't really necessary since, the output method in sonic_installer is from 2017 and haven't changed since then.

xincunli-sonic · 2025-01-17T00:45:19Z

host_modules/image_service.py

+                elif line.startswith("Available:"):
+                    continue
+                else:
+                    available_images.append(line.strip())


Suggest to refactor the parsing into a separate helper function, making it easier to test and debug independently

xincunli-sonic · 2025-01-17T00:45:56Z

host_modules/image_service.py

+        logger.info("Listing SONiC images")
+
+        try:
+            output = subprocess.check_output(


output

Add log for raw output for debugging purpose.

Still applicable, before we do parse, we could log it for future debug purpose.

We have logs for 1. before calling sonic-installer, 2. the output of sonic-installer before we try to parse it, as well as 3. when sonic-installer have a nonzero rc. Added one for parse result of sonic-installer output.

xincunli-sonic · 2025-01-17T00:46:40Z

host_modules/image_service.py

+                    available_images.append(line.strip())
+
+        except subprocess.CalledProcessError as e:
+            logger.error("Failed to list images: {}".format(e.output.decode()))


error

Include more details in the error messages, such as the command that failed or the subprocess exit code?

xincunli-sonic · 2025-01-17T00:47:47Z

tests/host_modules/image_service_test.py

+            "Available:\n"
+            "image1\n"
+            "image2\n"
+        )


There would be multiple combination for the output, you may have to test and retrieve them in multiple versions?

What do you mean? All we care about is the service correctly output all of "current", "next" and "available" images, so adding different combinations doesn't really allow us to catch more bugs, right?

If you check the implementation, you will notice that the current, next, available will be the combination, and they may not exist.

Thanks for the clarification. Added testcase for outputs like "Current: blah\nNext: blah\n".

host_modules/image_service.py

mssonicbld · 2025-01-21T17:16:14Z

/azp run

azure-pipelines · 2025-01-21T17:16:23Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-21T21:55:18Z

/azp run

azure-pipelines · 2025-01-21T21:55:27Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-21T21:56:51Z

/azp run

azure-pipelines · 2025-01-21T21:57:00Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2025-01-21T22:26:37Z

host_modules/image_service.py

+
+        for line in output.split("\n"):
+            if "current:" in line.lower():
+                current_image = line.split(":")[1].strip()


[1]

Could you add error handling of index out of range? #Closed

mssonicbld · 2025-01-23T16:41:37Z

/azp run

azure-pipelines · 2025-01-23T16:41:46Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-23T16:50:12Z

/azp run

azure-pipelines · 2025-01-23T16:50:22Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-23T16:55:42Z

/azp run

azure-pipelines · 2025-01-23T16:55:53Z

Azure Pipelines successfully started running 1 pipeline(s).

xincunli-sonic

LGTM

Why I did it Supports OS.Verify for Upgrading OS How I did it Implement GNOI server and client for GNOI.OS.Verify Backend PR: sonic-net/sonic-host-services#206 How to verify it On Mellanox: gnoi_client -target 127.0.0.1:50052 -logtostderr -insecure -module OS -rpc Verify OS Verify {"version":"SONiC-OS-20240510.23"}

hdwhdw added 2 commits January 16, 2025 08:41

add dbus api for listing sonic images.

e0aebbd

add unit tests.

1130840

hdwhdw requested a review from Copilot January 16, 2025 16:50

Copilot AI reviewed Jan 16, 2025

View reviewed changes

host_modules/image_service.py Outdated Show resolved Hide resolved

address copilot comment.

56663a5

hdwhdw requested a review from Copilot January 16, 2025 16:59

Copilot AI reviewed Jan 16, 2025

View reviewed changes

hdwhdw requested review from qiluo-msft, ganglyu and xincunli-sonic January 16, 2025 16:59

xincunli-sonic reviewed Jan 17, 2025

View reviewed changes

FengPan-Frank reviewed Jan 21, 2025

View reviewed changes

host_modules/image_service.py Outdated Show resolved Hide resolved

hdwhdw added 4 commits January 21, 2025 16:28

use json string or error message as output.

35f60a7

address comment.

a0b223b

update the method name to list_image.

d8b5708

update unit test.

c8104f5

change name from list_images to list_image.

01d7eca

chnage to list_images.

ab8fa1a

qiluo-msft reviewed Jan 21, 2025

View reviewed changes

hdwhdw added 2 commits January 23, 2025 16:21

handles the case where current and available images are empty.

8dd89cd

add test coverage for missing lines and empty image string.

56e6115

add a testcase for lowercase output.

02b402c

more logs for parsing output from sonic-installer.

1ceda79

hdwhdw requested review from qiluo-msft, FengPan-Frank and xincunli-sonic January 23, 2025 17:01

xincunli-sonic approved these changes Jan 23, 2025

View reviewed changes

qiluo-msft approved these changes Jan 23, 2025

View reviewed changes

qiluo-msft merged commit d88d8d0 into sonic-net:master Jan 23, 2025
5 checks passed

hdwhdw deleted the image_list branch January 23, 2025 20:22

hdwhdw mentioned this pull request Jan 28, 2025

GNOI Implementation of OS.Verify sonic-net/sonic-gnmi#342

Merged

6 tasks

Implementation for ImageService.List #206

Implementation for ImageService.List #206

Conversation

hdwhdw commented Jan 16, 2025

mssonicbld commented Jan 16, 2025

azure-pipelines bot commented Jan 16, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

mssonicbld commented Jan 16, 2025

azure-pipelines bot commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xincunli-sonic Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdwhdw Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssonicbld commented Jan 21, 2025

azure-pipelines bot commented Jan 21, 2025

mssonicbld commented Jan 21, 2025

azure-pipelines bot commented Jan 21, 2025

mssonicbld commented Jan 21, 2025

azure-pipelines bot commented Jan 21, 2025

qiluo-msft Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

mssonicbld commented Jan 23, 2025

azure-pipelines bot commented Jan 23, 2025

mssonicbld commented Jan 23, 2025

azure-pipelines bot commented Jan 23, 2025

mssonicbld commented Jan 23, 2025

azure-pipelines bot commented Jan 23, 2025

xincunli-sonic left a comment

Choose a reason for hiding this comment

xincunli-sonic Jan 17, 2025 •

edited

Loading

hdwhdw Jan 21, 2025 •

edited

Loading

qiluo-msft Jan 21, 2025 •

edited

Loading