Move repo store from in memory to redis #333

prydonius · 2017-08-07T17:23:50Z

Adds Zoom Redis library
Adds collection for Repos that is backed by Redis
Removes config.Repo struct and moves to using generated models.Repo

- Adds Zoom Redis library - Adds collection for Repos that is backed by Redis - Removes config.Repo struct and moves to using generated models.Repo

migmartri

What's the reason for moving repositories to redis? Are we persisting its content across deployments? it seems that configure monocular loads the repos in config on every boot? cache.NewCachedRepos(conf.Repos)

Are we planning on storing the charts as well?

migmartri · 2017-08-08T01:00:30Z

docker-compose.yml

@@ -22,3 +22,14 @@ services:
      - $HOME/.kube/:/root/.kube
    environment:
      - ENVIRONMENT=development
+  redis:
+    image: bitnami/redis


Can you use a tagged image?

migmartri · 2017-08-08T01:00:58Z

docker-compose.yml

+    volumes:
+      - 'redis_data:/bitnami/redis'
+    ports:
+      - 6379:6379


is this port needed to be exposed in the host?

I'm exposing it because I was testing through my editor so needed to connect from my host, I think it's useful for that especially given testing locally in Go is very common.

(on a side note, I've been considering whether we should remove Docker Compose entirely and move to Minikube with host mounts and something like telepresence.io for the Go service).

migmartri · 2017-08-08T01:02:56Z

src/api/config/repos/repos.go

@@ -69,3 +63,7 @@ func loadReposFromFile(filePath string) (Repos, error) {
 	}
 	return yamlStruct.Repos, nil
 }
+
+func strToPtr(s string) *string {


are you sure this function does not exist somewhere else already?

It is in the handlers package, which imports repos so it can't be imported here. I'll move this to a new util package that only holds basic functions like this.

migmartri · 2017-08-08T01:07:27Z

src/api/data/cache/repos.go

+// Repos is a Zoom Collection for the Repo model
+var Repos *zoom.Collection
+
+// NewCachedRepos returns a data.Repos object to manage repositories


returns a data.Repos object to manage repositories

Does it?

er.. good catch ;). It did, but I moved to a static variable instead.

migmartri · 2017-08-08T01:14:06Z

src/api/Makefile

@@ -8,7 +8,7 @@ REPO_PATH := github.com/kubernetes-helm/${SHORT_NAME}
 DEV_ENV_IMAGE := quay.io/deis/go-dev:v0.22.0
 SWAGGER_IMAGE := quay.io/goswagger/swagger:0.6.0
 DEV_ENV_WORK_DIR := /go/src/${REPO_PATH}/src/api
-DEV_ENV_PREFIX := docker run --rm -e GO15VENDOREXPERIMENT=1 -v ${CURDIR}:${DEV_ENV_WORK_DIR} -w ${DEV_ENV_WORK_DIR}
+DEV_ENV_PREFIX := docker run --rm -e GO15VENDOREXPERIMENT=1 -v ${CURDIR}:${DEV_ENV_WORK_DIR} -w ${DEV_ENV_WORK_DIR} --net=host


This is interesting, why is this needed?

Rather than change the Makefile to start the test using Docker Compose, I opted to keep this as is and instead make sure a Redis server is running locally. In Travis, I enabled the Redis server and locally I am exporting 6379 from the container. Perhaps this is a bit lazy, and what I should do is change the Makefile to use Compose. What do you think?

prydonius · 2017-08-08T10:22:51Z

What's the reason for moving repositories to redis? Are we persisting its content across deployments? it seems that configure monocular loads the repos in config on every boot? cache.NewCachedRepos(conf.Repos)

This will lay the groundwork for adding repos at runtime via the UI (see #257), each Monocular instance needs a single source of truth for what repos exist. It uses the config to bootstrap the repos, yes - and actually currently it will add them back if they are deleted at runtime. We might want to change that, but I think it is fine for now. I consider the repos in the config file the "default repos" that should always be configured when starting up - which is useful in a CI/CD environment where you don't want to have to manually configure repos in the UI every time.

Are we planning on storing the charts as well?

I think this is the next step, yes. Though we may need to consider the points brought up in #251 (comment)

codecov-io · 2017-08-08T11:52:57Z

Codecov Report

Merging #333 into master will increase coverage by 0.99%.
The diff coverage is 85%.

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
+ Coverage   88.99%   89.98%   +0.99%     
==========================================
  Files          15       18       +3     
  Lines         618      769     +151     
==========================================
+ Hits          550      692     +142     
- Misses         45       48       +3     
- Partials       23       29       +6

Impacted Files	Coverage Δ
src/api/config/repos/repos.go	`100% <ø> (ø)`	⬆️
src/api/mocks/charts.go	`84.26% <100%> (+0.74%)`	⬆️
src/api/data/pointerto/pointerto.go	`100% <100%> (ø)`
src/api/config/cache.go	`100% <100%> (ø)`
src/api/handlers/charts/charts.go	`100% <100%> (+15.25%)`	⬆️
src/api/handlers/releases/releases.go	`100% <100%> (ø)`	⬆️
src/api/handlers/repos/repos.go	`63.63% <50%> (-36.37%)`	⬇️
src/api/data/cache/cache.go	`87.87% <78.94%> (+10.95%)`	⬆️
src/api/data/repos.go	`81.81% <81.81%> (ø)`
src/api/config/cors/cors.go	`100% <0%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e53aad...d5518d6. Read the comment docs.

mkmik · 2017-08-09T00:40:52Z

src/api/config/cache.go

+)
+
+// Pool is a pool of Zoom connections used by other packages
+var Pool *zoom.Pool


so we have a global pool instance that starts uninitialized and gets set every time you call NewRedisPool?

hm, does look a bit surprising given the fact that the constructor is NewRedisPool which is usually how you call functions that create something and return it instead of also having side effects.

Furthermore, the write to the global variable is not protected by a mutex. This will thus fail if you call NewRedisPool and access Pool from different goroutines (it can be easily shown with building the code
/ running the tests with the -race flag).

An simple improvement would be to just make this explicit: InitPool, and possibly also a DestroyPool that will nil the global variable after closing the pool, so that tests cannot accidentally run with a closed pool if they forget to properly initialize the pool.

Another improvement would be to provide an accessor GetPool or Pool() that accesses the global variable protected by a sync.Mutex (or better, by a sync.RWMutex).

Obviously, avoiding global state would be an alternative, but in Go globals are generally less frowned upon, but that doesn't mean that we have to make them racy.

Thanks @mmikulicic, this is definitely the part I was most concerned about with this PR so thanks for the feedback! I agree that InitPool is a better name, and I will move to using an accessor with a mutex.

mkmik · 2017-08-09T00:52:44Z

src/api/data/repos.go

+type Repo models.Repo
+
+// ModelId returns the unique name of the Repo
+func (r *Repo) ModelId() string {


are you sure you cannot name it ModelID (see https://github.com/golang/go/wiki/CodeReviewComments#initialisms) ?

Yeah, linter complained about it too, but unfortunately this is how they are named in Zoom https://github.com/albrow/zoom#what-is-a-model

mkmik · 2017-08-09T00:54:07Z

src/api/data/repos.go

+
+// ModelId returns the unique name of the Repo
+func (r *Repo) ModelId() string {
+	return *r.Name


are you sure you want to crash if r.Name is uninitialized?

An option is to return e.g. "<nil>" if nil; useful so that you don't crash the app you're debugging.

mkmik · 2017-08-09T01:01:11Z

src/api/data/util/util.go

+}
+
+// StrToPtr converts a string to a *string
+func StrToPtr(s string) *string {


Nitpick StrToPtrOfWhat?: obviously of str! hence StrToStrPtr, or perhaps justStrPtr?

or turning this around: why don't we just call the package pointerto, and have pointerto.String, pointerto.Int64?

Whilst creating a package is nice, I can see util having more useful shared functions in the future, so I'll rename this to StrPtr.

creating packages is cheap.

util is a particularly bad name for a package, for a few reasons:

by definition, it means that the bunch of thing you put in there has not much in common besides being generally useful. The family of FooPtr functions here have a lot in common: they take a Foo and return a *Foo, for the purpose of taking the address of a literal which is not allowed by language spec.

without goimports programming in Go would be such a pain. goimports work by assuming that there is signal in the combination of packagename+function signature. It doesn't work well when a lot of different packages have both the same name and same functions (like the often happens with the various log libraries) and obviously it doesn't work well you need code from multiple packages that are named in the same way (something that usually doesn't happen with the aforementioned log libraries). This makes util a choice of last resort, because abusing it will eventually lead to having many util packages spread in your code base and in the libraries you depend on and thus a kitten will die every time you'll have to manually import or manually set an alias of an import and/or rename a bunch of package aliases in a code snippet that you're moving around the codebase just because of a conflict with the util name. Ah, and some people call it utils ... utils.Foo + util.Bar

Fair point, you've convinced me - I'll go for creating a pointerto package, I like that it's more readable to. Thanks!

mkmik · 2017-08-09T01:02:38Z

src/api/handlers/charts/charts_test.go

+	assert.Equal(t, w.Code, http.StatusBadRequest, "expect a 400 response code")
+	var httpBody models.Error
+	assert.NoErr(t, testutil.ErrorModelFromJSON(w.Body, &httpBody))
+	assert.Equal(t, *httpBody.Code, int64(400), "response code in HTTP body data")


is this int64(..) really needed for readability? Constant are automatically interpreted to be of the required type.

This appeared to be failing because it was interpreting 400 as an int and not equating it to *httpBody.Code, but I'll check again.

ah sure, I missed the fact that assert.Equal accepts interface{}. Yeah, the revolt of the helpers...

uses sync.Once to ensure pool and collection is only initialized once

prydonius · 2017-08-09T15:59:26Z

@mmikulicic made all the requested changes - PTAL!

r.e. miniredis, I will move to that in a separate PR to avoid adding more to this one.

mkmik · 2017-08-10T11:33:05Z

src/api/config/cache.go

+}
+
+// CloseRedisPool closes a pool of Zoom connections
+func CloseRedisPool() {


this is not threadsafe.

the sync.Once provides a very simple pattern on top of a mutex but it can only do one thing.

if you also want to safely mutate the pool variable (by setting it to nil) you need to deal with that mutex explicitly.

Another option is to just call close and let it be called multiple times and ignore the error.

Another option again is to just not care about closing the pool, since there is no provision here for reopening it again later (since the once is not reset)

I was considering adding a mutex here, but I agree that the easier option is to just not close the pool.

mkmik · 2017-08-10T11:34:35Z

src/api/config/cache_test.go

+	currentConfig = Configuration{}
+	pool := GetRedisPool()
+	assert.NotNil(t, pool, "Redis Pool")
+	CloseRedisPool()


doing this here means that the moment you add another test into this package it won't work, because closing the pool will not reset the once and thus subsequent calls to GetRedisPool will just return nil.

mkmik · 2017-08-11T08:54:31Z

src/api/data/cache/auto_refresher_test.go

-			Name: "waps",
-			URL:  "./localhost",
+	repos := []models.Repo{
+		models.Repo{


Go allows to avoid repeating the name of the type:

[]models.Repo { { Name: ..., URL: ..., }, }

mkmik · 2017-08-11T08:55:11Z

src/api/data/cache/cache_test.go

-			Name: "stable",
-			URL:  "http://storage.googleapis.com/kubernetes-charts",
+	repos := []models.Repo{
+		models.Repo{


mkmik · 2017-08-11T08:55:23Z

src/api/data/cache/cache_test.go

+func setupTestRepoCache(repos *[]models.Repo) {
+	if repos == nil {
+		repos = &[]models.Repo{
+			models.Repo{


mkmik · 2017-08-11T08:57:38Z

src/api/data/cache/repos.go

+// NewCachedRepos sets up a Zoom collection of repositories
+func NewCachedRepos(repos []models.Repo) {
+	log.Info("setting up Repos collection")
+	var err error


(general comment, not longer relevant if you follow my suggestion above):

if you don't use this err outside of the closure passed to once.Do then please declare it inside of the closure below

mkmik · 2017-08-11T08:59:26Z

src/api/data/cache/repos.go

+var once sync.Once
+
+// NewCachedRepos sets up a Zoom collection of repositories
+func NewCachedRepos(repos []models.Repo) {


This is racy since the Repos global could be access from multiple goroutines in mixed read and write mode.
Furthermore I don't really like deciding here how we want to fail.

Let's rewrite it as:

var ( reposSingleton *zoom.Collection once sync.Once ) func UpdateCache(repos []models.Repo) error { repos, err := GetRepos() if err != nil { return err } for _, r := range repos { // Convert to Zoom model repo := data.Repo(r) err = repos.Save(&repo) if err != nil { return err } } return nil } func GetRepos() (*zoom.Collection, error) { var err error once.Do(func() { reposSingleton, err = config.GetRedisPool().NewCollectionWithOptions(&data.Repo{}, zoom.DefaultCollectionOptions.WithIndex(true)) }) return reposSingleton, err }

prydonius · 2017-08-11T15:00:28Z

There's a bug with the API responses because getRepoObject in handlers is using the repos from configuration and not from Redis. Since the handlers package cannot import the data/cache package (import cycle), I need to do some rearranging here.

mkmik · 2017-08-11T15:11:00Z

There's a bug with the API responses because getRepoObject in handlers is using the repos from configuration and not from Redis. Since the handlers package cannot import the data/cache package (import cycle), I need to do some rearranging here.

yeah, lazy initialization is dangerous

allows helpers package to use the repos collection

mkmik · 2017-08-14T14:00:41Z

src/api/config/repos/repos.go

-	Repo{
-		Name:   "stable",
-		URL:    "https://kubernetes-charts.storage.googleapis.com",
+	models.Repo{


I thought we removed these unnecessary type names here

mkmik · 2017-08-14T14:02:34Z

src/api/data/cache/cache.go

+	if err != nil {
+		return nil, err
+	}
+	repos := []*data.Repo{}


I personally prefer to use:

var repos []*data.Repo

in this case, since it better conveys the idea that it will get filled by FindAll

mkmik · 2017-08-14T14:02:54Z

src/api/data/cache/cache.go

+	if err != nil {
+		return err
+	}
+	repos := []*data.Repo{}


mkmik

ok, generally ok for me, modulo a couple of minor comments above

Move repo store from in memory to redis

a770f39

- Adds Zoom Redis library - Adds collection for Repos that is backed by Redis - Removes config.Repo struct and moves to using generated models.Repo

prydonius requested a review from migmartri August 7, 2017 17:24

Enable Redis in Travis and run test container with host network

f4cd3b1

migmartri reviewed Aug 8, 2017

View reviewed changes

Adnan Abdulhussein added 2 commits August 8, 2017 12:35

fix restapi package tests

3f6f913

small fixes from review

09d3d56

Adnan Abdulhussein added 3 commits August 8, 2017 13:16

move StrToPtr to util package

1011c51

add tests

0626e65

improving coverage

3e30598

prydonius force-pushed the 257-redis-repo-cache branch from e0d6c0d to 3e30598 Compare August 8, 2017 17:03

migmartri approved these changes Aug 8, 2017

View reviewed changes

mkmik reviewed Aug 9, 2017

View reviewed changes

Adnan Abdulhussein added 3 commits August 9, 2017 16:04

move to single pool during tests

545bba6

uses sync.Once to ensure pool and collection is only initialized once

create pointerto package

70add94

don't crash if repo name not set

767ef61

mkmik reviewed Aug 11, 2017

View reviewed changes

don't close redis pool

384cde5

mkmik reviewed Aug 11, 2017

View reviewed changes

Adnan Abdulhussein added 4 commits August 12, 2017 11:06

avoid repeating type in array declaration

28e1ce1

make reposSingleton thread safe

cd78906

reorg repos collection methods

bf157c7

allows helpers package to use the repos collection

fix tests

52bacf7

mkmik reviewed Aug 14, 2017

View reviewed changes

src/api/data/cache/cache.go Outdated

if err != nil {

return err

}

repos := []*data.Repo{}

Copy link

mkmik Aug 14, 2017

Choose a reason for hiding this comment

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

same here

mkmik approved these changes Aug 14, 2017

View reviewed changes

review changes

d5518d6

prydonius merged commit ab03fb7 into helm:master Aug 14, 2017

prydonius deleted the 257-redis-repo-cache branch August 14, 2017 15:04

Move repo store from in memory to redis #333

Move repo store from in memory to redis #333

Conversation

prydonius commented Aug 7, 2017

migmartri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented Aug 8, 2017

codecov-io commented Aug 8, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented Aug 11, 2017

mkmik commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkmik left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 8, 2017 •

edited

Loading

prydonius commented Aug 9, 2017 •

edited

Loading