-
Notifications
You must be signed in to change notification settings - Fork 57
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
Enhancements to the Strimzi Kafka Operator for managing shared volume mounts, #112
Conversation
Signed-off-by: rao2100 <rao2100@gmail.com>
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.
Thanks for the proposal. As I already mentioned in the PR, we have to make sure it matches all the various usecases mentioned in the existing Strimzi issues such as strimzi/strimzi-kafka-operator#3693. And not just your single use-case. So you should cover all of them in this proposal and explain how they will be fulfilled.
My (personal) view is that we do not want to extend the existing .storage
APi which is designed in sync withj Kafka's requirement. Instead, I think the right way might be to extend the Pod and Container template to allow adding volumes and mount points?
|
||
**Sample Test** | ||
|
||
- This test case is to test the strimzi operator customization that allows mounting of shared volume across kafka-connect pods |
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.
You should also explain how will this be cover in the various test environments -> how will you provide the shared storage for example?
|
||
While formulating this proposal, several alternatives were considered but ultimately rejected for various reasons: | ||
|
||
- **Using Local Storage**: Initially, the idea of using local storage for each pod was considered. However, this was rejected because it would not allow data sharing across pods, which is a crucial requirement for ensuring high availability and fault tolerance in distributed systems. |
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.
I think it is fine to reject local storage. But I do not think shared persistent volumes are a good basis for distributed systems as you suggest as they cause many limitations as to how distributed they can be.
While formulating this proposal, several alternatives were considered but ultimately rejected for various reasons: | ||
|
||
- **Using Local Storage**: Initially, the idea of using local storage for each pod was considered. However, this was rejected because it would not allow data sharing across pods, which is a crucial requirement for ensuring high availability and fault tolerance in distributed systems. | ||
- **Static Volume Mounts**: Another alternative was to use static volume mounts with predefined paths. This was rejected due to its lack of flexibility. It would not allow custom specification of the container directory for storage volume mounting, which is necessary for accommodating different system configurations and use cases. |
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.
I don't think it is really clear what Static Volume Mounts
are.
@@ -0,0 +1,108 @@ | |||
# Shared Volume Mount Changes | |||
|
|||
The proposed update to the Strimzi Kafka Operator introduces management of shared volume mounts for kafka connect cluster, |
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.
We need to have a reusable solution that covers all the different use cases for mounting some volumes. So you should not focus just on your immediate need. We have to make sure this change can survive the next proposal from someone else with different requirement.
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.
I am with Jakub on this.
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.
I left a few comments but one thing you should do is splitting all the sentences, one per line. It's really hard to make comments on a specific sentence when there are more and how GitHub works.
|
||
This change introduces enhancements to the Strimzi Kafka Operator for managing shared volume mounts, improving task resiliency across nodes. Key changes include: | ||
|
||
1) **mountPath:** A string property to specify the container directory for mounting the storage volume. While the default is /var/lib/kafka/data, this allows for custom path specification. By default, it will try to mount read write many storage if accessMode is not specified. |
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.
It's not just about where the volume is mounted but this path is also related to the Kafka broker configuration where putting the logs via log.dirs
param.
Also how it's going to work in terms of backward compatibility if for an already running cluster you change this path. What happens to the log segments?
|
||
1) **mountPath:** A string property to specify the container directory for mounting the storage volume. While the default is /var/lib/kafka/data, this allows for custom path specification. By default, it will try to mount read write many storage if accessMode is not specified. | ||
|
||
2) **accessMode:** An optional string property defining the storage access mode, determining permissions like read-only or read-write. |
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.
can you elaborate what's the rationale behind this need?
@@ -0,0 +1,108 @@ | |||
# Shared Volume Mount Changes | |||
|
|||
The proposed update to the Strimzi Kafka Operator introduces management of shared volume mounts for kafka connect cluster, |
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.
I am with Jakub on this.
@rao2100 Do you plan to incorporate the comments into the proposal? Or should this be closed? |
Discussed on the community call on 2.5.2024: Should be closed as there does not seem to be any progress. Feel free to reopen it / open new PR with the comments incorporated. |
Proposal to introduce enhancements to the Strimzi Kafka Operator for managing shared volume mounts, improving task resiliency across nodes.
The changes for the proposal in the PR (strimzi/strimzi-kafka-operator#9761)