FoundationDB / fdb-kubernetes-operator

A kubernetes operator for FoundationDB
Apache License 2.0
247 stars 82 forks source link

customParameters should be merged together #636

Open johscheuer opened 3 years ago

johscheuer commented 3 years ago

I tried to use this modified sample cluster spec (I removed the default fields):

processes:
    general:
      customParameters:
        - "locality_test=1"
        - "trace_format=json"
    storage:
      customParameters:
        - "knob_disable_posix_kernel_aio=1"

I would expect that the customParameters are merged and all storage processes will have all 3 custom parameters but only the most specific list of customParameters will be used e.g. in my case only knob_disable_posix_kernel_aio=1 will be set. This is the resulting fdbmonitor.conf:

root@sample-cluster-storage-4:/var/fdb# cat /var/dynamic-conf/fdbmonitor.conf
[general]
kill_on_configuration_change = false
restart_delay = 60
[fdbserver.1]
command = /usr/bin/fdbserver
cluster_file = /var/fdb/data/fdb.cluster
seed_cluster_file = /var/dynamic-conf/fdb.cluster
public_address = 10.1.8.225:4501
class = storage
logdir = /var/log/fdb-trace-logs
loggroup = sample-cluster
datadir = /var/fdb/data
locality_instance_id = storage-4
locality_machineid = sample-cluster-storage-4
locality_zoneid = sample-cluster-storage-4
knob_disable_posix_kernel_aio=1

I think it will be useful to merge the customParameters and actually that would be something that I expect.

brownleej commented 3 years ago

I think this is a good feature, but unless we de-dupe them it's a breaking change. We may also need to determine how to support having a specific process type whose custom parameters do not include all of the parameters from the general type.

johscheuer commented 3 years ago

I would say that the latter is not supported in the first version, one could also argue that such knobs shouldn't be defined in the general section.

gm42 commented 8 months ago

I think this is a good feature, but unless we de-dupe them it's a breaking change.

@brownleej duplicates are currently going to trigger an error, I just created this related issue