)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":31737,"name":"Hirotaka Wakabayashi","email":"hiwkby@yahoo.com","username":"hiwkby"},"change_message_id":"fc4983459b8f76f92d3ad8126acc488ac412ba94","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"a794462f_7f0e9416","updated":"2025-12-17 11:19:26.000000000","message":"Hello mongoose404, thank you for the patch and the detailed description! Yes, the default shm_size is 64MiB and LGTM basically, but I think this PR requires additional unit tests because it changes the signature of existing functions.","commit_id":"710578be4eb63296a2cd26d77188dcd1bf451530"},{"author":{"_account_id":36080,"name":"Erkin Mussurmankulov","display_name":"Eric","email":"mangust404@gmail.com","username":"mongoose404","status":"PS Cloud services employee"},"change_message_id":"5adad27388256a50bdf39cf61ea8fcdd64ac6e6f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"03f4b31b_73456270","in_reply_to":"a794462f_7f0e9416","updated":"2025-12-17 14:44:37.000000000","message":"Hello Hirotaka! Thank you for the review.\n\nI\u0027ve added tests for `docker_util.start_container`. It seems that the patchset #1 were not considering network isolation (the `_create_container_with_low_level_api` call).\n\nI cannot test the case with `network_isolation\u003dTrue` against the real OS setup because our OS envs doesn\u0027t support network isolation yet.\nBut I did test the code on my desktop docker, just removed mocks, run unittest and `_create_container_with_low_level_api` made a real docker API call, and it works fine. \"`docker inspect test`\" command shows `\"ShmSize\": 536870912` as expected.\n\nThe second case in the patchset 2, without network isolation, is also tested against real OS setup in our production with real customers PostgreSQL databases, and it works fine.\n\nChanges in rebuild api call argument is already covered by `BuiltInstanceTasksTest::test_upgrade`.\n\nPlease let me know if additional unit tests are required for the patch.","commit_id":"710578be4eb63296a2cd26d77188dcd1bf451530"},{"author":{"_account_id":31737,"name":"Hirotaka Wakabayashi","email":"hiwkby@yahoo.com","username":"hiwkby"},"change_message_id":"e44430dc1d85bb101948117ea248c82c49ec64c4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"2ab3bb64_c679a99e","updated":"2025-12-18 12:20:19.000000000","message":"Hello mangoose404, Thanks for updating the patch! — LGTM. I plan to merge it after making the following small adjustments. The goal is to preserve existing behavior while enabling a performance improvement for PostgreSQL users.\n\n#1 Add a configuration switch to control whether we pass Docker’s shm_size when starting the container (default: disabled). Using shm_size leverages mmap-backed shared memory; for non-PostgreSQL datastores, or PostgreSQL deployments that use SysV shared memory, mmap isn’t always used.\n\n#2 Add a configuration option to set the shared memory size when enabled (default: 64 MiB). This aligns with Docker’s default:\nhttps://github.com/moby/moby/blob/master/daemon/config/config.go#L42\n\nThanks in advance,","commit_id":"b051730bf741d62199087a1f24c05fa24bf7e4a6"},{"author":{"_account_id":26285,"name":"wu.chunyang","email":"wchy1001@gmail.com","username":"wu.chunyang"},"change_message_id":"14fe03ae1a6ce60e3696522c52898ea93c257796","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"50d47772_deeb40be","updated":"2025-12-18 13:31:47.000000000","message":"Hi， thanks for the feedback. I quick reviewed this commit, and it looks like this patch only adds for upgrade and rebuild interface? i didn\u0027t see the value in the build interface? \n\nAlso you use all the memory for the shim Size. but i recommend to use 50% memory as the same as the linux default behavior.(I think 50% should be sufficient)\n\nAdditionally, in this commit, you changed the guest api, this may cause errors when upgrade for an existing instance after you upgrade the Trove taskmanager. if we adopt this commit, you also need to update the RPC version and test if the guest agent support that RPC version before sending RCP call. lastly, I think this value is only relevant to guest agent, so i don\u0027t want to expose this value to our control plane.\n\nAs in trove project, the instance is dedicated for the database container. so i prefer to calculate the shim size by the following formula: int(physical memory / 2) in guest agent side, and we pass this value to \"create_container\" function.  The reasons for this: 1. we don\u0027t need to change the current guest API, 2. It\u0027s more easy for present instances to upgrade(just rebuild the image, update tag, perform rebuild action).","commit_id":"b051730bf741d62199087a1f24c05fa24bf7e4a6"},{"author":{"_account_id":36080,"name":"Erkin Mussurmankulov","display_name":"Eric","email":"mangust404@gmail.com","username":"mongoose404","status":"PS Cloud services employee"},"change_message_id":"f81fb61cd8484e407d8d9711cd830d29b79867d5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"e6ebb2ae_9174c6ef","in_reply_to":"2ab3bb64_c679a99e","updated":"2025-12-18 18:52:43.000000000","message":"Hello Hirotaka!\n\nI hope I did everything right in the last patchset.\n\nNo changes in RPC now, also I\u0027ve added additional tests for start_db method.\n\nPlease let me know if additional changes are required.","commit_id":"b051730bf741d62199087a1f24c05fa24bf7e4a6"},{"author":{"_account_id":26285,"name":"wu.chunyang","email":"wchy1001@gmail.com","username":"wu.chunyang"},"change_message_id":"578da0eb7065e764a6117a96f92bcb5e3121ccb9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"b43d3046_096be811","in_reply_to":"50d47772_deeb40be","updated":"2025-12-18 13:38:03.000000000","message":"This behavior should be safe as the kernel does this by default. And we don\u0027t need to consider safety for Docker, as it only runs a couple of containers.","commit_id":"b051730bf741d62199087a1f24c05fa24bf7e4a6"},{"author":{"_account_id":31737,"name":"Hirotaka Wakabayashi","email":"hiwkby@yahoo.com","username":"hiwkby"},"change_message_id":"a00a20c41ea31f8bd30bfeef72529a7c10043190","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"aaf8f453_26715654","in_reply_to":"a097fccd_368b31b0","updated":"2025-12-19 15:56:09.000000000","message":"Hello Wu, Thanks for the comment! The original approach is very aggressive, since it makes the entire VM memory available to the Docker container. Another idea is to let the Docker container’s shared memory be a percentage of the VM’s memory. For example:\n\n```\n[postgres]\ndocker_image \u003d your-registry/your-repo/postgres\nbackup_docker_image \u003d your-registry/your-repo/db-backup-postgres\ndocker_shm_size_in_ratio \u003d 0.5\n```\n\nHere, `0.5` means the container can use up to 50% of the VM’s memory for shared memory. If the flavor has 1 GB of RAM, the container would start with `--shm-size\u003d512M`.","commit_id":"b051730bf741d62199087a1f24c05fa24bf7e4a6"},{"author":{"_account_id":36080,"name":"Erkin Mussurmankulov","display_name":"Eric","email":"mangust404@gmail.com","username":"mongoose404","status":"PS Cloud services employee"},"change_message_id":"f93aa2cb59a22849713414e1ca6490d5a2a52fab","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"0700af07_64263c18","in_reply_to":"aaf8f453_26715654","updated":"2025-12-19 17:01:33.000000000","message":"Hello, Hirotaka!\n\nIMO the approach with docker_shm_size_in_ratio looks much better than fixed size and better than 100% of the memory.\nAlso you can set it to 0, which will return docker\u0027s default 64M, as backwards compatibility solution.\n\nI will modify the code to utilize this approach asap.","commit_id":"b051730bf741d62199087a1f24c05fa24bf7e4a6"},{"author":{"_account_id":36080,"name":"Erkin Mussurmankulov","display_name":"Eric","email":"mangust404@gmail.com","username":"mongoose404","status":"PS Cloud services employee"},"change_message_id":"d0ec88be06e73571f8d86aeabc26a37edfe2ac77","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"9f9214a3_94ef874b","in_reply_to":"b43d3046_096be811","updated":"2025-12-18 15:40:02.000000000","message":"Hello, Wu! Thank you for the review.\nI made a dedicated config options as Hirotaka specified, so cloud providers may enable and set as many shm_size as they need.\n\nNo need to change RPC and all other stuff too in the last patchset.","commit_id":"b051730bf741d62199087a1f24c05fa24bf7e4a6"},{"author":{"_account_id":26285,"name":"wu.chunyang","email":"wchy1001@gmail.com","username":"wu.chunyang"},"change_message_id":"32eb119c0d5238b7f0c7d2ae536b3b26ceec79b7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"a097fccd_368b31b0","in_reply_to":"e6ebb2ae_9174c6ef","updated":"2025-12-19 15:18:45.000000000","message":"Setting a fixed value for all PostgreSQL database instances is problematic, as no single value can be suitable for all instance flavors.\n\nDocker uses a default /dev/shm size of 64MB mainly to prevent the host OS from being exhausted by one or more containers. However, in the Trove scenario, only the Trove guest agent is allowed to manipulate Docker, and in most cases, the node runs only a single database container.\n\nGiven this deployment model, it is reasonable to allocate /dev/shm up to half of the host memory for the database container, aligning the container behavior with the default Linux tmpfs policy.","commit_id":"b051730bf741d62199087a1f24c05fa24bf7e4a6"},{"author":{"_account_id":36080,"name":"Erkin Mussurmankulov","display_name":"Eric","email":"mangust404@gmail.com","username":"mongoose404","status":"PS Cloud services employee"},"change_message_id":"4e9f20b426bcbd922e0bd589e9decd2ee30f4e40","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"af642b02_5417aaaa","updated":"2025-12-20 12:05:46.000000000","message":"Hello guys! I hope your weekend is going well :-)\n\n\nKey features of the last pachset:\n\n1. Per-datastore ``docker_shm_size_in_ratio`` single configuration option.\n2. No changes in API\u0027s, managers and control plane\u0027s code\n3. Leveraging of `psutil.virtual_memory()` in the guest, the `total` key returns physical ram size (without swap), no need to fetch this data from control plane, this approach seems robust to me, it will also work for rebuild (doesn\u0027t change automatically after flavor resize though, rebuild is need).\n4. Dependency recon: `psutil` library is already present in guest image and it\u0027s a strong dependency for `oslo.utils` and `pyroute2`, so hopefully it will not be removed in the future.\n5. unit tests for `docker_utils.start_container` method\n6. docs \u0026 releasenotes based on Hirotaka\u0027s follow-up patchset\n7. docs says that `0.5` is recommended value, here I agree with Wu, 50% of RAM max seems like a best choice for recommended value. 100% is too much even for some exotic postgresql setups.\n\n\nTests in my dev env:\nFor mysql instance without ``docker_shm_size_in_ratio`` option set:\n```\n  # docker inspect database | grep Shm\n            \"ShmSize\": 67108864,\n```\n\nFor postgresql instance with ``docker_shm_size_in_ratio\u003d0.5``:\n```\n  # docker inspect database | grep Shm\n            \"ShmSize\": 488636416,\n```\n\nTotal memory for 1000 ram flavor is 978673664 (seems like some amount of RAM is reserved by kernel).\n978673664 // (1024*1024) \u003d 933 ; 933 * 0.5 \u003d 466 (M)\n\n\n\nIf some additional changes are required, please let me know.","commit_id":"5e6eeae8d8d65114aab2c4cf73bfa525283fdd4f"},{"author":{"_account_id":36080,"name":"Erkin Mussurmankulov","display_name":"Eric","email":"mangust404@gmail.com","username":"mongoose404","status":"PS Cloud services employee"},"change_message_id":"166717a9b0fd400f452b2d7a45f4bc48c8257b00","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"58bee219_31aa7c48","updated":"2025-12-22 14:27:48.000000000","message":"Hello guys!\n\nI am very sorry, a few additional things are going on in this patchset.\n\nI wanted to create two separate tickets later, but unfortunately they are all connected, so it\u0027s impossible to make work of resize, upgrade and rebuild without fixing these two bugs:\n\n1. in upgrade action there is a error: `datastore_version.name` is passed to guest agent instead of `datastore_version.version`. The error is not visible when your datastore and name are equal, but if datastore.name is \"16.10-rc1\" and datastore.version is \"16.10\", then guest agent on upgrade action will try to pull docker image like `postgresql/16.10-rc1`, which is obviously not exists and it\u0027s a error.\n2. PostgresManager and PgSqlApp were missing `upgrade()` method. I hope it\u0027s not intentionally and it\u0027s a bug, just nobody noticed it before.\n\n\nRecap of the changes in the patchset:\n1. Calculation of shm_size now based on Hirotaka\u0027s request. `Ratio\u003d0` means `shm_mem\u003d64M`, ratio\u003d1.0 means flavor\u0027s ram.\n2. We need to change API for passing flavor ram for rebuild and resize, API version is increased to 1.3 here. As an alternative we may pass flavor ram in guest_info.conf file, but it will lead to additional excessive logic of updating this file on resize, also the solution with guest_info.conf looks less tidy.\n3. rebuild and resize API arguments are combined into single *_info dicts in the same manner as upgrade_info\n4. Additional changes required in resize guest action: re-creating of the database container is a mandatory\n5. Docs were updated accordingly\n\n\nManual tests that I\u0027ve performed on real instances in our dev OS setup:\n```\n|                            | Create | Upgrade | Resize | Rebuild |\n|----------------------------|--------|---------|--------|---------|\n| PostgreSQL 16.10 ratio\u003d0.5 | OK     | OK      | OK     | OK      |\n| MySQL 5.7.50 ratio\u003d0       | OK     | OK      | OK     | OK      |\n```\n\n\nPlease let me know if something requires additional work.","commit_id":"4918feab317cf2a44147302b1fecf722d821b723"},{"author":{"_account_id":31737,"name":"Hirotaka Wakabayashi","email":"hiwkby@yahoo.com","username":"hiwkby"},"change_message_id":"01ffb2e664a57ed60fc237b2e948bebbe6418df7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"ce291667_b963c479","in_reply_to":"4ac08fc7_98ddfb0a","updated":"2025-12-24 09:39:14.000000000","message":"Hello Wu! Thank you very much for the review and the patch! I agree with allowing \"ipc_mode\u003dhost\" as an option, since it gives Trove admins the ability to override the default conservative shared memory settings when needed. I think \"ipc_mode\u003dhost\" should be disabled by default, as it shares the host\u0027s /dev/shm rather than creating container-isolated tmpfs.","commit_id":"4918feab317cf2a44147302b1fecf722d821b723"},{"author":{"_account_id":26285,"name":"wu.chunyang","email":"wchy1001@gmail.com","username":"wu.chunyang"},"change_message_id":"7d09f89eb70671c331dfabe4152aa11377b1804f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"4ac08fc7_98ddfb0a","in_reply_to":"58bee219_31aa7c48","updated":"2025-12-23 12:28:05.000000000","message":"Hello, I really appreciate your update. However, this patch looks much more complex, and I am wondering whether it makes sense to make /dev/shm configurable.\n\nI have submitted a new patch to experiment with /dev/shm sizing here:\nhttps://review.opendev.org/c/openstack/trove/+/971705\n\nI would appreciate your feedback and suggestions. thanks","commit_id":"4918feab317cf2a44147302b1fecf722d821b723"},{"author":{"_account_id":31737,"name":"Hirotaka Wakabayashi","email":"hiwkby@yahoo.com","username":"hiwkby"},"change_message_id":"b813643a0e99de2bf9db5bc089b9d87efab306ad","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"cf51dee8_03fe1d34","in_reply_to":"5ca788c7_878e7294","updated":"2025-12-25 15:52:06.000000000","message":"Thank you for the thoughtful reply. I reviewed the ipc_mode and shm_size settings, and my conclusion is that ipc_mode\u003dhost is the best fit for us. The approach is simple, clear and stable for VM host. The container’s /dev/shm is set to roughly 50% of the VM’s RAM, meaning about half of the VM memory can be used to the container’s /dev/shm.\n\nPreviously, my view was: while I agree that, given our single-tenant model, we should enable what’s needed to maximize performance, I couldn’t understand deliberately removing isolation—especially when more secure technical options exist—just to improve the performance of a particular database because VM hardening should be critical when we do not trust the DB container image.:P","commit_id":"4918feab317cf2a44147302b1fecf722d821b723"},{"author":{"_account_id":26285,"name":"wu.chunyang","email":"wchy1001@gmail.com","username":"wu.chunyang"},"change_message_id":"aaa990c58f04b752d17d36f8c5196a39c45a9083","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"882abf45_127cde33","in_reply_to":"bc86c00d_f6da3cf6","updated":"2025-12-29 01:22:31.000000000","message":"Hi Eric, looking forward to your contribution to KeyDB. If there are no objections, I will complete that patch.","commit_id":"4918feab317cf2a44147302b1fecf722d821b723"},{"author":{"_account_id":26285,"name":"wu.chunyang","email":"wchy1001@gmail.com","username":"wu.chunyang"},"change_message_id":"9f1f20b6ab34be2c32b5ff5ac177478671546b53","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"5ca788c7_878e7294","in_reply_to":"ce291667_b963c479","updated":"2025-12-25 01:51:08.000000000","message":"Hi Hirotaka, Docker is designed for general-purpose workloads, so it creates a private IPC namespace for each container for isolation and safety. However, in our Trove deployment, the guest instance is dedicated to a single database container. In this context, it may be reasonable to set ipc_mode to host so that the database processes behave as if they were running directly on the host system.\n\nAdditionally, I am wondering if there are any known drawbacks to using ipc_mode\u003dhost. If this option were configurable, it seems difficult to manage it per database instance, and it would likely need to be applied at the datastore level.\n\nWhat do you think about this?","commit_id":"4918feab317cf2a44147302b1fecf722d821b723"},{"author":{"_account_id":36080,"name":"Erkin Mussurmankulov","display_name":"Eric","email":"mangust404@gmail.com","username":"mongoose404","status":"PS Cloud services employee"},"change_message_id":"f1dc282a67d34f7eb2daa62cc906536096f20e7d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"bc86c00d_f6da3cf6","in_reply_to":"cf51dee8_03fe1d34","updated":"2025-12-25 19:04:37.000000000","message":"Hello, Hirotaka!\n\nThank you for the concern and the clear opinion.\n\nFrom my perspective as a cloud provider employee, trove shouldn\u0027t provide any *official* database images which are supported and are 100% works, 100% stable and 100% secure, or even pretend that such images exists. Yes, the datastore support matrix exists, but IMHO each cloud provider would and SHOULD build and test their own database images, and the responsibility for the correct work and security issues will take cloud provider to their clients. Responsibility for working or non-working images should NOT be on the trove core dev team. You just give us a proper tools, without any guarantees, as it is also said in the apache license in each file.\n\nSo if we (cloud providers) will face a broken docker database image, which will mess with host\u0027s /dev/shm, we may rebuild that image with some applied patch, or just simply switch to another version which works fine.\nMore than that, something similar already happened in our production Openstack. We have KeyDB manager (as the replacement for Redis because of licensing issues), and the keydb image 6.3.4 is broken. We\u0027ve found it in the \"hard\" way, on some of our client\u0027s production instances. But without production load everything seemed fine on basic tests. We downgraded it to 6.3.3 and it works fine now.\n\nBTW we plan to contribute keydb manager after revamping, when some other dependant patches would be either merged or rejected, the are a dozen of them not sent yet, sorry for that ( ._.)\"\"\nI can fully understand a complexity and responsibility of your decisions here and we\u0027re very grateful for your work guys!\n\np.s. so we\u0027ll stick with Wu\u0027s solution, and I can abandon this patchset, right?","commit_id":"4918feab317cf2a44147302b1fecf722d821b723"}],"trove/guestagent/utils/docker.py":[{"author":{"_account_id":31737,"name":"Hirotaka Wakabayashi","email":"hiwkby@yahoo.com","username":"hiwkby"},"change_message_id":"02d7cbffcbe2a9fd5e6ef67c3deb8e5dfb6f7ce9","unresolved":true,"context_lines":[{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    shm_size \u003d None"},{"line_number":156,"context_line":"    shm_ratio \u003d cfg.get_configuration_property(\u0027docker_shm_size_in_ratio\u0027)"},{"line_number":157,"context_line":"    if shm_ratio \u003e 0:"},{"line_number":158,"context_line":"        shm_size_mb \u003d psutil.virtual_memory().total // units.Mi"},{"line_number":159,"context_line":"        shm_size \u003d f\u0027{int(shm_size_mb * shm_ratio)}M\u0027"},{"line_number":160,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"d866c605_241215fe","line":157,"updated":"2025-12-21 08:50:18.000000000","message":"Hello Eric, thank you for updating the patch. The definition we’re aiming for is: shm_ratio \u003d 0 means 64MB, and shm_ratio \u003d 1 means all memory. The intent can be expressed as:\n\n```\nram_in_mb \u003d flavor[\u0027ram\u0027]\nshm_size \u003d ram_in_mb * ratio + 64 * (1 - ratio)\n```\n\nI like the idea of using psutil for clarity, but since Trove\u0027s docker shared memory is meant to be based on the flavor’s RAM, relying on psutil might make that less obvious. Would it be better to keep it tied directly to the flavor size?","commit_id":"5e6eeae8d8d65114aab2c4cf73bfa525283fdd4f"},{"author":{"_account_id":36080,"name":"Erkin Mussurmankulov","display_name":"Eric","email":"mangust404@gmail.com","username":"mongoose404","status":"PS Cloud services employee"},"change_message_id":"b00059f1b7a570df086def40d07e99bde7c7f1c2","unresolved":false,"context_lines":[{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    shm_size \u003d None"},{"line_number":156,"context_line":"    shm_ratio \u003d cfg.get_configuration_property(\u0027docker_shm_size_in_ratio\u0027)"},{"line_number":157,"context_line":"    if shm_ratio \u003e 0:"},{"line_number":158,"context_line":"        shm_size_mb \u003d psutil.virtual_memory().total // units.Mi"},{"line_number":159,"context_line":"        shm_size \u003d f\u0027{int(shm_size_mb * shm_ratio)}M\u0027"},{"line_number":160,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"24a9a022_743e39d9","line":157,"in_reply_to":"d866c605_241215fe","updated":"2025-12-21 15:25:25.000000000","message":"Yes, sure, I will provide modified patchset tomorrow.","commit_id":"5e6eeae8d8d65114aab2c4cf73bfa525283fdd4f"}]}