Redis as alternative storage for container images repository cache #2815

prometherion · 2020-02-03T17:48:08Z

Still a WIP for the feature filled in #2814: allowing redis as an alternative for the container images repository cache.

Providing new flags (--cache-{backend,request-timeout}) in order to allow backend selection and abstract the backend requests timeout (deprecating --memcached-timeout providing backward compatibility along with deprecation notice).

This first contribution aims to scaffold the main file also for future backend cache storages but keeping high backward compatibility to don't break things.

TODO:

squaremo · 2020-02-05T11:04:09Z

👋 This is a pretty substantial contribution in the making, thank you for getting involved!

May I ask, what are the advantages of Redis over memcached here? I have never been very convinced of memcached's suitability (it was something of an arbitrary choice), so I am interested to hear your take on it.

prometherion · 2020-02-05T12:29:49Z

Honestly, there are any personal preferences over memcached or redis: both got their pros and cons, advantages or trade-off. I like redis because of low memory footprint but, in the end, it's just something personal, as preferred flavor :)

2opremio · 2020-02-07T13:30:56Z

@prometherion Thanks a lot for the effort! I will try to review this early next week. In the meantime, would you mind adding an e2e test? extending the fluxctl list-images and image_release tests to execute both on memcached and redis would be a good start.

prometherion · 2020-02-07T17:45:07Z

I'm working on the refactoring to allow test with multiple cache backend but I need to perform first another refactoring regarding the files generated by fluxctl install.

Since I followed the actual implementation, we end up with the following file naming:

{redis,memcache}-dep.yaml
{redis,memcache}-svc.yaml

To allow bats testing we would need various fixtures for each backend because kustomize doesn't allow to use wildcards on resources names

A possible workaround could be output just cache-{dep,svc}.yaml instead of specifying the backend kind.

2opremio · 2020-02-07T18:51:08Z

The Kustomize files in the tests allow templating, in case that helps

.circleci/config.yml

squaremo · 2020-02-12T14:10:22Z

@prometherion Do you mind squashing some of the fixup commits? I'm thinking of Wrong package name and the like.

prometherion · 2020-02-12T17:21:12Z

sure @squaremo: just done!

Morriz · 2020-02-18T18:14:25Z

Really hoping to see this one land! So we can get redis pubsub on new registry images...yay!

prometherion · 2020-02-28T15:32:32Z

@2opremio so, in the end, agreed on discharge the idea of running parallel CircleCI jobs.

Actually, implemented testing of both backing services (redis and memcached):

14_release_image-{memcached,redis}
15_fluxctl_install-{memcached,redis}

If it's ok I'm going to squash commits to keep it cleaner.

2opremio · 2020-02-28T17:06:26Z

Sorry to be picky, but I would factor out the test code and call it using a different backends

prometherion · 2020-02-28T17:08:29Z

No problem :) Going to refactor this

…

On Fri, Feb 28, 2020, 18:06 Alfonso Acosta ***@***.***> wrote: Sorry to be picky, but I would factor out the test code and call it using a different backend indtead — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2815?email_source=notifications&email_token=ACJ7S3WVHPRPGGQQPGAAXRDRFFABHA5CNFSM4KPJKNQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENJH3BQ#issuecomment-592608646>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJ7S3X3Y5DCN7YMYHQASGTRFFABHANCNFSM4KPJKNQA> .

prometherion · 2020-03-02T09:46:42Z

@2opremio just pushed: I moved the test logic inside the lib/{release_image,fluxctl_list}.bash files and just calling them from the single BATS files.

I'm still using prefix backend cache service name strategy since I have to ensure to perform the teardown also in case of errors of a single test.

I would write something like (pseudo-code)

@test "Fluxctl list-workloads on memcached" {
  setup 'memcached'
  warmup
  # Test fluxctl list-services
  run fluxctl --k8s-fwd-ns "${FLUX_NAMESPACE}" list-workloads --namespace "${DEMO_NAMESPACE}"
  list_workloads "${status}" "${output}"
  teardown 'memcached'
}

@test "Fluxctl list-workloads on redis" {
  setup 'redis'
  warmup
  # Test fluxctl list-services
  run fluxctl --k8s-fwd-ns "${FLUX_NAMESPACE}" list-workloads --namespace "${DEMO_NAMESPACE}"
  list_workloads "${status}" "${output}"
  teardown 'redis'
}

But honestly, doesn't look good to me since I wouldn't be able to execute tests per backing service (e.g.: E2E_TESTS='*-redis*' make e2e)

Morriz · 2020-03-17T21:17:37Z

Any updates here?

ianmuscat · 2020-03-31T12:24:39Z

Since Redis offers authentication out of the box, perhaps it might be worth considering allowing users to have the ability to configure Flux to use password-based authentication with Redis. Right now it seems that this value is hardcoded to an empty string

flux/pkg/registry/cache/redis.go

Line 58 in e655c99

Password: "", // no password set

2opremio · 2020-04-09T19:27:44Z

Unfortunately I don't have time for it, sorry.

prometherion · 2020-04-10T07:46:03Z

NP, who's in charge or available to take a look at this?

squaremo

A few nits, but the main change I'm requesting is to remove support for changing the cache from fluxctl install; it's not really meant to contain all options, and it would make this PR simpler*.

If you are (understandably) fatigued by this PR, then no worries -- I still think it is a solid contribution 💯 and I will probably end making these few adjustments and rebasing at some point.

*Though you might have to be arrange the end-to-end tests a little differently.

.circleci/config.yml

chart/flux/README.md

chart/flux/templates/NOTES.txt

squaremo · 2020-04-10T09:38:58Z

chart/flux/templates/memcached.yaml

@@ -1,4 +1,4 @@
-{{- if and (eq .Values.memcached.enabled true) (eq .Values.registry.disableScanning false)  }}
+{{- if and (and (eq .Values.memcached.enabled true) (eq .Values.redis.enabled false)) (eq .Values.registry.disableScanning false) }}


Is there something about go templating that means you have to compare values to booleans, rather than just evaluating them?

I just followed the same comparison applied previously, but we change simplify it if you wish: just tried to don't make too many big diffs to make PR easier to review

squaremo · 2020-04-10T10:04:08Z

cmd/fluxctl/install_cmd.go

@@ -52,6 +52,7 @@ fluxctl install --git-url 'git@github.com:<your username>/flux-get-started' --gi
 		"do not scan container image registries to fill in the registry cache")
 	cmd.Flags().StringVarP(&opts.outputDir, "output-dir", "o", "", "a directory in which to write individual manifests, rather than printing to stdout")
 	cmd.Flags().BoolVar(&opts.AddSecurityContext, "add-security-context", true, "Ensure security context information is added to the pod specs. Defaults to 'true'")
+	cmd.Flags().StringVar(&opts.CacheBackend, "cache-backend", "memcached", "Select the docker images repository cache backend. Defaults to 'memcached'")


Despite the fact that other random flags have found their way in here (I'm looking at you, --add-security-context), I don't think it's necessary for the cache backend to be an option here, especially not while it's experimental.

from an end-user perspective, if I want to give a try to a different cache service, I would really enjoy this flag: furthermore: at first glance, it looks like a Helm alternative and I get your point, probably flux install should just provide a battle-tested installation without so many parameters.

If we want to remove the cache-backend flag, I guess the same fate has to be reserved for add-security-context.

cmd/fluxd/main.go

squaremo · 2020-04-10T10:22:22Z

cmd/fluxd/main.go

@@ -60,7 +59,7 @@ const (

 	// This is used as the "burst" value for rate limiting, and
 	// therefore also as the limit to the number of concurrent fetches
-	// and memcached connections, since these in general can't do any
+	// and cache backend connections, since these in general can't do any


I don't think it's used for Redis though?

indeed, but we could use the same option for the cache.RedisConfig MaxConns property: WDYT?

squaremo · 2020-04-10T10:26:36Z

pkg/install/templates/cache-svc.yaml.tmpl

+apiVersion: v1
+kind: Service
+metadata:
+  name: redis


Wouldn't it be better if this corresponded with the default value for the service name? That way there is less to accidentally get wrong.

(notwithstanding I advise above to remove this from fluxctl install entirely)

keep in mind that removing from fluxctl install will require additional time since I'll have to find a better solution on how to hack the e2e tests to evaluate also the Redis server.

@squaremo

Commented by [@squaremo](fluxcd#2815 (review))

fluxcd#2815 (review)

fluxcd#2815 (comment)

Allowing redis as an alternative for the container images repository cache. Providing new flags (`--cache-{backend,request-timeout}`) in order to allow backend selection and abstract the backend requests timeout (deprecating `--memcached-timeout` providing backward compatibility along with deprecation notice). This first contribution aims to scaffold the main file also for future backend cache storages but keeping high backward compatibility to don't break things. TODO: - Redis unit tests - Redis integration tests - README.md changes - Kustomize changes - Docs update

Refactoring has been requested to avoid the import cycle, also to take adavantage of `integration_test` suite in order to test same cache warmer for each implementations. TODO: - find out how to setup the properly integration-tests: Makefile task seems pointing to a missing bash script or binary

Documenting the additional variables: actually, didn't provided an abstraction since it's just an experimental feature and we don't want to break backward compatibility for users overriding variables.

- Updated the docs for new feature - Updated how to use Redis - Updated fluxctl install command to allow experimental feature - Switching to `cache-{dep,svc}.yaml` for fluxctl install output - Allowing tests per cache backend (using `CACHE_BACKEND` env variable)

Instead of running all tests in parallel, since we got few of them that really requires to interact with the image scanner component, duplicating them instead of putting a parallel build step on CircleCI.

fluxcd#2815 (review)

fluxcd#2815 (comment)

Fixing 374bbed

squaremo · 2020-06-04T16:08:53Z

Rebased this. @prometherion Are you OK if I remove the fluxctl install flag, and possibly make other, minor amendments?

prometherion · 2020-06-05T17:18:12Z

Rebased this. @prometherion Are you OK if I remove the fluxctl install flag, and possibly make other, minor amendments?

I can take care of it because I totally get your point and probably we have to hack a bit tests with kustomize, although that could increase the test complexity :(

Morriz · 2020-07-08T00:33:29Z

Bumping this as it would help us a lot

Morriz · 2020-08-11T13:55:50Z

@prometherion can you maybe take a look at this again? Progression here would help a lot

prometherion · 2020-08-25T12:35:03Z

@prometherion can you maybe take a look at this again? Progression here would help a lot

Should we proceed with this?

I mean, it seems Flux will be deprecated in favor of the GitOps Toolkit, not sure this PR is still relevant.

@squaremo could you give us feedback?

kingdonb · 2021-01-26T17:07:31Z

I see a lot of work has gone into this PR and I think there is some recognition in the discussion here that it is now a longshot for inclusion in a next release of Flux v1.

In the new image reflector controller, if I understand correctly image data are maintained in the ImageRepository custom resource and memcached is no longer used, I am not certain I understand how this scales as usage grows, but it seems it will no longer be a concern of redis vs. memcached. Will that moot the entire issue, or should we continue the discussion here?

kingdonb · 2021-01-28T12:49:58Z

This possibly related discussion has been opened: fluxcd/flux2#802

Deprecate "select latest by build time" for image automation

I have not yet evaluated the design of image-reflector-controller for scalability, but I think the folks who are more deeply embedded than myself have chosen this design to address scalability concerns exposed by flux v1. Especially if you have an opinion about this, if you have time to look, we would appreciate your comment there!

kingdonb · 2021-02-26T22:22:34Z

In the interest of reducing the number of open issues not directly related to supporting Flux v1 in maintenance mode, and respecting you may have moved on already, I will go ahead and close out this issue for now.

If you have a use case for Flux that isn't covered well in the new Flux v2 (which is a total rewrite), we want to hear about it.

If you've been following our development efforts then of course we hope you are able to upgrade, here's more info on how to find support with that: https://fluxcd.io/support/

It looks like the decision has been made, and heavyweight image metadata caches in Flux are going to be a thing of the past soon, (for many users in production this is a thing of the past already!) Thanks for making this contribution. Closing.

prometherion changed the title ~~[WIP] Redis~~ [WIP] Redis as alternative storage for container images repository cache Feb 3, 2020

prometherion force-pushed the issues/2814 branch from ae85df4 to a4d1467 Compare February 6, 2020 09:47

prometherion changed the title ~~[WIP] Redis as alternative storage for container images repository cache~~ Redis as alternative storage for container images repository cache Feb 6, 2020

prometherion force-pushed the issues/2814 branch from 15a7bb8 to 69900f1 Compare February 7, 2020 06:31

prometherion force-pushed the issues/2814 branch from 69900f1 to 19f87b7 Compare February 7, 2020 18:24

2opremio reviewed Feb 7, 2020

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

prometherion force-pushed the issues/2814 branch 4 times, most recently from def506e to dbc1bd1 Compare February 8, 2020 10:18

prometherion requested a review from 2opremio February 8, 2020 10:20

prometherion force-pushed the issues/2814 branch from dbc1bd1 to 78430b3 Compare February 8, 2020 10:38

prometherion force-pushed the issues/2814 branch 2 times, most recently from a1a5c47 to 59768a9 Compare February 12, 2020 17:19

prometherion force-pushed the issues/2814 branch from de8fe29 to 7451f88 Compare February 28, 2020 11:32

j-zimnowoda mentioned this pull request Mar 23, 2020

A customer can upgrade otomi-stack linode/apl-core#55

Closed

squaremo suggested changes Apr 10, 2020

View reviewed changes

prometherion added a commit to prometherion/flux that referenced this pull request May 5, 2020

Request changes

b06f44b

Commented by [@squaremo](fluxcd#2815 (review))

prometherion force-pushed the issues/2814 branch from 51361c3 to b06f44b Compare May 5, 2020 07:36

prometherion added a commit to prometherion/flux that referenced this pull request May 5, 2020

Request changes

374bbed

fluxcd#2815 (review)

prometherion force-pushed the issues/2814 branch from b06f44b to 374bbed Compare May 5, 2020 07:36

prometherion added a commit to prometherion/flux that referenced this pull request May 5, 2020

Using constants for cache backing services

6767d84

fluxcd#2815 (comment)

prometherion added 10 commits June 4, 2020 16:50

Adding experimental feature to Helm charts

1322155

Documenting the additional variables: actually, didn't provided an abstraction since it's just an experimental feature and we don't want to break backward compatibility for users overriding variables.

Limit tests run with different image scanner backing services

6c4f478

Instead of running all tests in parallel, since we got few of them that really requires to interact with the image scanner component, duplicating them instead of putting a parallel build step on CircleCI.

Abstracting tests

aaa798a

Adding Redis to GitHub CI

8e9bc97

Request changes

99d269a

fluxcd#2815 (review)

Using constants for cache backing services

dce8de1

fluxcd#2815 (comment)

Typo

155900f

Fixing 374bbed

squaremo force-pushed the issues/2814 branch from f7463a6 to 155900f Compare June 4, 2020 16:07

This was referenced Oct 5, 2020

Move Fons to maintainer emeritus #3319

Merged

Move Fons to maintainer emeritus fluxcd/helm-operator#547

Merged

kingdonb closed this Feb 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redis as alternative storage for container images repository cache #2815

Redis as alternative storage for container images repository cache #2815

prometherion commented Feb 3, 2020 •

edited

Loading

squaremo commented Feb 5, 2020

prometherion commented Feb 5, 2020

2opremio commented Feb 7, 2020

prometherion commented Feb 7, 2020

2opremio commented Feb 7, 2020

squaremo commented Feb 12, 2020

prometherion commented Feb 12, 2020

Morriz commented Feb 18, 2020

prometherion commented Feb 28, 2020

2opremio commented Feb 28, 2020 •

edited

Loading

prometherion commented Feb 28, 2020 via email

prometherion commented Mar 2, 2020

Morriz commented Mar 17, 2020

ianmuscat commented Mar 31, 2020 •

edited

Loading

2opremio commented Apr 9, 2020

prometherion commented Apr 10, 2020

squaremo left a comment

squaremo Apr 10, 2020

prometherion May 5, 2020

squaremo Apr 10, 2020

prometherion May 5, 2020

squaremo Apr 10, 2020

prometherion May 5, 2020

squaremo Apr 10, 2020

squaremo Apr 10, 2020

prometherion May 5, 2020

squaremo commented Jun 4, 2020

prometherion commented Jun 5, 2020

Morriz commented Jul 8, 2020

Morriz commented Aug 11, 2020

prometherion commented Aug 25, 2020 •

edited

Loading

kingdonb commented Jan 26, 2021

kingdonb commented Jan 28, 2021

kingdonb commented Feb 26, 2021

		@@ -1,4 +1,4 @@
		{{- if and (eq .Values.memcached.enabled true) (eq .Values.registry.disableScanning false) }}
		{{- if and (and (eq .Values.memcached.enabled true) (eq .Values.redis.enabled false)) (eq .Values.registry.disableScanning false) }}

Redis as alternative storage for container images repository cache #2815

Redis as alternative storage for container images repository cache #2815

Conversation

prometherion commented Feb 3, 2020 • edited Loading

squaremo commented Feb 5, 2020

prometherion commented Feb 5, 2020

2opremio commented Feb 7, 2020

prometherion commented Feb 7, 2020

2opremio commented Feb 7, 2020

squaremo commented Feb 12, 2020

prometherion commented Feb 12, 2020

Morriz commented Feb 18, 2020

prometherion commented Feb 28, 2020

2opremio commented Feb 28, 2020 • edited Loading

prometherion commented Feb 28, 2020 via email

prometherion commented Mar 2, 2020

Morriz commented Mar 17, 2020

ianmuscat commented Mar 31, 2020 • edited Loading

2opremio commented Apr 9, 2020

prometherion commented Apr 10, 2020

squaremo 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

squaremo commented Jun 4, 2020

prometherion commented Jun 5, 2020

Morriz commented Jul 8, 2020

Morriz commented Aug 11, 2020

prometherion commented Aug 25, 2020 • edited Loading

kingdonb commented Jan 26, 2021

kingdonb commented Jan 28, 2021

kingdonb commented Feb 26, 2021

prometherion commented Feb 3, 2020 •

edited

Loading

2opremio commented Feb 28, 2020 •

edited

Loading

ianmuscat commented Mar 31, 2020 •

edited

Loading

prometherion commented Aug 25, 2020 •

edited

Loading