Add image pruning

[ncdc]

No description provided.

[smarterclayton]

style note, this really doesn't have anything to do with pruning controller. Just make it a method addImagesToGraph

[ncdc]

I considered that, but I need c.registryURL. Do you want me to pass that in?

[smarterclayton]

Create

type PruneImagesAlgorithm struct {
// things needed for prune
}

func (PruneImagesAlgorithm) Run() {}

I'd like the traversal code be separate from the graph setup code separate from the actual "things to delete". For instance, creating the image graph is one step NewPruneImagesAlgorithm(), you call Run(fn func(Image)) or something and return a count of the number of images affected.

----- Original Message -----

•   return fmt.Errorf("error listing all replication controllers: %v", err)
  

• }

• g := graph.New()
• c.addImagesToGraph(g, images)
• c.addImageStreamsToGraph(g, streams)
• c.addPodsToGraph(g, pods)
• c.addReplicationControllersToGraph(g, rcs)
• c.prune(g)

• return nil
  +}

+const minimumImageAgeInMinutesToPrune = 60
+
+func (c *PruningController) addImagesToGraph(g graph.Graph, images
*imageapi.ImageList) {

I considered that, but I need c.registryURL. Do you want me to pass that in?

---

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1841/files#r28803613

[ncdc]

so would you do this?

type ImagePruner struct {
  g graph.Graph
  images client.ImageInterface
  // ...
}

And what would the func that takes an Image that you pass to Run do?

[smarterclayton]

----- Original Message -----

•   return fmt.Errorf("error listing all replication controllers: %v", err)
  

• }

• g := graph.New()
• c.addImagesToGraph(g, images)
• c.addImageStreamsToGraph(g, streams)
• c.addPodsToGraph(g, pods)
• c.addReplicationControllersToGraph(g, rcs)
• c.prune(g)

• return nil
  +}

+const minimumImageAgeInMinutesToPrune = 60
+
+func (c *PruningController) addImagesToGraph(g graph.Graph, images
*imageapi.ImageList) {

so would you do this?

type ImagePruner struct {
  g graph.Graph
  images client.ImageInterface
  // ...
}

And what would the func that takes an Image that you pass to Run do?

I don't think the Pruner needs to know about client.ImageInterface - the Function you pass to run would do. That way you can dry run without having to change the algorithm.

So a set of stages:

1. Need to load and fetch objects and add them to a graph
2. Identify any images that aren't referenced by the graph
3. Delete those images

But you could replace #3 with print the images out. Or replace #3 with a function that deletes the images AND removes them from an IR. Or removes them from the IR, and then deletes them later.

It's important to keep that last step separate from the first two.

---

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1841/files#r28806542

[ncdc]

Something along these lines?

type ImagePruner interface {
  ListPrunableImages() []*imageapi.Image
}

type imagePruner struct {
  g graph.Graph
}

func NewImagePruner(images client.ImageInterface, streams ...) ImagePruner {
  g := graph.New()
  addImagesToGraph(g, images)
  // ...
  return &imagePruner{g:g}
}

[ncdc]

Or should NewImagePruner take the actual lists of images, streams, pods, rcs that have been retrieved elsewhere?

[smarterclayton]

Yeah, that's probably the easiest way. Yo could configure your own graph or call the NewMethod.

----- Original Message -----

•   return fmt.Errorf("error listing all replication controllers: %v", err)
  

• }

• g := graph.New()
• c.addImagesToGraph(g, images)
• c.addImageStreamsToGraph(g, streams)
• c.addPodsToGraph(g, pods)
• c.addReplicationControllersToGraph(g, rcs)
• c.prune(g)

• return nil
  +}

+const minimumImageAgeInMinutesToPrune = 60
+
+func (c *PruningController) addImagesToGraph(g graph.Graph, images
*imageapi.ImageList) {

Or should NewImagePruner take the actual lists of images, streams, pods, rcs
that have been retrieved elsewhere?

---

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1841/files#r28809750

[bparees]

Can you summarize somewhere the overall pruning strategy? what gets pruned, what is sufficient to prevent something from being pruned, etc?

It looks like any image that's the current image for a tag won't be pruned, but it's less clear how running pods and RCs are intended to factor in (looks like they keep images around). What about a BuildConfig that references a particular ImageStreamImage? Should also note that pruning old images could make it impossible to rerun a previous build since the exact image it used won't be available anymore (not suggesting we shouldn't prune those, but it's a ramification that should be doc'd)

[ncdc]

The description is here: https://trello.com/c/dWeQNKRL

BuildConfigs are not taken into account at the moment. Should they be?

Sent from my iPhone

On Apr 27, 2015, at 10:04 PM, Ben Parees notifications@github.com wrote:

Can you summarize somewhere the overall pruning strategy? what gets pruned, what is sufficient to prevent something from being pruned, etc?

It looks like any image that's the current image for a tag won't be pruned, but it's less clear how running pods and RCs are intended to factor in (looks like they keep images around). What about a BuildConfig that references a particular ImageStreamImage? Should also note that pruning old images could make it impossible to rerun a previous build since the exact image it used won't be available anymore (not suggesting we shouldn't prune those, but it's a ramification that should be doc'd)

—
Reply to this email directly or view it on GitHub.

[bparees]

seems like they should be. Again, ImageStream and ImageStreamTag references from BuildConfigs are presumably already covered by the "image that represents the the current state of a tag in a stream" rule, but at a minimum an ImageStreamImage reference from a BuildConfig should prevent pruning.

What we want to do about Builds that consumed a particular Image is less clear to me, but it seems like at least an option to not prune those images would be a good idea. or better, not pruning them by default.

[ncdc]

Let's discuss build configs soon.

If we have a plan for pruning builds, I'm fine including them in the criteria for image pruning.

Sent from my iPhone

On Apr 27, 2015, at 10:17 PM, Ben Parees notifications@github.com wrote:

seems like they should be. Again, ImageStream and ImageStreamTag references from BuildConfigs are presumably already covered by the "image that represents the the current state of a tag in a stream" rule, but at a minimum an ImageStreamImage reference from a BuildConfig should prevent pruning.

What we want to do about Builds that consumed a particular Image is less clear to me, but it seems like at least an option to not prune those images would be a good idea. or better, not pruning them by default.

—
Reply to this email directly or view it on GitHub.

[bparees]

addBuild* methods look right to me.

[pmorie]

This is the right place to have a high-level comment describing the lack of a delete API in the registry and explaining why this is factored like it is.

[smarterclayton]

Why this change?

[ncdc]

I thought it was needed for something I was working on a while ago, but it's not - I can undo it.

[smarterclayton]

Not a lot of comments

[ncdc]

@smarterclayton did you look at my changes to Godeps?

[smarterclayton]

Yes, nothing particularly jumped out as bad.

[ncdc]

Cool, thanks. I'm working on a bit more cleanup (to hopefully get rid of the retries when updating the image streams).

[ncdc]

[test]

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2331/)

[ncdc]

@smarterclayton ok to merge so QE can start testing? or perhaps a fork ami?

[derekwaynecarr]

This was lgtm as well. Forgot to comment as such.

Sent from my iPhone

On May 21, 2015, at 8:55 PM, Andy Goldstein notifications@github.com wrote:

@smarterclayton ok to merge so QE can start testing? or perhaps a fork ami?

—
Reply to this email directly or view it on GitHub.

[smarterclayton]

Go ahead and merge, there really no negative impact.

On May 21, 2015, at 8:55 PM, Andy Goldstein notifications@github.com wrote:

@smarterclayton ok to merge so QE can start testing? or perhaps a fork ami?

—
Reply to this email directly or view it on GitHub.

[ncdc]

Ok, hopefully my changes to the registry to support pruning don't have any adverse effects. I've been testing locally and haven't had any issues pushing/pulling images.

[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2055/) (Image: devenv-fedora_1597)

[openshift-bot]

Evaluated for origin up to 05d1dae