feature(redis) support redis sentinel #11155

bitsf · 2020-03-20T07:01:26Z

Add redis sentinel support

change the redis conf, remove port field, add schema field, change host field format to

for schema redis
hostname:post
for schema redis+sentinel
hostname1:port,hostname2:port,hostname3:port/monitor_name

the full raw url is
redis://<hostname>:<post>/<dbnum>
redis+sentinel://<hostname1>:<port>,<hostname2>:<port>,<hostname2>:<port>/<monitor_name>/<dbnum>

Signed-off-by: Ziming Zhang zziming@vmware.com
Change-Id: Icbcbb5ebe842c4866657e32d5a253f293e643854

codecov · 2020-03-20T07:05:37Z

Codecov Report

Merging #11155 into master will decrease coverage by 0.24%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11155      +/-   ##
==========================================
- Coverage   63.64%   63.40%   -0.25%     
==========================================
  Files         878      880       +2     
  Lines       54133    54289     +156     
  Branches     1836     1836              
==========================================
- Hits        34455    34422      -33     
- Misses      16100    16286     +186     
- Partials     3578     3581       +3

Flag	Coverage Δ
#unittests	`63.40% <ø> (-0.25%)`	⬇️

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

Impacted Files	Coverage Δ
...ithub.com/goharbor/harbor/src/chartserver/cache.go	`55.68% <0.00%> (-9.66%)`	⬇️
...ithub.com/goharbor/harbor/src/chartserver/utils.go	`75.94% <0.00%> (-8.80%)`	⬇️
...com/goharbor/harbor/src/jobservice/runner/redis.go	`63.63% <0.00%> (-4.05%)`	⬇️
...harbor/harbor/src/jobservice/common/utils/utils.go	`54.38% <0.00%> (-2.95%)`	⬇️
...om/goharbor/harbor/src/jobservice/config/config.go	`51.75% <0.00%> (-2.56%)`	⬇️
...oharbor/harbor/src/jobservice/runtime/bootstrap.go	`59.05% <0.00%> (-2.14%)`	⬇️
...or/harbor/src/server/middleware/repoproxy/proxy.go	`7.14% <0.00%> (-0.23%)`	⬇️
.../goharbor/harbor/src/controller/blob/controller.go	`74.82% <0.00%> (ø)`
...m/goharbor/harbor/src/common/utils/redis/helper.go
.../goharbor/harbor/src/chartserver/redis_sentinel.go	`1.57% <0.00%> (ø)`
... and 6 more

make/photon/prepare/templates/registry/config.yml.jinja

src/core/main.go

reasonerjt · 2020-03-22T06:19:17Z

This is a valuable PR thanks @bitsf !

But I have some comment in regards to how we set the redis/redis+sentinel settings into core.
Instead of adding magics we may better introduce a more structural way to set these settings.

steven-zou

comments left

make/photon/prepare/utils/configs.py

src/core/api/health.go

src/core/main.go

src/internal/cache/redisclient.go

tools/migration/cfg/migrator_2_0_0/__init__.py

src/internal/cache/redisclient.go

heww

Please rename internal/cache to lib/redis because there is no internal in the harbor now and cache it does not match the function of this pkg.

src/common/utils/redis/helper.go

heww · 2020-07-15T04:55:50Z

src/jobservice/job/impl/gc/garbage_collection.go

+		PoolMaxIdle:           0,
+		PoolMaxActive:         1,
+		PoolIdleTimeout:       60 * time.Second,
+		DialConnectionTimeout: dialConnectionTimeout,
+		DialReadTimeout:       dialReadTimeout,
+		DialWriteTimeout:      dialWriteTimeout,


Can we unify the Redis param in harbor core?

This PR is focus on support redis sentinel, so I don't want to optimize the param too much for each components, as this might introduce new issues.

ninjadq · 2020-07-17T07:49:50Z

make/photon/prepare/utils/chart.py

 from g import templates_dir, config_dir, data_dir, DEFAULT_UID, DEFAULT_GID
+from urllib.parse import urlsplit


standard lib should be placed in the first import group

ninjadq · 2020-07-17T07:55:28Z

make/photon/prepare/utils/chart.py

+    elif u.scheme == 'redis+sentinel':
+        return {
+            'cache_store': 'redis_sentinel',
+            'cache_redis_mastername': u.path.split('/')[1],


master isn't political right.

redis itself is using words master/slave

ninjadq · 2020-07-17T07:58:38Z

make/harbor.yml.tmpl

 #   password:
+#   # sentinel_master_set must be set to support redis+sentinel
+#   #sentinel_master_set:


master should changed to main or something else

ninjadq · 2020-07-17T08:02:05Z

make/photon/prepare/utils/configs.py

 from g import versions_file_path, host_root_dir, DEFAULT_UID, INTERNAL_NO_PROXY_DN
+from models import InternalTLS
+from urllib.parse import urlencode


urllib shuold placed in system library import group

ninjadq · 2020-07-17T08:04:07Z

make/photon/prepare/utils/configs.py

        'password': '',
    }
    kwargs.update(redis or {})
-    kwargs['db'] = db
+    kwargs['scheme'] = kwargs.get('sentinel_master_set', None) and 'redis+sentinel' or 'redis'


word 'master' should change.

'None' seems useless here.

ninjadq · 2020-07-17T08:29:51Z

make/photon/prepare/utils/configs.py

-    if kwargs['password']:
-        return "redis://anonymous:{password}@{host}:{port}/{db}".format(**kwargs)
-    return "redis://{host}:{port}/{db}".format(**kwargs)
+    return "{scheme}://{password_part}{host}{sentinel_part}{db_part}".format(**kwargs) + get_redis_url_param(kwargs)


recommend only store content in variables like 'db_part', 'sentinel_part', 'password_part'. And the delimiter like/ ,@ can rendered in format string.

ninjadq · 2020-07-17T08:33:57Z

make/photon/prepare/utils/configs.py

+
+def get_redis_url_param(redis=None):
+    s = {}
+    if 'idle_timeout_seconds' in redis:


This command will raise an Exception instead of go to the false branch if redis is None

ninjadq · 2020-07-17T08:35:26Z

make/photon/prepare/utils/configs.py

+
+
+def get_redis_url_param(redis=None):
+    s = {}


it's better use a meaningful variable name

ninjadq · 2020-07-17T08:40:59Z

make/photon/prepare/utils/misc.py

 from g import DEFAULT_UID, DEFAULT_GID, host_root_dir
+from pathlib import Path


move pathlib to system lib import group

Signed-off-by: Ziming Zhang <zziming@vmware.com>

bitsf force-pushed the support_redis_sentinel branch from 5f69094 to 2235db0 Compare March 20, 2020 07:05

bitsf force-pushed the support_redis_sentinel branch 3 times, most recently from 1f51deb to 9b94732 Compare March 20, 2020 10:30

bitsf requested review from steven-zou, ninjadq and reasonerjt March 20, 2020 11:19

bitsf assigned ninjadq, reasonerjt and steven-zou Mar 20, 2020

bitsf force-pushed the support_redis_sentinel branch 12 times, most recently from a2b29c2 to 3890b87 Compare March 21, 2020 07:57

reasonerjt reviewed Mar 22, 2020

View reviewed changes

make/photon/prepare/templates/registry/config.yml.jinja Outdated Show resolved Hide resolved

reasonerjt reviewed Mar 22, 2020

View reviewed changes

src/core/main.go Outdated Show resolved Hide resolved

steven-zou reviewed Mar 23, 2020

View reviewed changes

ninjadq reviewed Mar 23, 2020

View reviewed changes

tools/migration/cfg/migrator_2_0_0/__init__.py Outdated Show resolved Hide resolved

bitsf force-pushed the support_redis_sentinel branch from 3890b87 to 5bbad65 Compare March 23, 2020 11:30

bitsf force-pushed the support_redis_sentinel branch from 5bbad65 to 27d2646 Compare April 1, 2020 02:14