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.

Sign up for GitHub

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

Jump to bottom

Container image clean up bug fix #15772

Merged
merged 3 commits into from
Jul 14, 2023
Merged

Container image clean up bug fix #15772

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad18bfc
Container image clean up bug fix
lixiaoyuner Jul 10, 2023
a21f26b
kube image and local image maybe the same id
lixiaoyuner Jul 12, 2023
f67cd31
Rename some variables to be more readable
lixiaoyuner Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/sonic-ctrmgrd/ctrmgr/ctrmgrd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ def on_state_update(self, key, op, data):

def do_tag_latest(self, feat, docker_id, image_ver):
ret = kube_commands.tag_latest(feat, docker_id, image_ver)
if ret != 0:
if ret == 1:
# Tag latest failed. Retry after an interval
self.start_time = datetime.datetime.now()
self.start_time += datetime.timedelta(
Expand All @@ -590,7 +590,7 @@ def do_tag_latest(self, feat, docker_id, image_ver):

log_debug("Tag latest as local failed retry after {} seconds @{}".
format(remote_ctr_config[TAG_RETRY], self.start_time))
else:
elif ret == 0:
last_version = self.st_data[feat][ST_FEAT_CTR_STABLE_VER]
if last_version == image_ver:
last_version = self.st_data[feat][ST_FEAT_CTR_LAST_VER]
Expand All @@ -600,6 +600,10 @@ def do_tag_latest(self, feat, docker_id, image_ver):
self.st_data[ST_FEAT_CTR_LAST_VER] = last_version
self.st_data[ST_FEAT_CTR_STABLE_VER] = image_ver
self.do_clean_image(feat, image_ver, last_version)
elif ret == -1:
# This means the container we want to tag latest is not running
# so we don't need to do clean up
pass

def do_clean_image(self, feat, current_version, last_version):
ret = kube_commands.clean_image(feat, current_version, last_version)
Expand Down
27 changes: 17 additions & 10 deletions src/sonic-ctrmgrd/ctrmgr/kube_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ def tag_latest(feat, docker_id, image_ver):
else:
log_error(err)
elif ret == -1:
ret = 0
log_debug(out)
else:
log_error(err)
return ret
Expand All @@ -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 = {}
Copy link

@losha228 losha228 Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

for info in image_info.split("\n"):
rep, version, docker_id = info.split()
rep, version, image_id = info.split()
if len(rep.split("/")) == 1:
version_dict_default[version] = {DOCKER_ID: docker_id, REPO: rep}
version_dict_default[version] = {IMAGE_ID: image_id, REPO: rep}
else:
version_dict[version] = {DOCKER_ID: docker_id, REPO: rep}
version_dict[version] = {IMAGE_ID: image_id, REPO: rep}

if current_version in version_dict:
image_prefix = version_dict[current_version][REPO]
Expand All @@ -509,9 +509,16 @@ def _do_clean(feat, current_version, last_version):
return ret, out, err
# should be only one item in version_dict_default
for k, v in version_dict_default.items():
local_version, local_repo, local_docker_id = k, v[REPO], v[DOCKER_ID]
tag_res, _, err = _run_command("docker tag {} {}:{} && docker rmi {}:{}".format(
local_docker_id, image_prefix, local_version, local_repo, local_version))
local_version, local_repo, local_image_id = k, v[REPO], v[IMAGE_ID]
# 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
Copy link

@losha228 losha228 Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

if local_version in version_dict:
tag_res, _, err = _run_command("docker rmi {}:{} && docker tag {} {}:{} && docker rmi {}:{}".format(
image_prefix, local_version, local_image_id, image_prefix, local_version, local_repo, local_version))
else:
tag_res, _, err = _run_command("docker rmi {}:{}".format(local_repo, local_version))
if tag_res == 0:
msg = "Tag {} local version images successfully".format(feat)
log_debug(msg)
Expand All @@ -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()]
Copy link

Choose a reason for hiding this comment

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

the "versions" here means image id list ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the "versions" here means image id list ?

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

if versions:
clean_res, _, err = _run_command("docker rmi {} --force".format(" ".join(versions)))
else:
Expand All @@ -534,7 +541,7 @@ def _do_clean(feat, current_version, last_version):
err = "Failed to clean {} old version images. Err: {}".format(feat, err)
ret = 1
else:
err = "Failed to docker images |grep {} |awk '{{print $3}}'".format(feat)
err = "Failed to docker images |grep {} |awk '{{print $3}}'. Error: {}".format(feat, err)
ret = 1

return ret, out, err
Expand Down
23 changes: 21 additions & 2 deletions src/sonic-ctrmgrd/tests/kube_commands_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@
},
2: {
common_test.DESCR: "Tag a unstable container",
common_test.RETVAL: 0,
common_test.RETVAL: -1,
common_test.ARGS: ["snmp", "123456", "v1"],
common_test.PROC_CMD: [
"docker ps |grep 123456"
Expand Down Expand Up @@ -382,7 +382,7 @@
common_test.ARGS: ["snmp", "20201231.84", ""],
common_test.PROC_CMD: [
"docker images |grep snmp |grep -v latest |awk '{print $1,$2,$3}'",
"docker tag 507f8d28bf6e sonick8scue.azurecr.io/docker-sonic-telemetry:20201231.74 && docker rmi docker-sonic-telemetry:20201231.74"
"docker rmi docker-sonic-telemetry:20201231.74"
],
common_test.PROC_OUT: [
"docker-sonic-telemetry 20201231.74 507f8d28bf6e\n\
Expand All @@ -394,6 +394,25 @@
0
]
},
5: {
common_test.DESCR: "Clean image successfuly(local to dry-kube to kube)",
common_test.RETVAL: 0,
common_test.ARGS: ["snmp", "20201231.84", "20201231.74"],
common_test.PROC_CMD: [
"docker images |grep snmp |grep -v latest |awk '{print $1,$2,$3}'",
"docker rmi sonick8scue.azurecr.io/docker-sonic-telemetry:20201231.74 && docker tag 507f8d28bf6e sonick8scue.azurecr.io/docker-sonic-telemetry:20201231.74 && docker rmi docker-sonic-telemetry:20201231.74"
],
common_test.PROC_OUT: [
"docker-sonic-telemetry 20201231.74 507f8d28bf6e\n\
sonick8scue.azurecr.io/docker-sonic-telemetry 20201231.74 507f8d28bf6f\n\
sonick8scue.azurecr.io/docker-sonic-telemetry 20201231.84 507f8d28bf6g",
""
],
common_test.PROC_CODE: [
0,
0
]
},
}

class TestKubeCommands(object):
Expand Down