-
Notifications
You must be signed in to change notification settings - Fork 22
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
Mariadb max_packet_allowed variable exposed #295
Conversation
I think I see what is missing. I'm not accustomed to s6 overlay version 3 yet and can see I'm missing a chunk of definitions to fire. I'll work on filling in the missing bits. Ah learning. ;) |
Okay a little progress on creating the S6 structure. I went ahead to at least give max_packet_allowed a default value of 256MB for when the container is first started (Dockerfile ENV) however confd & s6 don't seem to be "firing" or pulling the value from the project .env file. @nigelgbanks Any pointers on this per se? |
That file is for docker-compose.yml file and is not included in the container environment variables. You'd have to specify the environment variable on the For more information, the relevant docker-compose documentation. I'll submit a pull request that sorts this out and exposes it, following the conventions of the this repo. |
Rather than making a new MR which ignores the work here, can you confirm what elements here are in the right direction please? From your comments sounds like I just need to add the following below to the project My intent isn't to make you redo my work here per se, just to learn and practice enough on how to expose variables properly. mariadb:
image: ${REPOSITORY}/mariadb:${TAG}
environment:
DB_MAX_ALLOWED_PACKET: ${DB_MAX_ALLOWED_PACKET:-268435456} I'll test shortly. Thanks for taking a look! |
Seems to work, the value in the mariadb-server.cnf file now reflects the value within the .env file when changed and if not uses the default 256MB. Are there other settings files or techniques needed? |
Sure, I was opting for a separate pull as every line will change, but I'll put notes on this pull. |
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.
Also rebase on main
to get passing tests as the latest version of docker compose
introduced a breaking change, that has been fixed on main
.
mariadb/Dockerfile
Outdated
@@ -22,5 +22,7 @@ RUN --mount=type=cache,id=mariadb-apk-${TARGETARCH},sharing=locked,target=/var/c | |||
# base image. Set to 10 minutes just incase it ran on very old or overallocated | |||
# hardware. | |||
ENV S6_CMD_WAIT_FOR_SERVICES_MAXTIME=600000 | |||
# Default value of 256 MB in bytes | |||
ENV DB_MAX_ALLOWED_PACKET=268435456 |
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.
Please change to the following, and remove the comment.
ENV MYSQL_MAX_ALLOWED_PACKET=16777216
Prefix to match the other environment variables provided by the image, see the README.md.
Also, change the default value to be the same default as is provided by MariaDB (16777216
) to not change the existing behavior. See the documentation here.
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.
Also add documentation for the new environment variable to the image's README.md.
For example:
Environment Variable | Default | Description |
---|---|---|
MYSQL_ROOT_PASSWORD | The database root user password. Defaults to DB_ROOT_PASSWORD |
|
MYSQL_ROOT_USER | The database root user (used to create the site database). Defaults to DB_ROOT_USER |
|
MYSQL_MAX_ALLOWED_PACKET | 16777216 | Max packet length to send to or receive from the server, documentation |
@@ -0,0 +1,7 @@ | |||
[template] | |||
src = "mariadb-server.tmpl" |
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.
Following convention, elsewhere in the repository the src
should be named mariadb-server.cnf.tmpl
.
Also rename the corresponding file in the template folder.
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.
Oh, same goes for this .toml
file, it should become mariadb-server.cnf.toml
[template] | ||
src = "mariadb-server.tmpl" | ||
dest = "/etc/my.cnf.d/mariadb-server.cnf" | ||
uid = 100 |
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.
uid
should be 0
src = "mariadb-server.tmpl" | ||
dest = "/etc/my.cnf.d/mariadb-server.cnf" | ||
uid = 100 | ||
gid = 1000 |
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.
gid
should also be 0
.
dest = "/etc/my.cnf.d/mariadb-server.cnf" | ||
uid = 100 | ||
gid = 1000 | ||
mode = "0664" |
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.
mode
should be 0644
.
@@ -8,7 +8,8 @@ | |||
# this is only for the mysqld standalone daemon | |||
[mysqld] | |||
# skip-networking | |||
|
|||
# Allow for max_allowed_packet to be set in the project .env | |||
max_allowed_packet ={{ getenv "DB_MAX_ALLOWED_PACKET" }} |
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.
Remove the comment as it is not accurate.
Change the line to:
max_allowed_packet={{ getenv "MYSQL_MAX_ALLOWED_PACKET" }}
Also don't forget to rename the file as is stated in an earlier comment.
@@ -0,0 +1 @@ | |||
oneshot |
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.
Delete this file.
@@ -0,0 +1 @@ | |||
/etc/s6-overlay/scripts/set-max-packet.sh |
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.
Delete this file.
@@ -0,0 +1,8 @@ | |||
#!/command/with-contenv bash |
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.
Delete this file.
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.
Delete this file.
Yup, that was what I was saying. 👍 |
c92f255
to
672b564
Compare
Thanks for all the responses, comments and great insights here @nigelgbanks I'll get to work today with atomic fixes and commits, along with the appropriate resolutions. I appreciate you taking the time to help me learn and get the right changes in. |
Hi @nigelgbanks Think we're ready for another review! I've pushed all of the changes and here is a list below of what I did in order. Thanks again!
Enumerating objects: 35, done.
Counting objects: 100% (35/35), done.
Delta compression using up to 8 threads
Compressing objects: 100% (20/20), done.
Writing objects: 100% (27/27), 2.93 KiB | 2.93 MiB/s, done.
Total 27 (delta 6), reused 2 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (6/6), completed with 4 local objects.
remote:
remote: GitHub found 6 vulnerabilities on Islandora-Devops/isle-buildkit's default branch (6 moderate). To find out more, visit:
remote: https://github.com/Islandora-Devops/isle-buildkit/security/dependabot
remote:
To https://github.com/Islandora-Devops/isle-buildkit.git
+ c92f255...672b564 HEAD -> mariadb_max_packet (forced update)
|
Hi @nigelgbanks I think we're in business. Just tested all of this on a local isle-dc project and it seems to work nicely. FYI, one just can't exceed 1 GB as a max value per docs https://mariadb.com/docs/server/ref/mdb/system-variables/max_allowed_packet/. Test 1 - Use default settings of 16 MB
mariadb:
image: islandora/mariadb:local
MariaDB [(none)]> SHOW VARIABLES WHERE variable_name = 'max_allowed_packet';
+--------------------+----------+
| Variable_name | Value |
+--------------------+----------+
| max_allowed_packet | 16777216 |
+--------------------+----------+
1 row in set (0.004 sec) Test 2 - Change settings in docker-compose.yml to 900 MB
mariadb:
image: islandora/mariadb:local
environment:
MYSQL_MAX_ALLOWED_PACKET: 943718400
MariaDB [(none)]> SHOW VARIABLES WHERE variable_name = 'max_allowed_packet';
+--------------------+----------+
| Variable_name | Value |
+--------------------+----------+
| max_allowed_packet | 943718400 |
+--------------------+----------+
1 row in set (0.004 sec) Please note: That one can also use the following: mariadb:
#image: ${REPOSITORY}/mariadb:${TAG}
image: islandora/mariadb:local
environment:
MYSQL_MAX_ALLOWED_PACKET: ${MYSQL_MAX_ALLOWED_PACKET:-268435456} with a line in the |
Hi @nigelgbanks
Trying to make max_packet_allowed a variable instead of a value not exposed to the user.
Despite the
mariadb
container using thebase
image which I assumed uses confd, the environmental variable which I set in the isle-dc project.env
file ofDB_MAX_ALLOWED_PACKET=268435456
doesn't seem to work.Am I missing something?
Just curious, I'll keep plugging along.
Hope you are well.
Cheers ,
Gavin