datastax / pulsar-ansible

Apache License 2.0
4 stars 9 forks source link

[DOC] Mark Host inventory auto generation as experimental feature #22

Closed MMirelli closed 2 years ago

MMirelli commented 2 years ago

Although auto generating ansible host inventory is a nice feature, I think it's out of the scope of pulsar ansible to generate host inventories. Users should do that independently. I find the auto-generation solution is convenient as long as it works. The main reasons why I wouldn't support it, at least officially, are:

  1. As long as pulsar-ansible is primarily a set of ansible playbooks I think this feature is a nice-to-have addition, whenever it works, but it shouldn’t be advertised as a stable / maintained feature. Playbooks usually assume that the underlying infrastructure is up and running and have the inventory as input.
  2. buildAnsiHostInvFile.sh is a bash script working only on bash v4.0+ - based on associative arrays. This script can be very fragile in different environments, hence difficult to deliver to customers and maintain in the future.

A possible solution would consist of:

  1. documenting the format of an ansible inventory
  2. labelling Host inventory generation as an experimental and additional feature (not supported / tested).
yabinmeng commented 2 years ago

This can't be treated as an experimental (and optional) feature because it determines the correctness of the Ansible host inventory file. Bash script or not, there needs to be something to do auto-gen the host inventory file.

I don't see why bash 4.0+ is a problem. Just like a server software like Pulsar requires minimum Java version, it is legit to require a minimum bash version.

MMirelli commented 2 years ago

Yes that's fair. Should we then replace the current format of clusterDefRaw with a json / yaml file whose fields could be accessed with jello / yq standard tools to access configuration files? As a result, we will have jello / yq as a requirement, but this would simplify the advanced scripting in buildAnsiHostInvFile.sh.

A yaml file solving the problem could be:

zookeeper: # server type 2) 
  - internal_ip: 'x.y.z.k' # 0)
    external_ip: 'a.b.c.d' # 1)
    region: 'region1' # 3)
    availability_zone: 'az1' # 4)
    contact_point: 'no' # 5)
    deployment_state: '' # 6)
  - internal_ip: 'x.y.z.h' # 0)
    external_ip: 'a.b.c.f' # 1)
    region: 'region1' # 3)
    availability_zone: 'az1' # 4)
    contact_point: 'no' # 5)
    deployment_state: '' # 6)
broker: # server type 2) 
  - internal_ip: 'x.y.z.m' # 0)
    external_ip: 'a.b.c.e' # 1)
    region: 'region1' # 3)
    availability_zone: 'az1' # 4)
    contact_point: 'yes' # 5)
    deployment_state: '' # 6)
bookkeeper: # server type 2) 
  - internal_ip: 'x.y.z.j' # 0)
    external_ip: 'a.b.c.g' # 1)
    region: 'region1' # 3)
    availability_zone: 'az1' # 4)
    contact_point: 'no' # 5)
    deployment_state: '' # 6)

This would also have the advantage that the file itself could easily be generated / modified by yq scripts.

MMirelli commented 2 years ago

The yq solution is not the best, as large enterprises might not allow non-standard software to be used. A new issue aimed at enhancing the portability of https://github.com/datastax/pulsar-ansible/blob/master/bash/buildAnsiHostInvFile.sh will be opened.