Add feature to pull-policy for pulling images according to user specified pulling interval (e.g., hourly, daily, weekly, interval=5d4h30m )

[Parthiba-Hazra]

Summary

This is the minimal PR for issue - #1368 .We need to discuss about the correctness and quality of changes. I didn't add any test cases yet as some functionalities might change after some reviews, will add those tests gradually as this PR keeps progressing.

Output

Documentation

Right now it's not documented yet.

• Should this change be documented?
  • Yes

Related

#1368

Resolves #1368

[jjbustamante]

Shouldn't we check the interval first before deciding to fetch from the daemon?

if pull, _ := f.CheckImagePullInterval(name); !pull {
   img, err := f.fetchDaemonImage(name)
   if err == nil {
      // removes it from the image.json file, maybe it was deleted
   }
   return img, err
} 

[Parthiba-Hazra]

I think it's related to this message
I will update it soon.

[jjbustamante]

@Parthiba-Hazra this is looking really good! thanks for your amazing work!!

I think your tests are failing because you need to run make format to fix some formatting issues

You can run make verify in your local environment before pushing, usually we forget about it

[Parthiba-Hazra]

@Parthiba-Hazra this is looking really good! thanks for your amazing work!!

I think your tests are failing because you need to run make format to fix some formatting issues

You can run make verify in your local environment before pushing, usually we forget about it

@jjbustamante currently I'm still working on test cases .. actually getting some errors while running test cases of fetcher_test.go, it's saying it can't connect to docker daemon cause it's not running although it is running. Let me resolve this and add the all necessary test cases ... I will try to finish it withing this week.

[jjbustamante]

What about creating a handler for these methods? I think this ReadImageJSON has a lot of responsibilities:

• It is trying to discover the user's home directory
• it creates the whole folder structure to save the image.json on the disk
• it reads and writes!

type ImagePullPolicyManager struct {
	Logger logging.Logger
}

func (i *ImagePullPolicyManager) Read(path string) (*ImageJSON, error) {
  	imagePath := filepath.Join(path, "image.json")
	// Check if the file exists, if not, create it with minimum JSON
	if _, err := os.Stat(imagePath); os.IsNotExist(err) {
                // We don't need to write on disk, we are reading, right? 
                // We return our default 
		return &ImagePullPolicy{
			Interval: &Interval{
				PullingInterval: "7d",
				LastPrune:       "",
			},
			Image: &ImageData{},
		}, nil
	}
        // do what you are doing today
}

func (i *ImagePullPolicyManager) Write(imagePullPolicy ImagePullPolicy, path string) error {
	// I will move all the marshaling logic into these methods, I don't want the method caller to worry about them
        updatedJSON, err := json.MarshalIndent(imagePullPolicy, "", "    ")
	if err != nil {
		return errors.Wrap(err, "failed to marshal updated records")
	}
	return WriteFile(updatedJSON, filepath.Join(path, "image.json"))
}

func WriteFile(data []byte, path string) error {
        // Try this one, it will create the file if it doesn't exits.
	file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644)
	if err != nil {
		return errors.New("failed to open file: " + err.Error())
	}
	defer file.Close()
	_, err = file.Write(data)
	if err != nil {
		return errors.New("failed to write data to file: " + err.Error())
	}
	return nil
}

[Parthiba-Hazra]

Yes, this looks way better than what I was currently doing.

[Parthiba-Hazra]

@jjbustamante I'm not sure about spawning the manager here. should we create the manager in the caller function of ParsePullPolicy and then pass it to the function?

[jjbustamante]

Yeah, let add some suggestions about this.

First of all, a simplify class diagram of your current implementation looks like this:

classDiagram
    Client o-- ImageFetcher
    Fetcher  ..|>  ImageFetcher
    Fetcher o-- ImagePullChecker 
    ImagePullPolicyManager ..|> ImagePullChecker

    Client : +bool experimental
    Client : +map[string]string registryMirrors
    Client : +string version
    Client : +authn.Keychain

    Client --> ImagePullPolicyManager: Remove it!
    ImagePullChecker ..> Fetcher : Remove it!
  
    class  ImageFetcher {
          <<interface>>
    }

    class Fetcher {
    }

    class ImagePullChecker {
        <<interface>>
        CheckImagePullInterval()
	Read()
	PruneOldImages(f *Fetcher) error
	UpdateImagePullRecord()
        Write()
    }

    class ImagePullPolicyManager {
    }

The first thing I noticed that doesn't look very health is the circular dependency between ImagePullChecker and Fetcher in the method PruneOldImages(f *Fetcher) error, we need to remove that, I noticed what you are trying to do is to load a local image from the daemon to check if the image exists, maybe you can use the imgutil.Local image to do that, but we need to remove that circular dependency.

The other thing I will suggest is:

• You are creating an instance of NewPullPolicyManager(logger) in 4 places right now, I think we can:
  • hold a reference into the Client to an image.ImagePullChecker and add an option to set it up

type Client struct {
   imagePullChecker    image.ImagePullChecker
}

func WithImagePullChecker(i image.ImagePullChecker) Option {
       return func(c *Client) {
               c.imagePullChecker = i
       }
}

• After that, you can move the creation of the PullPolicyManager to the cmd/cmd.go just right before creating the client. Update the method to create the client to use the Option we declared before

imagePullChecker := image.NewPullPolicyManager(logger)
packClient, err := initClient(logger, cfg, imagePullChecker)

func initClient(logger logging.Logger, cfg config.Config, imagePullChecker image.ImagePullChecker) (*client.Client, error) {
 return client.NewClient(client.WithLogger(logger), client.WithExperimental(cfg.Experimental), client.WithImagePullChecker(imagePullChecker))
}

• I will move the ParsePullPolicy method to be part of the ImagePullChecker interface

func (i *ImagePullPolicyManager) ParsePullPolicy(policy string) (PullPolicy, error) {
  // Do what we have today
}

• Because of the previous change, all the commands are broken now, I will update the commands that need to call ParsePullPolicy and passthrough the ImagePullChecker, for example in cmd/cmd.go

 // remember we have the imagePullChecker at this point, because cmd is creating it.
 rootCmd.AddCommand(commands.Build(logger, cfg, packClient, imagePullChecker))

• I will repeat the same thing for all the other commands that are broken

[jjbustamante]

I am not totally convinced about this name, I do not have any suggestion, but not sure. I will try to think about it

[Parthiba-Hazra]

@jjbustamante what about ImagePullPolicyService or ImagePullPolicyHandler

[jjbustamante]

I like ** ImagePullPolicyHandler**. Also, can you move the interface definition to the client file? I think it must be along with the others

[Parthiba-Hazra]

@jjbustamante please one more question- should we need to create a mock of NewPullPolicyManager method for test cases (I think we should) -

func NewPullPolicyManager(logger logging.Logger) ImagePullChecker {
	return &ImagePullPolicyManager{Logger: logger}
}

or can use it directly during tests.
edit - asking for the commands test cases which are currently broken due to requested changes

[Parthiba-Hazra]

I like ** ImagePullPolicyHandler**. Also, can you move the interface definition to the client file? I think it must be along with the others

if we move the ImagePullChecker interface to the client file then there will be a import cycle between client and image package cause Fetcher struct has a field imagePullChecker type ImagePullChecker interface.

[jjbustamante]

Oh! .. ok, let's keep it there for now, we will come back to that later

[codecov]

Codecov Report

Attention: Patch coverage is 53.93258% with 164 lines in your changes are missing coverage. Please review.

Project coverage is 78.95%. Comparing base (6fc092b) to head (a68e9e8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2075      +/-   ##
==========================================
- Coverage   79.72%   78.95%   -0.77%     
==========================================
  Files         176      177       +1     
  Lines       13263    13583     +320     
==========================================
+ Hits        10573    10723     +150     
- Misses       2021     2155     +134     
- Partials      669      705      +36     

Flag	Coverage Δ
os_linux	77.90% <53.94%> (-0.75%)	⬇️
os_macos	75.31% <39.05%> (-1.03%)	⬇️
os_windows	78.35% <53.94%> (-0.76%)	⬇️
unit	78.95% <53.94%> (-0.77%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[jjbustamante]

I think you need to update your fork, and rebase your branch to get the latest dependencies updates

[Parthiba-Hazra]

@jjbustamante codecov/patch demanding some test cases yet.. should we work on those or we can ignore that ?

[edmorley]

Hi! Thank you so much for working on this :-D (I'm the person that asked for this in #1368)

I notice that the implementation here currently supports both:

• named durations (eg hourly, daily, weekly)
• and custom durations (interval=<_d_h_m>)

I wonder if we could simplify things and support only one or the other?

My instinct would be to say the named values would cover 95% of needs and so perhaps we don't need the interval=<_d_h_m> form? (Bearing in mind, the main use-case here is to prevent excessive pulls during CI runs, or to reduce hassle/download requirements when building locally and the upstream builder rapidly changes; both of which are handled by the named hourly, daily, weekly durations. Anyone who needs anything more advanced can use a pull policy of never and handle their own pulling or similar.) Removing interval=<_d_h_m> would also mean the parsing implementation can be simplified, and from an end-user perspective means the config options are more consistent (ie: all words, not some words, some key-value pairs etc.)

Also, I see that a new prune-interval config option was added to control when to remove image entries from the .pack/image.json file. If we were to set an upper bound for the pull interval duration (eg say the interval is not allowed to be longer than N duration), we could potentially simplify things further, and remove prune-interval, and instead set pruning to be that max value?

Combining all of the above, if we were to remove support for interval=<_d_h_m>, then we know the max possible pull-policy duration is weekly, and as such the auto-pruning can simply prune any entry older than a week, meaning we can then remove the prune-interval config option.

Thoughts? :-)

[Parthiba-Hazra]

Hi! Thank you so much for working on this :-D (I'm the person that asked for this in #1368)

I notice that the implementation here currently supports both:

• named durations (eg hourly, daily, weekly)
• and custom durations (interval=<_d_h_m>)

I wonder if we could simplify things and support only one or the other?

My instinct would be to say the named values would cover 95% of needs and so perhaps we don't need the interval=<_d_h_m> form? (Bearing in mind, the main use-case here is to prevent excessive pulls during CI runs, or to reduce hassle/download requirements when building locally and the upstream builder rapidly changes; both of which are handled by the named hourly, daily, weekly durations. Anyone who needs anything more advanced can use a pull policy of never and handle their own pulling or similar.) Removing interval=<_d_h_m> would also mean the parsing implementation can be simplified, and from an end-user perspective means the config options are more consistent (ie: all words, not some words, some key-value pairs etc.)

Also, I see that a new prune-interval config option was added to control when to remove image entries from the .pack/image.json file. If we were to set an upper bound for the pull interval duration (eg say the interval is not allowed to be longer than N duration), we could potentially simplify things further, and remove prune-interval, and instead set pruning to be that max value?

Combining all of the above, if we were to remove support for interval=<_d_h_m>, then we know the max possible pull-policy duration is weekly, and as such the auto-pruning can simply prune any entry older than a week, meaning we can then remove the prune-interval config option.

Thoughts? :-)

@edmorley thank you for your review, sure we can do your cahanges easily, actually I implement this after going through all the maintainer's comments on that issue and I found out this would be the most flexible solution. @jjbustamante @jkutner any thought on this ?

[jjbustamante]

@edmorley thank you for your review, sure we can do your cahanges easily, actually I implement this after going through all the maintainer's comments on that issue and I found out this would be the most flexible solution. @jjbustamante @jkutner any thought on this ?

I am happy with @edmorley suggestions if we can support 95% of the use cases and keep the code simpler, let's go for it!

[Parthiba-Hazra]

@edmorley thank you for your review, sure we can do your cahanges easily, actually I implement this after going through all the maintainer's comments on that issue and I found out this would be the most flexible solution. @jjbustamante @jkutner any thought on this ?

I am happy with @edmorley suggestions if we can support 95% of the use cases and keep the code simpler, let's go for it!

Sure, I'm gonna work on this. Thanks.

[Parthiba-Hazra]

@jjbustamante I think it's not a good idea to call Read and Write under every function. Should we handle it in the Fetch method only? What do you think?