Skip to content
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

Merged
merged 46 commits into from
Nov 22, 2024
Merged

Enable Multi DB #20305

merged 46 commits into from
Nov 22, 2024

Conversation

Pan-XT
Copy link
Contributor

@Pan-XT Pan-XT commented Sep 19, 2024

Why I did it

Enable Multi DB for improving routing loading performance. The detail performancee data could be found in the following HLD
[HLD] MultiDB HLD
sonic-net/SONiC#1855

How I did it

modify [dockers/docker-database/Dockerfile.j2] and add [multi_database_config.json.j2]
modify [dockers/docker-database/docker-database-init.sh] to enable Multi DB if /etc/sonic/enable_multidb exists and ensure that the redis service has the necessary permissions to access /var/lib/$inst
modify [Makefile.work] [slave.mk] [files/build_templates/sonic_debian_extension.j2 ] for setting ENABLE_MULTIDB=y when building SONiC image.

some fix PR for multiple database process, otherwise redis instance cannot be launched correctly.
#20726 #20545 sonic-net/sonic-swss#3292

How I verified it

sonic-net/sonic-mgmt#15084

@Pan-XT Pan-XT requested a review from lguohan as a code owner September 19, 2024 17:47
@eddieruan-alibaba
Copy link
Collaborator

Double committed to Phoenixwing eddieruan-alibaba@9f9f4d8

and verified on vSONiC

admin@PE3:~$ ps aux | grep redis
message+ 1011 1.4 0.2 86764 22372 pts/0 Sl 04:30 0:31 /usr/bin/redis-server 127.0.0.1:6379
message+ 1012 0.6 0.2 77548 22316 pts/0 Sl 04:30 0:14 /usr/bin/redis-server 127.0.0.1:6380
message+ 1015 0.5 0.2 64748 17696 pts/0 Sl 04:30 0:11 /usr/bin/redis-server 127.0.0.1:6381
message+ 1016 1.9 0.3 71404 22532 pts/0 Sl 04:30 0:43 /usr/bin/redis-server 127.0.0.1:6382
message+ 1018 0.2 0.2 62188 16456 pts/0 Sl 04:30 0:05 /usr/bin/redis-server 127.0.0.1:6383

@eddieruan-alibaba
Copy link
Collaborator

/azpw ms_conflict

@eddieruan-alibaba
Copy link
Collaborator

@StormLiangMS, @liushilongbuaa Can you trigger the following command?

Github Branch not ready
Please wait a few minutes to run again!
'/azpw ms_conflict'

Copy link
Collaborator

@eddieruan-alibaba eddieruan-alibaba left a comment

Choose a reason for hiding this comment

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

This change is enabled in Phoenix wing and verified in Phoenix wing's daily sanity.

http://phoenixwing.com.cn/vsonic

@eddieruan-alibaba
Copy link
Collaborator

Validated with phoenix wing vsonic image

admin@PE3:$ ls -l /etc/sonic/enable_multidb
-rw-r--r-- 1 root root 0 Oct 11 01:46 /etc/sonic/enable_multidb
admin@PE3:
$ ps aux | grep redis
message+ 990 0.8 0.5 86764 39228 pts/0 Sl 06:28 7:06 /usr/bin/redis-server 127.0.0.1:6379
message+ 991 0.8 0.2 71404 21248 pts/0 Rl 06:28 6:50 /usr/bin/redis-server 127.0.0.1:6380
message+ 992 0.3 0.2 64748 17384 pts/0 Sl 06:28 2:28 /usr/bin/redis-server 127.0.0.1:6381
message+ 994 2.9 0.3 71404 24000 pts/0 Sl 06:28 23:44 /usr/bin/redis-server 127.0.0.1:6382
message+ 996 0.1 0.2 62188 16904 pts/0 Sl 06:28 1:22 /usr/bin/redis-server 127.0.0.1:6383
admin 111122 0.0 0.0 6868 648 pts/0 S+ 19:51 0:00 grep redis

@@ -1,5 +1,6 @@
{% if INSTANCES %}
{% for redis_inst, redis_items in INSTANCES.items() %}

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this unnecessary change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

#Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Does changing {%- endfor %} to {% endfor %} work? That will make the intention a bit clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

and revert chmod -f 0760 $sock_file
@yuezhoujk
Copy link

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yuezhoujk
Copy link

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yuezhoujk
Copy link

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kperumalbfn
Copy link
Contributor

@Pan-XT Could you add this build option ENABLE_MULTIDB in the multi DB HLD doc?

@kperumalbfn
Copy link
Contributor

@Pan-XT Could you also sonic-mgmt tests for multi DB. Though this option is not enabled by default, will be good to have these tests in sonic-mgmt.

@eddieruan-alibaba
Copy link
Collaborator

@Pan-XT Could you also sonic-mgmt tests for multi DB. Though this option is not enabled by default, will be good to have these tests in sonic-mgmt.

@kperumalbfn he replied it before. sonic-net/sonic-mgmt#15084. We need to build a new image with this compile flag is enabled, then reuse existing test cases. No need to create new one since we don't expect any differences except performance gain.

@Pan-XT
Copy link
Contributor Author

Pan-XT commented Nov 16, 2024

@Pan-XT Could you add this build option ENABLE_MULTIDB in the multi DB HLD doc?

I raise a PR sonic-net/SONiC#1855

@Pan-XT
Copy link
Contributor Author

Pan-XT commented Nov 19, 2024

@lguohan

Can you help to review this PR, I have already updated the description based on your suggestion

Copy link
Contributor

@kperumalbfn kperumalbfn left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Please update the HLD with the performance numbers.

@kperumalbfn kperumalbfn merged commit 9e888d4 into sonic-net:master Nov 22, 2024
22 checks passed
},
"redis1": {
"hostname" : "{{HOST_IP}}",
"port" : 6378,
Copy link
Contributor

Choose a reason for hiding this comment

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

pls note that env REDIS_PORT/BMP_DB_PORT need to be reserved, and port > 6400 is not used currently.

@FengPan-Frank
Copy link
Contributor

also some fix PR for multiple database process, otherwise redis instance cannot be launched correctly.

#20545
#20726

@Pan-XT
Copy link
Contributor Author

Pan-XT commented Nov 22, 2024

also some fix PR for multiple database process, otherwise redis instance cannot be launched correctly.

#20545 #20726

Thanks for the suggestion. These PRs are merged and I link them in description.

VladimirKuk pushed a commit to Marvell-switching/sonic-buildimage that referenced this pull request Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants