#54767 Remove extraneous volumes in Keeper image #61683
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Funnily enough, when I build & start the container locally: So it seems to me that there's probably a mismatch in the Docker image already (which prepares /var/lib/clickhouse) and the default keeper config (which tries to use /var/lib/clickhouse-keeper) And this issue is also present in current public Which really makes me think that my point in the issue about no one actually using the bare image as-is without explicit volumes is probably true. |
|
It's a pretty good catch, actually! Would you like to fix it as well? We definitely should use The last one should be replaced by |
|
Sure, I’ll do that shortly. I refrained as I expected you’d prefer it fixed in a separate PR |
|
No, let's fix every image's flaw at once and backport it |
|
Ok so the situation is a little worse than it sounds. The commit above at least makes the Docker image more readable (imo), but alas I can't entirely change the entrypoint... The problem is that keeper, with the default setup, will expect both /var/lib/clickhouse and /var/lib/clickhouse-keeper to exist and be writable... If I put /var/lib/clickhouse on both sides, ie editing the Dockerfile with /var/lib/clickhouse (instead of -keeper): slow crashMeanwhile if I put /var/lib/clickhouse-keeper on both sides, ie editing entrypoint.sh:41 to use -keeper: fast crashThe problem is that there are references to /var/lib/clickhouse inside the default keeper config, for coordination logs/snapshots, which I think is what prevents us from only using /var/lib/clickhouse-keeper here. And if we change that, we are getting into serious backwards-incompatibility territory, so I'd do that change separately, personally (and not backport it). |
Felixoid
reviewed
Mar 23, 2024
Tristan971
force-pushed
the
54767-remove-extraneous-keeper-docker-volumes
branch
from
66975d3
to
788fe1a
Compare
Felixoid
added
the
can be tested
robot-ch-test-poll1
added
the
pr-build
|
This is an automated comment for commit 38fb8b3 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
Specifically the 2 non-data-related volumes Fixes issue ClickHouse#54767
Since it appears mandatory at the moment with default Keeper config
Tristan971
force-pushed
the
54767-remove-extraneous-keeper-docker-volumes
branch
from
6a5a97d
to
38fb8b3
Compare
|
(rebased, since I presume that's what failed these 2 tests) |
|
It's good from my PoV! Let's wait for another opinion from the team. |
|
Thanks for raising it up and fixing it! |
robot-clickhouse
added a commit
that referenced
this pull request
Mar 27, 2024
robot-clickhouse
added a commit
that referenced
this pull request
Mar 27, 2024
This was referenced Mar 27, 2024
robot-clickhouse
added a commit
that referenced
this pull request
Mar 27, 2024
robot-clickhouse
added a commit
that referenced
this pull request
Mar 27, 2024
robot-ch-test-poll3
added
the
pr-backports-created
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Remove from the Keeper Docker image the volumes at /etc/clickhouse-keeper and /var/log/clickhouse-keeper
Documentation entry for user-facing changes
Motivation: Fixes #54767.
In summary (more details in the issue), VOLUME directives in Docker images cannot be overriden by users (neither by extending the image, nor at runtime). Therefore, volumes that are not strictly necessary are: