Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker #2278

Junchao-Mellanox · 2022-07-27T02:59:53Z

What I did

Fix issues:

upgrade-docker shall not check STATE DB for those container which do not support warm mode
rollback-docker sometimes cannot really rollback the container because it cannot get the correct docker image tag

How I did it

for containers do not support warm mode, ignore STATE_DB check
use another command to get docker image tag

How to verify it

Manual test.

UT is not created for this change because there are many docker command running for these two CLIs, thus:

Mock so many docker command causes the test case very tricky. The case would be even more complicated than the CLIs.
UT cannot really test the command in case of so many docker command

Instead, a new sonic-mgmt test case is on the way to cover these two CLI. Will create PR for the sonic-mgmt new test case later.

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)

…lback-docker

yxieca · 2022-07-28T21:50:56Z

@Junchao-Mellanox can you add unit test?

Junchao-Mellanox · 2022-07-29T02:00:26Z

Hi Ying, these two CLIs runs a lot docker/sonic commands which makes it hard to be unit tested. I have a sonic-mgmt PR to cover the test: sonic-net/sonic-mgmt#6054 . Would that be ok?

Junchao-Mellanox · 2022-08-04T09:13:09Z

Hi @yxieca , could you please provide your comment?

qiluo-msft · 2022-08-04T21:28:54Z

sonic_installer/common.py

@@ -31,6 +31,8 @@ def run_command(command):
    if proc.returncode != 0:
        sys.exit(proc.returncode)

+    return out.rstrip("\n")


return out.rstrip("\n")

To make coverage easier, do you really need this change? #Closed

Since I will rollback line 942, I don't need this change any more.

qiluo-msft · 2022-08-04T21:51:41Z

sonic_installer/main.py

@@ -938,7 +939,8 @@ def rollback_docker(container_name):
    version_tag = ""
    for id in image_id_all:
        if id != image_id_previous:
-            version_tag = get_docker_tag_name(id)


get_docker_tag_name

you can change the implementation of get_docker_tag_name(), and this function should be easy to unit test, you still need some mock. #Closed

Hi qi, thanks for the suggestion. I would to rollback this change because I found the real issue is in sonic build system. Here is my analyze:

During my previous manual test with a local image, I found get_docker_tag_name() cannot return the correct tag name of docker. The return value of get_docker_tag_name() does not equal to output of docker images.

However, I re-visit the logic today, and I found:

get_docker_tag_name() tries to get the docker tag by querying a docker label specified by sonic make file.

cmd = "docker inspect --format '{{.ContainerConfig.Labels.Tag}}' " + image

This label is added in slave.mk. See https://github.com/sonic-net/sonic-buildimage/blob/736c739bf492a73c60cfca000b853c040f53a104/slave.mk#L895

docker build --squash --no-cache \ ... --label Tag=$(SONIC_IMAGE_VERSION) \

The real docker tag is added in build_debian.sh. See https://github.com/sonic-net/sonic-buildimage/blob/736c739bf492a73c60cfca000b853c040f53a104/files/build_templates/sonic_debian_extension.j2#L697

sudo LANG=C DOCKER_HOST="$DOCKER_HOST" chroot $FILESYSTEM_ROOT docker tag {{imagename}}:latest {{imagename}}:"${SONIC_IMAGE_VERSION}"

In theory, item#2 and item#3 shall get the same value of ${SONIC_IMAGE_VERSION}. But it doesn't. I got from build log:

For item#2, it prints:

docker-database.gz.log:Step 23/25 : LABEL Tag=syslog-rate-limit.0-dirty-20220804.115836

For item#3, it prints:

sonic-mellanox.bin.log:+ sudo LANG=C DOCKER_HOST= chroot ./fsroot-mellanox docker tag docker-database:latest docker-database:syslog-rate-limit.0-dirty-20220804.165951

As you can see, value of SONIC_IMAGE_VERSION is different in the same build. I suppose SONIC_IMAGE_VERSION value shall always be the same in the same build.

The issue is probably related to the timestamp in the SONIC_IMAGE_VERSION. However, I found timestamp is not part of SONIC_IMAGE_VERSION in official image. So, maybe we can ignore this issue.

qiluo-msft · 2022-08-04T21:58:04Z

sonic_installer/main.py

+    if cleanup_image:
+        # Unless requested, the previoud docker image will be preserved
+        for id in image_id_all:
+            if id != image_id_latest and id == image_id_previous:


id != image_id_latest and id == image_id_previous

This check is weird, seems like image_id_latest != image_id_previous, and not related to the loop var. Could you double check? #Closed

linux-foundation-easycla · 2022-08-05T06:55:00Z

The committers listed above are authorized under a signed CLA.

✅ login: Junchao-Mellanox (a3bbf46, 58b1cec, b4a0720)

liat-grozovik · 2022-08-08T12:26:57Z

@qiluo-msft could you please help to approve again following the improved coverage?

Update sonic-utilities submodule pointer to include the following: * Fix GCU bug when backend service modifying config ([sonic-net#2295](sonic-net/sonic-utilities#2295)) * Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker ([sonic-net#2278](sonic-net/sonic-utilities#2278)) * [crm] add checking for CRM interval range ([sonic-net#2293](sonic-net/sonic-utilities#2293)) * Fix the issue that sonic_platform is not installed on vs image ([sonic-net#2300](sonic-net/sonic-utilities#2300)) * Add FEC correctable and uncorrectable port stats ([sonic-net#2027](sonic-net/sonic-utilities#2027)) * Add CLI to configure YANG config validation ([sonic-net#2147](sonic-net/sonic-utilities#2147)) * Add override testcase to verify removal ([sonic-net#2288](sonic-net/sonic-utilities#2288)) * Fix version in db_migrator for ([sonic-net#2289](sonic-net/sonic-utilities#2289)) * [intfutil] Check whether the FEC mode is supported on the platform before configuring it to CONFIG_DB ([sonic-net#2223](sonic-net/sonic-utilities#2223)) * Transfer organization from Azure to sonic-net ([sonic-net#2284](sonic-net/sonic-utilities#2284)) * [watermarkstat] Fix CLI script for unconfigured PG counters ([sonic-net#2239](sonic-net/sonic-utilities#2239)) * Improve the way to check port type of RJ45 port ([sonic-net#2249](sonic-net/sonic-utilities#2249)) Signed-off-by: dprital <drorp@nvidia.com>

…lback-docker (sonic-net#2278) #### What I did Fix issues: 1. upgrade-docker shall not check STATE DB for those container which do not support warm mode 2. rollback-docker sometimes cannot really rollback the container because it cannot get the correct docker image tag #### How I did it 1. for containers do not support warm mode, ignore STATE_DB check 2. use another command to get docker image tag #### How to verify it Manual test. UT is not created for this change because there are many docker command running for these two CLIs, thus: 1. Mock so many docker command causes the test case very tricky. The case would be even more complicated than the CLIs. 2. UT cannot really test the command in case of so many docker command Instead, a new sonic-mgmt test case is on the way to cover these two CLI. Will create PR for the sonic-mgmt new test case later.

Junchao-Mellanox added 2 commits July 20, 2022 18:19

Fix issues for sonic_installer upgrade-docker and sonic_installer rol…

a3bbf46

…lback-docker

Allow mgmt-framework to be warm restart

58b1cec

Junchao-Mellanox requested a review from keboliu July 27, 2022 03:00

stepanblyschak previously approved these changes Jul 27, 2022

View reviewed changes

Junchao-Mellanox requested a review from yxieca July 28, 2022 09:09

qiluo-msft reviewed Aug 4, 2022

View reviewed changes

Fix review comment and add unit test

b4a0720

Junchao-Mellanox dismissed stepanblyschak’s stale review via b4a0720 August 5, 2022 06:54

qiluo-msft previously approved these changes Aug 5, 2022

View reviewed changes

Improve coverage

d690b7b

Junchao-Mellanox dismissed qiluo-msft’s stale review via d690b7b August 8, 2022 03:05

qiluo-msft approved these changes Aug 8, 2022

View reviewed changes

qiluo-msft merged commit bcf36eb into sonic-net:master Aug 8, 2022

dprital mentioned this pull request Aug 9, 2022

[submodule] Update sonic-utilities submodule sonic-net/sonic-buildimage#11657

Closed

7 tasks

stephenxs mentioned this pull request Aug 9, 2022

[submodule] Advance sonic-swss and sonic-utilities pointer sonic-net/sonic-buildimage#11643

Merged

7 tasks

Junchao-Mellanox deleted the fix-upgrade-docker-1 branch March 6, 2023 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker #2278

Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker #2278

Junchao-Mellanox commented Jul 27, 2022 •

edited

Loading

yxieca commented Jul 28, 2022

Junchao-Mellanox commented Jul 29, 2022

Junchao-Mellanox commented Aug 4, 2022

qiluo-msft Aug 4, 2022 •

edited

Loading

Junchao-Mellanox Aug 5, 2022

qiluo-msft Aug 4, 2022 •

edited

Loading

Junchao-Mellanox Aug 5, 2022 •

edited

Loading

qiluo-msft Aug 4, 2022 •

edited

Loading

Junchao-Mellanox Aug 5, 2022

linux-foundation-easycla bot commented Aug 5, 2022 •

edited

Loading

liat-grozovik commented Aug 8, 2022

Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker #2278

Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker #2278

Conversation

Junchao-Mellanox commented Jul 27, 2022 • edited Loading

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)

yxieca commented Jul 28, 2022

Junchao-Mellanox commented Jul 29, 2022

Junchao-Mellanox commented Aug 4, 2022

qiluo-msft Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Junchao-Mellanox Aug 5, 2022

Choose a reason for hiding this comment

qiluo-msft Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Junchao-Mellanox Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Junchao-Mellanox Aug 5, 2022

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Aug 5, 2022 • edited Loading

liat-grozovik commented Aug 8, 2022

Junchao-Mellanox commented Jul 27, 2022 •

edited

Loading

qiluo-msft Aug 4, 2022 •

edited

Loading

qiluo-msft Aug 4, 2022 •

edited

Loading

Junchao-Mellanox Aug 5, 2022 •

edited

Loading

qiluo-msft Aug 4, 2022 •

edited

Loading

linux-foundation-easycla bot commented Aug 5, 2022 •

edited

Loading