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.

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

fix pinned the preloaded pause image #9460

Closed
wants to merge 1 commit into from

Conversation

Iceber
Copy link
Member

@Iceber Iceber commented Dec 4, 2023

Fix pause image already exists but is not pinned.

The code that needs to be fixed is in two places:

  1. use image reference/name to get image cri labels
  2. when updating the image, not just determine the io.cri-containerd.image label

related pr: #8866 (comment)

@Iceber Iceber marked this pull request as draft December 4, 2023 03:31
@Iceber Iceber force-pushed the fix_preload_pause_image branch 2 times, most recently from 4f5b28a to a127acf Compare December 4, 2023 05:05
@Iceber Iceber force-pushed the fix_preload_pause_image branch from a127acf to 97c87d8 Compare December 4, 2023 05:58
@Iceber Iceber marked this pull request as ready for review December 4, 2023 07:19
Copy link
Contributor

@adisky adisky left a comment

Choose a reason for hiding this comment

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

@Iceber how the images are preloaded in the containerd? as i know the image tars are loaded in the content store and then imported using ctr.

how other containerd cri specific labels are get added to it? like this one io.cri-containerd.image=managed? I think the pinned label should get added in the same way

edit: after going through the changes you are doing the same, I was wondering if the image leak problem could be solved the same way? UpdateImage is also gets called when containerd service is restarted, if the user has updated the sandbox image they definitely need to restart the containerd service and during that we can cleanup the pinned image label

func (c *criService) loadImages(ctx context.Context, cImages []containerd.Image) {

@Iceber
Copy link
Member Author

Iceber commented Dec 22, 2023

how the images are preloaded in the containerd? as i know the image tars are loaded in the content store and then imported using ctr.

yes, sometimes the pause image exists before containerd restarts, and containerd may restart for various reasons, such as a manual restart or an upgrade restart.

Regardless of whether pre-existing (preloaded) pause image have the io.cri-containerd.image=managed label or not, they will not be pinned due to a bug. This pr is to ensure that paused images are correctly labelled as pinned after containerd reboot, or if they are loaded/pulled via ctr.

@Iceber
Copy link
Member Author

Iceber commented Dec 22, 2023

if the user has updated the sandbox image they definitely need to restart the containerd service and during that we can cleanup the pinned image label

Does this mean clean up the pinned label of the old pause?

@adisky
Copy link
Contributor

adisky commented Dec 22, 2023

if the user has updated the sandbox image they definitely need to restart the containerd service and during that we can cleanup the pinned image label

Does this mean clean up the pinned label of the old pause?

yes, the current UpdateImage won't work, maybe a new function while containerd restarts to cleanup old pause image

@Iceber
Copy link
Member Author

Iceber commented Dec 22, 2023

@adisky What are your thoughts on identifying the old pause image?
Regarding unpin's old pause, #8866 has something like

@adisky
Copy link
Contributor

adisky commented Jan 2, 2024

@Iceber I am ok with the approach but as the feedback from that PR it would be good if we can have a consistent approach so suggested if we can try this

edit: after going through the changes you are doing the same, I was wondering if the image leak problem could be solved the same way? UpdateImage is also gets called when containerd service is restarted, if the user has updated the sandbox image they definitely need to restart the containerd service and during that we can cleanup the pinned image label

@k8s-ci-robot
Copy link

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Iceber Iceber closed this Feb 27, 2024
@Iceber Iceber deleted the fix_preload_pause_image branch February 27, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants