DBUS API for GNOI System.SetPackage #171

hdwhdw · 2024-10-16T23:22:16Z

Create image_service class for supporting GNOI implementation for SetPackage.

TESTED: unit test.

ganglyu · 2024-10-18T00:35:44Z

host_modules/image_service.py

+    """
+
+    @host_service.method(host_service.bus_name(MOD_NAME), in_signature='ss', out_signature='is')
+    def download(self, image_url, save_as):


Do we need to verify this image_url?

I think dbus services should be a thin layer above CLI and we can push the complexity of such to the caller (gNOI). So this one (download) is basically a wrapper for curl and we don't do additional logic here. What do you think?

ganglyu · 2024-10-18T00:36:07Z

host_modules/image_service.py

+
+
+    @host_service.method(host_service.bus_name(MOD_NAME), in_signature='s', out_signature='is')
+    def install(self, path):


Can this path be a url？

Yes (didn't know that). Similar to above this is a thin layer around "sonic-installer install" so if sonic-installer supports url this also can be a url. Renamed the variable to reflect that.

linux-foundation-easycla · 2024-10-29T20:00:28Z

The committers listed above are authorized under a signed CLA.

✅ login: hdwhdw / name: Dawei Huang (a3b8657, 79b2961, 87eedd8, 9b157d8, 1af745e, c2dfa09, ebeb42d, 1822b00, 71e34b4, daeaec0, 5a1260c, 6b452bb, 6b1f8de, 4278f74, 4f6bd40, b315f05, 1b794ba, c8950cb, 0a32a2d)

qiluo-msft · 2024-11-04T22:25:28Z

host_modules/image_service.py

+
+MOD_NAME = "image_service"
+
+DEFAULT_IMAGE_SAVE_AS = "/host/downloaded-sonic"


host

host

/tmp/ is more safe as a default file location. #Closed

qiluo-msft · 2024-11-04T22:30:33Z

host_modules/image_service.py

+    @host_service.method(
+        host_service.bus_name(MOD_NAME), in_signature="ss", out_signature="is"
+    )
+    def download(self, image_url, save_as):


download

Do we need to support hash check? #Closed

Done, and added unit tests.

qiluo-msft · 2024-11-04T22:36:15Z

host_modules/image_service.py

+        dir = os.path.dirname(save_as)
+        if not os.path.exists(dir):
+            os.makedirs(dir)
+        cmd = ["/usr/bin/curl", "-Lo", save_as, image_url]


curl

curl

This is python code, it is better to use python library to download binary? like using requests. #Closed

qiluo-msft · 2024-11-04T22:38:50Z

host_modules/image_service.py

+        dir = os.path.dirname(save_as)
+        if not os.path.exists(dir):
+            os.makedirs(dir)
+        cmd = ["/usr/bin/curl", "-Lo", save_as, image_url]


save_as

Inspired by another gnoi API design:
target should initially write the file to a temporary location so a failure does not destroy the original file.

If save_as == TMP_IMAGE_FILE, what will happen? Does it harm?

Good point. I use the tempfile library instead now. The library is designed to prevent collision.

qiluo-msft · 2024-11-04T22:39:52Z

host_modules/image_service.py

+    @host_service.method(
+        host_service.bus_name(MOD_NAME), in_signature="ss", out_signature="is"
+    )
+    def download(self, image_url, save_as):


save_as

always check absolute path. #Closed

qiluo-msft · 2024-11-04T22:43:48Z

host_modules/image_service.py

+        # Create parent directory.
+        dir = os.path.dirname(save_as)
+        if not os.path.exists(dir):
+            os.makedirs(dir)


makedirs

makedirs

This function will be finally called by outside, and it is an attacking point. Can we enforce that the dir must exist, and every user has the "w" permission, similar to "/tmp" permission ? #Closed

qiluo-msft · 2024-11-04T22:44:16Z

host_modules/image_service.py

+            where: either a local path or a remote url pointing to the image.
+        """
+        logger.info("Using sonic-installer to install the image at {}.".format(where))
+        cmd = ["sudo", "/usr/local/bin/sonic-installer", "install", "-y", where]


sudo

sudo

Is sudo needed? #Closed

Interesting. I deem it necessary because it is needed for manual run, but so as many config command in other modules. This is not needed.

To more accurately capture what this service really does: everything related to a sonic image.

sonic-installer command support image through url. Update variable names and documentation to reflect that.

Using black.

hdwhdw · 2024-11-05T02:08:43Z

For the record: Force push to reset the email.

ganglyu · 2024-11-06T00:53:13Z

host_modules/image_service.py

+                for chunk in response.iter_content(chunk_size=8192):
+                    tmp_file.write(chunk)
+                temp_file_path = tmp_file.name
+            os.rename(temp_file_path, save_as)


What will happen if this save_as already exists?

os.rename will do nothing. Using os.replace instead.

ganglyu · 2024-11-06T00:56:18Z

host_modules/image_service.py

+            with open(file_path, "rb") as f:
+                for chunk in iter(lambda: f.read(4096), b""):
+                    hash_func.update(chunk)
+            return 0, hash_func.hexdigest()


Can we use md5sum directly? Is this hash result the same as md5sum result? How long will it take to calculate checksum for a typical sonic image?

1-2 seconds. md5sum is slightly faster but not by a noticeable margin. And since this is python code, I think we should use python library to have better control over the logic.

$ python3 Python 3.11.2 (main, May 2 2024, 11:59:08) [GCC 12.2.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> from md5 import calculate_md5 >>> calculate_md5('sonic.bin') (2.589256763458252, '489a131693a4c98803d5a520df409bf9') >>> $ time md5sum sonic.bin 489a131693a4c98803d5a520df409bf9 sonic.bin real 0m1.879s user 0m1.621s sys 0m0.257s

hdwhdw requested review from qiluo-msft, vvolam and ganglyu October 16, 2024 23:24

ganglyu reviewed Oct 18, 2024

View reviewed changes

hdwhdw changed the title ~~Implements necessary host services for image download and installation~~ Create ImageService with download and install. Oct 29, 2024

hdwhdw force-pushed the setpackage branch from 7e00097 to 288ec6c Compare October 29, 2024 20:05

hdwhdw requested a review from ganglyu October 29, 2024 20:06

hdwhdw marked this pull request as ready for review October 30, 2024 19:20

qiluo-msft reviewed Nov 4, 2024

View reviewed changes

hdwhdw changed the title ~~Create ImageService with download and install.~~ Create image_service class for supporting GNOI implementation for SetPackage. Nov 5, 2024

hdwhdw force-pushed the setpackage branch 3 times, most recently from 41b449f to b443096 Compare November 5, 2024 02:02

hdwhdw added 9 commits November 5, 2024 02:06

Create install_sonic host module and its unit test.

a3b8657

Rename the DBUS service to image_service rather than sonic installer.

5a1260c

To more accurately capture what this service really does: everything related to a sonic image.

Addressed comment.

1af745e

sonic-installer command support image through url. Update variable names and documentation to reflect that.

Format fixes for test image service.

ebeb42d

Automated format fix for image_service.py

6b452bb

Using black.

Change the default save-as location.

6b1f8de

Remove sudo, not needed.

71e34b4

Use python API for download.

79b2961

Add Checksum method.

9b157d8

hdwhdw added 3 commits November 5, 2024 02:07

updated unit test for download

4278f74

remaining unit tests, also not raise exception in download for non 200.

87eedd8

format fix

daeaec0

hdwhdw force-pushed the setpackage branch from b443096 to daeaec0 Compare November 5, 2024 02:08

hdwhdw added 2 commits November 5, 2024 02:13

check for absolute path and added test case.

c2dfa09

Use temporary file to cache downloaded file.

b315f05

hdwhdw requested a review from qiluo-msft November 5, 2024 02:29

hdwhdw added 3 commits November 5, 2024 16:12

Add unit tests so we have 100% coverage.

c8950cb

uses tempfile library instead.

0a32a2d

format fix

1b794ba

ganglyu reviewed Nov 6, 2024

View reviewed changes

use os.replace instead of os.rename to force override.

1822b00

ganglyu approved these changes Nov 7, 2024

View reviewed changes

Merge branch 'sonic-net:master' into setpackage

4f6bd40

hdwhdw changed the title ~~Create image_service class for supporting GNOI implementation for SetPackage.~~ DBUS API for GNOI System.SetPackage Nov 8, 2024

qiluo-msft approved these changes Nov 8, 2024

View reviewed changes

qiluo-msft merged commit fec6080 into sonic-net:master Nov 8, 2024
5 checks passed

hdwhdw mentioned this pull request Feb 10, 2025

Independent DPU Upgrade HLD sonic-net/SONiC#1906

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DBUS API for GNOI System.SetPackage #171

DBUS API for GNOI System.SetPackage #171

hdwhdw commented Oct 16, 2024 •

edited

Loading

ganglyu Oct 18, 2024

hdwhdw Oct 18, 2024

ganglyu Oct 18, 2024

hdwhdw Oct 18, 2024

linux-foundation-easycla bot commented Oct 29, 2024 •

edited

Loading

qiluo-msft Nov 4, 2024 •

edited

Loading

hdwhdw Nov 5, 2024

qiluo-msft Nov 4, 2024 •

edited

Loading

hdwhdw Nov 5, 2024

qiluo-msft Nov 4, 2024 •

edited

Loading

hdwhdw Nov 5, 2024

qiluo-msft Nov 4, 2024

hdwhdw Nov 5, 2024

qiluo-msft Nov 5, 2024

hdwhdw Nov 6, 2024

qiluo-msft Nov 4, 2024 •

edited

Loading

hdwhdw Nov 5, 2024

qiluo-msft Nov 4, 2024 •

edited

Loading

hdwhdw Nov 5, 2024

qiluo-msft Nov 4, 2024 •

edited

Loading

hdwhdw Nov 4, 2024

hdwhdw commented Nov 5, 2024

ganglyu Nov 6, 2024

hdwhdw Nov 6, 2024

ganglyu Nov 6, 2024

hdwhdw Nov 6, 2024



		@host_service.method(host_service.bus_name(MOD_NAME), in_signature='s', out_signature='is')
		def install(self, path):


		MOD_NAME = "image_service"

		DEFAULT_IMAGE_SAVE_AS = "/host/downloaded-sonic"

DBUS API for GNOI System.SetPackage #171

DBUS API for GNOI System.SetPackage #171

Conversation

hdwhdw commented Oct 16, 2024 • 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

linux-foundation-easycla bot commented Oct 29, 2024 • edited Loading

qiluo-msft Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Nov 4, 2024 • 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

qiluo-msft Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdwhdw commented Nov 5, 2024

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

hdwhdw commented Oct 16, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 29, 2024 •

edited

Loading

qiluo-msft Nov 4, 2024 •

edited

Loading

qiluo-msft Nov 4, 2024 •

edited

Loading

qiluo-msft Nov 4, 2024 •

edited

Loading

qiluo-msft Nov 4, 2024 •

edited

Loading

qiluo-msft Nov 4, 2024 •

edited

Loading

qiluo-msft Nov 4, 2024 •

edited

Loading