You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bigtop.apache.org by cmars <gi...@git.apache.org> on 2018/10/02 19:08:41 UTC
[GitHub] bigtop pull request #400: WIP: Add Juju storage support to Kafka charm.
GitHub user cmars opened a pull request:
https://github.com/apache/bigtop/pull/400
WIP: Add Juju storage support to Kafka charm.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cmars/bigtop kafka-storage
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/bigtop/pull/400.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #400
----
commit a20af4e1885d96bd2b656b787ded2fdcdc277100
Author: Casey Marshall <gi...@...>
Date: 2018-09-22T11:30:37Z
Add Juju storage support to Kafka charm.
----
---
[GitHub] bigtop pull request #400: Add Juju storage support to Kafka charm.
Posted by cmars <gi...@git.apache.org>.
Github user cmars commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/400#discussion_r222448500
--- Diff: bigtop-packages/src/charm/kafka/layer-kafka/lib/charms/layer/bigtop_kafka.py ---
@@ -58,10 +60,14 @@ def configure_kafka(self, zk_units, network_interface=None):
if network_interface:
ip = Bigtop().get_ip_for_interface(network_interface)
override['kafka::server::bind_addr'] = ip
+ if log_dir:
+ override['kafka::server::log_dirs'] = log_dir
bigtop = Bigtop()
bigtop.render_site_yaml(roles=roles, overrides=override)
bigtop.trigger_puppet()
+ os.makedirs(log_dir, mode=0o700, exist_ok=True)
+ shutil.chown(log_dir, user='kafka')
--- End diff --
Fixed.
---
[GitHub] bigtop pull request #400: Add Juju storage support to Kafka charm.
Posted by kwmonroe <gi...@git.apache.org>.
Github user kwmonroe commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/400#discussion_r222415197
--- Diff: bigtop-packages/src/charm/kafka/layer-kafka/reactive/kafka.py ---
@@ -90,3 +98,39 @@ def serve_client(client, zookeeper):
client.send_port(kafka_port)
client.send_zookeepers(zookeeper.zookeepers())
hookenv.log('Sent Kafka configuration to client')
+
+
+@hook('logs-storage-attached')
+def storage_attach():
+ storageids = hookenv.storage_list('logs')
+ if not storageids:
+ hookenv.status_set('blocked', 'cannot locate attached storage')
+ return
+ storageid = storageids[0]
--- End diff --
I'm embarrassingly unfamiliar with `storage: multiple: range:` like you have defined in `metadata.yaml`. Is `storageids[0]` safe here because the range maxes out at 1 and therefore we'll only have 1 element in this list?
---
[GitHub] bigtop pull request #400: Add Juju storage support to Kafka charm.
Posted by kwmonroe <gi...@git.apache.org>.
Github user kwmonroe commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/400#discussion_r222415839
--- Diff: bigtop-packages/src/charm/kafka/layer-kafka/reactive/kafka.py ---
@@ -38,9 +40,10 @@ def configure_kafka(zk):
hookenv.status_set('maintenance', 'setting up kafka')
data_changed( # Prime data changed for network interface
'kafka.network_interface', hookenv.config().get('network_interface'))
+ log_dir = unitdata.kv().get('kafka.storage.log_dir')
--- End diff --
Let's add another `data_changed('kafka.storage.log_dir', log_dir)` here. This `configure_kafka` method is the primary way that kafka gets installed -- after that, we constantly check data_changed in `configure_kafka_zookeepers` to trigger a reinstall.
If we *don't* do a `data_changed` with `log_dir` here, we'll get installed and started, and then immediately reinstall on the first execution of `configure_kafka_zookeepers`, because the check for `data_changed('kafka.storage.log_dir', log_dir)` in that method will be new (and hence, True).
---
[GitHub] bigtop issue #400: BIGTOP-3092: kafka charm: support juju storage
Posted by cmars <gi...@git.apache.org>.
Github user cmars commented on the issue:
https://github.com/apache/bigtop/pull/400
@kwmonroe Updated
---
[GitHub] bigtop pull request #400: Add Juju storage support to Kafka charm.
Posted by cmars <gi...@git.apache.org>.
Github user cmars commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/400#discussion_r222448452
--- Diff: bigtop-packages/src/charm/kafka/layer-kafka/reactive/kafka.py ---
@@ -38,9 +40,10 @@ def configure_kafka(zk):
hookenv.status_set('maintenance', 'setting up kafka')
data_changed( # Prime data changed for network interface
'kafka.network_interface', hookenv.config().get('network_interface'))
+ log_dir = unitdata.kv().get('kafka.storage.log_dir')
--- End diff --
Done.
---
[GitHub] bigtop pull request #400: Add Juju storage support to Kafka charm.
Posted by cmars <gi...@git.apache.org>.
Github user cmars commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/400#discussion_r222424656
--- Diff: bigtop-packages/src/charm/kafka/layer-kafka/reactive/kafka.py ---
@@ -90,3 +98,39 @@ def serve_client(client, zookeeper):
client.send_port(kafka_port)
client.send_zookeepers(zookeeper.zookeepers())
hookenv.log('Sent Kafka configuration to client')
+
+
+@hook('logs-storage-attached')
+def storage_attach():
+ storageids = hookenv.storage_list('logs')
+ if not storageids:
+ hookenv.status_set('blocked', 'cannot locate attached storage')
+ return
+ storageid = storageids[0]
--- End diff --
It's explained near the bottom of [this section](https://docs.jujucharms.com/2.4/en/developer-storage#adding-storage) in the juju docs. Basically, we allow 0 or 1 stores to be attached. 0 allows us to deploy with no storage attached; /tmp would be used in that case.
The check above (`if not storageids`) ensures that if we've attached storage, we have at least one store, or the list would be empty and we'd be blocked. Not sure if that can actually happen (can you attach 0 stores?) but this case is covered in any case.
I borrowed this logic & metadata config from the postgresql charm, which has a similar storage usage pattern (single mounted volume for data).
---
[GitHub] bigtop pull request #400: Add Juju storage support to Kafka charm.
Posted by cmars <gi...@git.apache.org>.
Github user cmars commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/400#discussion_r222424942
--- Diff: bigtop-packages/src/charm/kafka/layer-kafka/lib/charms/layer/bigtop_kafka.py ---
@@ -58,10 +60,14 @@ def configure_kafka(self, zk_units, network_interface=None):
if network_interface:
ip = Bigtop().get_ip_for_interface(network_interface)
override['kafka::server::bind_addr'] = ip
+ if log_dir:
+ override['kafka::server::log_dirs'] = log_dir
bigtop = Bigtop()
bigtop.render_site_yaml(roles=roles, overrides=override)
bigtop.trigger_puppet()
+ os.makedirs(log_dir, mode=0o700, exist_ok=True)
+ shutil.chown(log_dir, user='kafka')
--- End diff --
Needs to happen after the `trigger_puppet`, otherwise the `kafka` user may not yet exist. However, it should be in an `if log_dir` clause.
---
[GitHub] bigtop pull request #400: Add Juju storage support to Kafka charm.
Posted by kwmonroe <gi...@git.apache.org>.
Github user kwmonroe commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/400#discussion_r222411244
--- Diff: bigtop-packages/src/charm/kafka/layer-kafka/lib/charms/layer/bigtop_kafka.py ---
@@ -58,10 +60,14 @@ def configure_kafka(self, zk_units, network_interface=None):
if network_interface:
ip = Bigtop().get_ip_for_interface(network_interface)
override['kafka::server::bind_addr'] = ip
+ if log_dir:
+ override['kafka::server::log_dirs'] = log_dir
bigtop = Bigtop()
bigtop.render_site_yaml(roles=roles, overrides=override)
bigtop.trigger_puppet()
+ os.makedirs(log_dir, mode=0o700, exist_ok=True)
+ shutil.chown(log_dir, user='kafka')
--- End diff --
These bits should get tucked under the `if log_dir` above, right?
---