Add cache command to cache non-minikube images #2203
Conversation
k8s-ci-robot
added
cncf-cla: yes
Codecov Report
@@ Coverage Diff @@
## master #2203 +/- ##
==========================================
- Coverage 27.75% 27.12% -0.63%
==========================================
Files 82 83 +1
Lines 5538 5684 +146
==========================================
+ Hits 1537 1542 +5
- Misses 3806 3946 +140
- Partials 195 196 +1
Continue to review full report at Codecov.
|
cmd/minikube/cmd/cache.go
Outdated
| Run: func(cmd *cobra.Command, args []string) { | ||
| // Cache and load images into docker daemon | ||
| err := cacheAndLoadImages(args) | ||
| if err != nil { |
There was a problem hiding this comment.
For functions that just return an error like this its preferable to collapse them into a one liner
if err := cacheAndLoadImages(args); err != nil {
...
}
cmd/minikube/cmd/cache.go
Outdated
| return nil, err | ||
| } | ||
| return bootstrapper.NewSSHRunner(client), nil | ||
|
|
cmd/minikube/cmd/cache.go
Outdated
| } | ||
|
|
||
| func cacheAndLoadImages(images []string) error { | ||
|
|
There was a problem hiding this comment.
I think that this is generic enough to put in machine/cache_images.go and export it as CacheAndLoadImages(images []string) error
cmd/minikube/cmd/cache.go
Outdated
| Short: "Delete an image from the local cache.", | ||
| Long: "Delete an image from the local cache.", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
|
|
pkg/minikube/machine/cache_images.go
Outdated
| @@ -98,6 +97,27 @@ func LoadImages(cmd bootstrapper.CommandRunner, images []string, cacheDir string | |||
| return nil | |||
| } | |||
|
|
|||
| // DeleteImages deletes images from local daemon | |||
There was a problem hiding this comment.
Whats the use case for deleting them from the minikube daemon? I think the delete function should just clean up the cache folder on the host if anything.
There was a problem hiding this comment.
my bad, i thought it was meant to delete from the daemon as well. sounds good!
cmd/minikube/cmd/config/config.go
Outdated
| @@ -213,6 +218,79 @@ func configurableFields() string { | |||
| return strings.Join(fields, "\n") | |||
| } | |||
|
|
|||
| // AddToConfigArray adds entries to an array in the config file | |||
| func AddToConfigArray(name string, images []string) error { | |||
There was a problem hiding this comment.
Maybe a map is a better data structure here if you want to maintain a list of keys without duplication.
cmd/minikube/cmd/config/config.go
Outdated
|
|
||
| // Add images to currently existing values in config file | ||
| if values != nil { | ||
| for _, v := range values.([]interface{}) { |
There was a problem hiding this comment.
cast values to a []string, so you don't have to do the casts below.
cmd/minikube/cmd/config/config.go
Outdated
| if values != nil { | ||
| // Add images that are in config file but not in images to finalImages | ||
| // These are the images that should remain in the config file after deletion | ||
| for _, v := range values.([]interface{}) { |
There was a problem hiding this comment.
[]string instead of []interface
cmd/minikube/cmd/config/config.go
Outdated
| return WriteConfig(configFile) | ||
| } | ||
|
|
||
| // DeleteFromConfigArray deletes entries in an array in the config file |
There was a problem hiding this comment.
maybe map[string]interface{} or map[string]struct{} so then you can just
map[key] = struct{}{} for adding
and delete(map, key) for deleting
I think either is fine though. Using a map makes it more complicated for users to edit the config file themselves (offhand, I'm not sure how the marshalled json would look)
There was a problem hiding this comment.
Yah I switched it to a map[string]interface{} and it doesn't look too bad in the json so will probably go with that
cmd/minikube/cmd/config/config.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| values := configFile[name] |
There was a problem hiding this comment.
i think you want a val, ok := configFile[name]. ok is a boolean that is true if the key exists in the map, and false if not.
priyawadhwa
force-pushed
the
cache-images
branch
from
285da7d
to
6652e7a
Compare
| @@ -17,18 +17,19 @@ limitations under the License. | |||
| package machine | |||
There was a problem hiding this comment.
I supposed this never worked, but we never cared about checking the error since it was done on a best-effort basis. But deleting the temp image tarballs in the VM requires sudo
diff --git a/pkg/minikube/machine/cache_images.go b/pkg/minikube/machine/cache_images.go
index ec26c0230..63cf4e40a 100644
--- a/pkg/minikube/machine/cache_images.go
+++ b/pkg/minikube/machine/cache_images.go
@@ -17,7 +17,6 @@ limitations under the License.
package machine
import (
- "golang.org/x/sync/errgroup"
"io/ioutil"
"os"
"os/exec"
@@ -25,6 +24,8 @@ import (
"runtime"
"strings"
+ "golang.org/x/sync/errgroup"
+
"k8s.io/minikube/pkg/minikube/assets"
"k8s.io/minikube/pkg/minikube/bootstrapper"
"k8s.io/minikube/pkg/minikube/config"
@@ -212,7 +213,7 @@ func LoadFromCacheBlocking(cmd bootstrapper.CommandRunner, src string) error {
return errors.Wrapf(err, "loading docker image: %s", dst)
}
- if err := cmd.Run("rm -rf " + dst); err != nil {
+ if err := cmd.Run("sudo rm -rf " + dst); err != nil {
return errors.Wrap(err, "deleting temp docker image location")
}
| @@ -193,6 +194,10 @@ var settings = []Setting{ | |||
| name: "disable-driver-mounts", | |||
| set: SetBool, | |||
| }, | |||
| { | |||
| name: "cache", | |||
There was a problem hiding this comment.
This also needs a set method, since thats what minikube config set uses. Otherwise, it will panic!
diff --git a/cmd/minikube/cmd/config/config.go b/cmd/minikube/cmd/config/config.go
index 15bad2c56..582027d5a 100644
--- a/cmd/minikube/cmd/config/config.go
+++ b/cmd/minikube/cmd/config/config.go
@@ -196,6 +196,7 @@ var settings = []Setting{
},
{
name: "cache",
+ set: SetConfigMap,
setMap: SetMap,
},
}
diff --git a/cmd/minikube/cmd/config/util.go b/cmd/minikube/cmd/config/util.go
index 5a2db1d7a..212771ae3 100644
--- a/cmd/minikube/cmd/config/util.go
+++ b/cmd/minikube/cmd/config/util.go
@@ -20,6 +20,7 @@ import (
"fmt"
"os"
"strconv"
+ "strings"
"github.com/pkg/errors"
"k8s.io/minikube/pkg/minikube/assets"
@@ -65,6 +66,16 @@ func SetMap(m config.MinikubeConfig, name string, val map[string]interface{}) er
return nil
}
+func SetConfigMap(m config.MinikubeConfig, name string, val string) error {
+ list := strings.Split(val, ",")
+ v := make(map[string]interface{})
+ for _, s := range list {
+ v[s] = nil
+ }
+ m[name] = v
+ return nil
+}
+
func SetInt(m config.MinikubeConfig, name string, val string) error {
i, err := strconv.Atoi(val)
if err != nil {
|
Can we get some documentation with this? |
The cache command adds and deletes images through the cache add and cache delete subcommands. The list of cached images is stored as a string array in the config file, so that it can be loaded into the docker daemon on minikube start.
#2065