-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Multi DB #20305
base: master
Are you sure you want to change the base?
Enable Multi DB #20305
Changes from 35 commits
aaefb71
c5c3e59
289f8e6
f51867e
42fc418
67a9804
4c6527e
46939a0
5b3c458
2bc006a
614bb83
e42bfc3
f51eedb
2b121de
397c6fa
81fdc48
a220f2c
e53cc25
87aab3e
b158823
8c942d6
bffe653
96b9d14
025031b
c4cf9c3
5981cb4
67c4798
1743065
ca52619
43869e1
43bcec0
8c5405e
2185014
b854771
5c98f54
26d834c
2f7eda0
3b30645
f402859
55808b5
90ace92
de605aa
f947e14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
{% if INSTANCES %} | ||
{% for redis_inst, redis_items in INSTANCES.items() %} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this unnecessary change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is no blank line here, all program:{{ redis_inst }} of critical_processes will be concentrated on one line, which will cause subsequent regular expression matching to fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does changing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing {%- endfor %} to {% endfor %} works. I apply this change instead of adding blank line. |
||
program:{{ redis_inst }} | ||
{%- endfor %} | ||
{%- endif %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,11 @@ mkdir -p /etc/supervisor/conf.d/ | |
if [ -f /etc/sonic/database_config$NAMESPACE_ID.json ]; then | ||
cp /etc/sonic/database_config$NAMESPACE_ID.json $REDIS_DIR/sonic-db/database_config.json | ||
else | ||
HOST_IP=$host_ip REDIS_PORT=$redis_port DATABASE_TYPE=$DATABASE_TYPE BMP_DB_PORT=$BMP_DB_PORT j2 /usr/share/sonic/templates/database_config.json.j2 > $REDIS_DIR/sonic-db/database_config.json | ||
if [ -f /etc/sonic/enable_multidb ]; then | ||
HOST_IP=$host_ip REDIS_PORT=$redis_port DATABASE_TYPE=$DATABASE_TYPE BMP_DB_PORT=$BMP_DB_PORT j2 /usr/share/sonic/templates/multi_database_config.json.j2 > $REDIS_DIR/sonic-db/database_config.json | ||
else | ||
HOST_IP=$host_ip REDIS_PORT=$redis_port DATABASE_TYPE=$DATABASE_TYPE BMP_DB_PORT=$BMP_DB_PORT j2 /usr/share/sonic/templates/database_config.json.j2 > $REDIS_DIR/sonic-db/database_config.json | ||
fi | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Pan-XT any sonic-mgmt tests for multi DB? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
fi | ||
|
||
# on VoQ system, we only publish redis_chassis instance and CHASSIS_APP_DB when | ||
|
@@ -125,6 +129,12 @@ do | |
else | ||
echo -n > /var/lib/$inst/dump.rdb | ||
fi | ||
|
||
chown -R redis:redis /var/lib/$inst | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add comments to this file, help other understand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here are the necessary permissions for enable_multidb, see /etc/supervisor/conf.d/supervisord.conf.j2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @Pan-XT , can you add your comments to this file? that will help other understand the propose of this code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok,I add commnets “the Redis process is operating under the 'redis' user in supervisord and make redis user own /var/lib/$inst inside db container.” There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #Closed |
||
sock_file="$REDIS_DIR/$inst.sock" | ||
if [[ -f $sock_file ]]; then | ||
chgrp -f redis $sock_file && chmod -f 0760 $sock_file | ||
fi | ||
done | ||
|
||
TZ=$(cat /etc/timezone) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,192 @@ | ||
{% set include_remote_db = (REMOTE_DB_IP is defined and REMOTE_DB_PORT is defined) %} | ||
{ | ||
"INSTANCES": { | ||
"redis":{ | ||
"hostname" : "{{HOST_IP}}", | ||
"port" : {{REDIS_PORT}}, | ||
"unix_socket_path" : "/var/run/redis{{DEV}}/redis.sock", | ||
"persistence_for_warm_boot" : "yes" | ||
{% if DATABASE_TYPE is defined and DATABASE_TYPE != "" %} | ||
,"database_type": "{{DATABASE_TYPE}}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't find code in sonic-swss-common to parse and handle 'database_type', is there a PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I search that PR, not find any code read "database_type" Otherwise, sonic-swss-common lib can't handle multi-DB, so please make sure sonic-swss-common also change to handle it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This field was added via It is current SONiC behavior Can you help to check with him to see what the need for this field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #Closed, I will offline sync with Ze |
||
{% endif %} | ||
}, | ||
"redis1": { | ||
"hostname" : "{{HOST_IP}}", | ||
"port" : 6380, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The port selection conflicts with both the chassis DB and Smart Switch DB service and must be changed. Port 6380 is used by the chassis Redis server, while ports 6381-6388 are assigned to the DPU databases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oleksandrivantsiv where does chassis DB's port get defined? This multi-db approach is enabled only on the platform which needs performance and controlled via a compile flag. ENABLE_MULTIDB=y The detail information could be found at https://lists.sonicfoundation.dev/g/sonic-wg-routing/wiki/37961 During the discussion in routing WG, we feel there is NO use case to enable it for chassis based platform for the following reasons.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed with Guahan on this one. 6380 will be kept for Chassis DB. It is not easy to change chassis's port number. We use 6379, 6378, 6377, ... a.k.a going down for new redis instances. @Pan-XT can you update your diff? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update redis1,reids2,redis3,redis4 port number to 6378,6377,6376,6375 |
||
"unix_socket_path" : "/var/run/redis{{DEV}}/redis1.sock", | ||
"persistence_for_warm_boot" : "yes" | ||
{% if DATABASE_TYPE is defined and DATABASE_TYPE != "" %} | ||
,"database_type": "{{DATABASE_TYPE}}" | ||
{% endif %} | ||
}, | ||
"redis2": { | ||
"hostname" : "{{HOST_IP}}", | ||
"port" : 6381, | ||
"unix_socket_path" : "/var/run/redis{{DEV}}/redis2.sock", | ||
"persistence_for_warm_boot" : "yes" | ||
{% if DATABASE_TYPE is defined and DATABASE_TYPE != "" %} | ||
,"database_type": "{{DATABASE_TYPE}}" | ||
{% endif %} | ||
}, | ||
"redis3": { | ||
"hostname" : "{{HOST_IP}}", | ||
"port" : 6382, | ||
"unix_socket_path" : "/var/run/redis{{DEV}}/redis3.sock", | ||
"persistence_for_warm_boot" : "yes" | ||
{% if DATABASE_TYPE is defined and DATABASE_TYPE != "" %} | ||
,"database_type": "{{DATABASE_TYPE}}" | ||
{% endif %} | ||
}, | ||
"redis4": { | ||
"hostname" : "{{HOST_IP}}", | ||
"port" : 6383, | ||
"unix_socket_path" : "/var/run/redis{{DEV}}/redis4.sock", | ||
"persistence_for_warm_boot" : "yes" | ||
{% if DATABASE_TYPE is defined and DATABASE_TYPE != "" %} | ||
,"database_type": "{{DATABASE_TYPE}}" | ||
{% endif %} | ||
}, | ||
"redis_chassis":{ | ||
"hostname" : "redis_chassis.server", | ||
"port": 6380, | ||
"unix_socket_path": "/var/run/redis-chassis/redis_chassis.sock", | ||
"persistence_for_warm_boot" : "yes" | ||
} | ||
{% if include_remote_db %} | ||
,"remote_redis":{ | ||
"hostname" : "{{REMOTE_DB_IP}}", | ||
"port" : {{REMOTE_DB_PORT}}, | ||
"unix_socket_path": "", | ||
"persistence_for_warm_boot" : "yes" | ||
} | ||
{% endif %}, | ||
"redis_bmp":{ | ||
"hostname" : "{{HOST_IP}}", | ||
"port" : {{BMP_DB_PORT}}, | ||
"unix_socket_path" : "/var/run/redis{{DEV}}/redis_bmp.sock", | ||
"persistence_for_warm_boot" : "yes" | ||
} | ||
}, | ||
"DATABASES" : { | ||
"APPL_DB": { | ||
"id": 0, | ||
"separator": ":", | ||
"instance": "redis1" | ||
}, | ||
"ASIC_DB": { | ||
"id": 1, | ||
"separator": ":", | ||
"instance": "redis2" | ||
}, | ||
"COUNTERS_DB": { | ||
"id": 2, | ||
"separator": ":", | ||
"instance": "redis3" | ||
}, | ||
"LOGLEVEL_DB": { | ||
"id": 3, | ||
"separator": ":", | ||
"instance": "redis" | ||
}, | ||
"CONFIG_DB": { | ||
"id": 4, | ||
"separator": "|", | ||
"instance": "redis" | ||
}, | ||
"PFC_WD_DB": { | ||
"id": 5, | ||
"separator": ":", | ||
"instance": "redis3" | ||
}, | ||
"FLEX_COUNTER_DB": { | ||
"id": 5, | ||
"separator": ":", | ||
"instance": "redis3" | ||
}, | ||
"STATE_DB": { | ||
"id": 6, | ||
"separator": "|", | ||
"instance": "redis" | ||
}, | ||
"SNMP_OVERLAY_DB": { | ||
"id": 7, | ||
"separator": "|", | ||
"instance": "redis" | ||
}, | ||
"SYSMON_DB": { | ||
"id": 10, | ||
"separator": "|", | ||
"instance": "redis" | ||
}, | ||
"BMC_DB": { | ||
"id": 12, | ||
"separator": ":", | ||
"instance": "redis4" | ||
}, | ||
"RESTAPI_DB": { | ||
"id": 8, | ||
"separator": "|", | ||
"instance": "redis" | ||
}, | ||
"GB_ASIC_DB": { | ||
"id": 9, | ||
"separator": ":", | ||
"instance": "redis" | ||
}, | ||
"GB_COUNTERS_DB": { | ||
"id": 10, | ||
"separator": ":", | ||
"instance": "redis" | ||
}, | ||
"GB_FLEX_COUNTER_DB": { | ||
"id": 11, | ||
"separator": ":", | ||
"instance": "redis" | ||
}, | ||
"APPL_STATE_DB": { | ||
"id": 14, | ||
"separator": ":", | ||
"instance": "redis" | ||
}, | ||
"CHASSIS_APP_DB" : { | ||
"id" : 12, | ||
"separator": "|", | ||
"instance" : "redis_chassis" | ||
}, | ||
"CHASSIS_STATE_DB" : { | ||
"id" : 13, | ||
"separator": "|", | ||
"instance" : "redis_chassis" | ||
} | ||
{% if DATABASE_TYPE is defined and DATABASE_TYPE == "dpudb" %} | ||
, | ||
"DPU_APPL_DB" : { | ||
"id" : 15, | ||
"separator": ":", | ||
"instance" : {% if include_remote_db %} "remote_redis" {% else %} "redis" {% endif %}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest define a variable for all these dupe template There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, the multi_database_config.json.j2 is consistent with the database_config.json.j2, except for the added content related to redis1,redis2,redis3,redis4. Defining a variable for all these dupe templates will be implemented in future modifications. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #Closed |
||
"format": "proto" | ||
}, | ||
"DPU_APPL_STATE_DB" : { | ||
"id" : 16, | ||
"separator": "|", | ||
"instance" : {% if include_remote_db %} "remote_redis" {% else %} "redis" {% endif %} | ||
}, | ||
"DPU_STATE_DB" : { | ||
"id" : 17, | ||
"separator": "|", | ||
"instance" : {% if include_remote_db %} "remote_redis" {% else %} "redis" {% endif %} | ||
}, | ||
"DPU_COUNTERS_DB" : { | ||
"id" : 18, | ||
"separator": ":", | ||
"instance" : {% if include_remote_db %} "remote_redis" {% else %} "redis" {% endif %} | ||
} | ||
{% endif %}, | ||
"BMP_STATE_DB" : { | ||
"id" : 20, | ||
"separator": "|", | ||
"instance" : "redis_bmp" | ||
} | ||
}, | ||
"VERSION" : "1.0" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,8 @@ dependent_startup=true | |
{% if INSTANCES %} | ||
{% for redis_inst, redis_items in INSTANCES.items() %} | ||
{%- if redis_inst != 'remote_redis' %} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest remove unnecessary change in this file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is the same as #20545 , so I remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #Closed |
||
|
||
[program:{{ redis_inst }}] | ||
{% if redis_items['hostname'] != '127.0.0.1' %} | ||
{%- set ADDITIONAL_OPTS = '--protected-mode no' %} | ||
|
@@ -53,6 +55,7 @@ stderr_logfile=syslog | |
{% endfor %} | ||
{% endif %} | ||
|
||
|
||
[program:flushdb] | ||
command=/bin/bash -c "sleep 300 && /usr/local/bin/flush_unused_database" | ||
priority=3 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pan-XT Could you upload a write-up for this multi-DB including the current issues and usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/dzhangalibaba/SONiC/blob/master/doc/database/multi_database_instances.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#Closed