Container image clean up bug fix #15772

lixiaoyuner · 2023-07-10T12:07:51Z

Why I did it

When do clean up container images, current code has two bugs need to be fixed. And some variables' name maybe cause confused, change the variables' name.

Work item tracking

Microsoft ADO (number only): 24502294

How I did it

We do clean up after tag latest successfully. But currently tag latest function only return 0 and 1, 0 means succeed and 1 means failed, when we get 1, we will retry, when we get 0, we will do clean up. Actually the code 0 includes another case we don't need to do clean up. The case is that when we are doing tag latest, the container image we want to tag maybe not running, so we can not tag latest and don't need to cleanup, we need to separate this case from 0, return -1 now.
When local mode(v1) -> kube mode(v2) happens, one problem is how to handle the local image, there are two cases. one case is that there was one kube v1 container dry-run(cause we don't relace the local if kube version = local version), we will remove the kube v1 image and tag the local version with ACR prefix and remove local v1 local tag. Another case is that there was no kube v1 container dry-run, we remove the local v1 image directly, cause the local v1 image should not be the last desire version.
About the docker_id variable, it may cause confused, it's actually docker image id, so rename the variable. About the two dicts and the list, rename them to be more readable.

How to verify it

Check tag latest and image clean up result.

Which release branch to backport (provide reason below if selected)

Tested branch (Please provide the tested image version)

20220531.28

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

losha228 · 2023-07-13T14:30:01Z

src/sonic-ctrmgrd/ctrmgr/kube_commands.py

+            # if there is a kube image with same version, need to remove the kube version
+            # and tag the local version to kube version for fallback preparation
+            # and remove the local version
+            # if there is no kube image with same version, just remove the local version


it's better to move the comments in front of "else" to explain else case.

losha228 · 2023-07-13T14:36:38Z

src/sonic-ctrmgrd/ctrmgr/kube_commands.py

@@ -523,7 +530,7 @@ def _do_clean(feat, current_version, last_version):
        if last_version in version_dict:
            del version_dict[last_version]

-        versions = [item[DOCKER_ID] for item in version_dict.values()]
+        versions = [item[IMAGE_ID] for item in version_dict.values()]


the "versions" here means image id list ?

the "versions" here means image id list ?

Yes, it's image id list which we need to remove. Let me rename the variable

losha228 · 2023-07-13T14:55:20Z

src/sonic-ctrmgrd/ctrmgr/kube_commands.py

@@ -487,18 +487,18 @@ def _do_clean(feat, current_version, last_version):
    err = ""
    out = ""
    ret = 0
-    DOCKER_ID = "docker_id"
+    IMAGE_ID = "image_id"
    REPO = "repo"
    _, image_info, err = _run_command("docker images |grep {} |grep -v latest |awk '{{print $1,$2,$3}}'".format(feat))
    if image_info:
        version_dict = {}
        version_dict_default = {}


one suggestion: use a meaningful name for variables version_dict/version_dict_default, it will make the code more readable,
there are two kinds of images:
local image: without docker registry repo prefix
remote image: with docker registry repo prefix

Why I did it When do clean up container images, current code has two bugs need to be fixed. And some variables' name maybe cause confused, change the variables' name. Work item tracking Microsoft ADO (number only): 24502294 How I did it We do clean up after tag latest successfully. But currently tag latest function only return 0 and 1, 0 means succeed and 1 means failed, when we get 1, we will retry, when we get 0, we will do clean up. Actually the code 0 includes another case we don't need to do clean up. The case is that when we are doing tag latest, the container image we want to tag maybe not running, so we can not tag latest and don't need to cleanup, we need to separate this case from 0, return -1 now. When local mode(v1) -> kube mode(v2) happens, one problem is how to handle the local image, there are two cases. one case is that there was one kube v1 container dry-run(cause we don't relace the local if kube version = local version), we will remove the kube v1 image and tag the local version with ACR prefix and remove local v1 local tag. Another case is that there was no kube v1 container dry-run, we remove the local v1 image directly, cause the local v1 image should not be the last desire version. About the docker_id variable, it may cause confused, it's actually docker image id, so rename the variable. About the two dicts and the list, rename them to be more readable. How to verify it Check tag latest and image clean up result.

mssonicbld · 2023-07-14T15:46:51Z

@lixiaoyuner PR conflicts with 202211 branch

Why I did it When do clean up container images, current code has two bugs need to be fixed. And some variables' name maybe cause confused, change the variables' name. Work item tracking Microsoft ADO (number only): 24502294 How I did it We do clean up after tag latest successfully. But currently tag latest function only return 0 and 1, 0 means succeed and 1 means failed, when we get 1, we will retry, when we get 0, we will do clean up. Actually the code 0 includes another case we don't need to do clean up. The case is that when we are doing tag latest, the container image we want to tag maybe not running, so we can not tag latest and don't need to cleanup, we need to separate this case from 0, return -1 now. When local mode(v1) -> kube mode(v2) happens, one problem is how to handle the local image, there are two cases. one case is that there was one kube v1 container dry-run(cause we don't relace the local if kube version = local version), we will remove the kube v1 image and tag the local version with ACR prefix and remove local v1 local tag. Another case is that there was no kube v1 container dry-run, we remove the local v1 image directly, cause the local v1 image should not be the last desire version. About the docker_id variable, it may cause confused, it's actually docker image id, so rename the variable. About the two dicts and the list, rename them to be more readable. How to verify it Check tag latest and image clean up result.

mssonicbld · 2023-07-17T15:26:36Z

Cherry-pick PR to 202305: #15870

Why I did it When do clean up container images, current code has two bugs need to be fixed. And some variables' name maybe cause confused, change the variables' name. Work item tracking Microsoft ADO (number only): 24502294 How I did it We do clean up after tag latest successfully. But currently tag latest function only return 0 and 1, 0 means succeed and 1 means failed, when we get 1, we will retry, when we get 0, we will do clean up. Actually the code 0 includes another case we don't need to do clean up. The case is that when we are doing tag latest, the container image we want to tag maybe not running, so we can not tag latest and don't need to cleanup, we need to separate this case from 0, return -1 now. When local mode(v1) -> kube mode(v2) happens, one problem is how to handle the local image, there are two cases. one case is that there was one kube v1 container dry-run(cause we don't relace the local if kube version = local version), we will remove the kube v1 image and tag the local version with ACR prefix and remove local v1 local tag. Another case is that there was no kube v1 container dry-run, we remove the local v1 image directly, cause the local v1 image should not be the last desire version. About the docker_id variable, it may cause confused, it's actually docker image id, so rename the variable. About the two dicts and the list, rename them to be more readable. How to verify it Check tag latest and image clean up result.

…sonic-buildimage into internal Fix conflict for rsyslog. Skip partial DNS unit test in internal branch after confirmed with Gang. Related work items: sonic-net#113, sonic-net#131, sonic-net#132, sonic-net#134, sonic-net#321, sonic-net#331, sonic-net#381, sonic-net#382, sonic-net#2525, sonic-net#2676, sonic-net#2698, sonic-net#2737, sonic-net#2789, sonic-net#2839, sonic-net#2845, sonic-net#2850, sonic-net#2882, sonic-net#2885, sonic-net#2887, sonic-net#2890, sonic-net#2895, sonic-net#13338, sonic-net#14105, sonic-net#15142, sonic-net#15223, sonic-net#15456, sonic-net#15487, sonic-net#15520, sonic-net#15726, sonic-net#15727, sonic-net#15758, sonic-net#15764, sonic-net#15765, sonic-net#15772, sonic-net#15779, sonic-net#15782, sonic-net#15785, sonic-net#15797, sonic-net#15798, sonic-net#15810, sonic-net#15811, sonic-net#15821

lixiaoyuner added 2 commits July 10, 2023 12:07

Container image clean up bug fix

ad18bfc

kube image and local image maybe the same id

a21f26b

lixiaoyuner marked this pull request as ready for review July 12, 2023 13:17

lixiaoyuner requested a review from lguohan as a code owner July 12, 2023 13:17

losha228 reviewed Jul 13, 2023

View reviewed changes

Rename some variables to be more readable

f67cd31

losha228 approved these changes Jul 14, 2023

View reviewed changes

lguohan approved these changes Jul 14, 2023

View reviewed changes

lguohan added Request for 202205 Branch Request for 202305 Branch labels Jul 14, 2023

lguohan merged commit 1bf2a61 into sonic-net:master Jul 14, 2023
18 checks passed

lixiaoyuner added the Request for 202211 Branch label Jul 14, 2023

yxieca added Included in 202205 Branch Approved for 202211 Branch labels Jul 14, 2023

mssonicbld added the Cherry Pick Conflict_202211 label Jul 14, 2023

StormLiangMS added the Approved for 202305 Branch label Jul 17, 2023

mssonicbld added the Created PR to 202305 Branch label Jul 17, 2023

mssonicbld mentioned this pull request Jul 17, 2023

[action] [PR:15772] Container image clean up bug fix #15870

Merged

11 tasks

yxieca added the Included in 202211 Branch label Jul 17, 2023

mssonicbld added a commit that referenced this pull request Jul 19, 2023

[ctrmgr]: Container image clean up bug fix (#15772) (#15870)

38e721b

mssonicbld added Included in 202305 Branch and removed Created PR to 202305 Branch labels Jul 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container image clean up bug fix #15772

Container image clean up bug fix #15772

lixiaoyuner commented Jul 10, 2023 •

edited

Loading

losha228 Jul 13, 2023 •

edited

Loading

lixiaoyuner Jul 13, 2023

losha228 Jul 13, 2023

lixiaoyuner Jul 13, 2023

losha228 Jul 13, 2023 •

edited

Loading

lixiaoyuner Jul 13, 2023

mssonicbld commented Jul 14, 2023

mssonicbld commented Jul 17, 2023

Container image clean up bug fix #15772

Container image clean up bug fix #15772

Conversation

lixiaoyuner commented Jul 10, 2023 • edited Loading

Why I did it

Work item tracking

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

losha228 Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

lixiaoyuner Jul 13, 2023

Choose a reason for hiding this comment

losha228 Jul 13, 2023

Choose a reason for hiding this comment

lixiaoyuner Jul 13, 2023

Choose a reason for hiding this comment

losha228 Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

lixiaoyuner Jul 13, 2023

Choose a reason for hiding this comment

mssonicbld commented Jul 14, 2023

mssonicbld commented Jul 17, 2023

lixiaoyuner commented Jul 10, 2023 •

edited

Loading

losha228 Jul 13, 2023 •

edited

Loading

losha228 Jul 13, 2023 •

edited

Loading