Bug in logging-deployer's handling of dynamic persistent volume claims?

[jtconnor]

Hi, I'm trying to use the logging-deployer with dynamic persistent claims via:

"""
es-pvc-dynamic
Set to true to have created persistent volume claims annotated so that their backing storage can be dynamically provisioned (if that is available for your cluster).
"""
per: https://docs.openshift.org/latest/install_config/aggregate_logging.html

The logging-deployer isn't creating the persistent volume claims :( and I think it may be due to an undefined variable in the install.sh logging-deployer script.

From line 392:

    if [ "${pvcs[$pvc]}" != 1 -a "${ES_PVC_SIZE}" != "" ]; then # doesn't exist, create it
      oc new-app logging-pvc-${es_pvc_dynamic:+"dynamic-"}template -p "NAME=$pvc,SIZE=${ES_PVC_SIZE}"
      pvcs["$pvc"]=1
    fi

ES_PVC_SIZE isn't defined anywhere. It looks like the variable should be es_pvc_size instead of ES_PVC_SIZE. I looked at the file history and it looks like this bug was introduced in:

0b455a7

Also, I noticed that the top of the install.sh script includes set -ex. Have you considered doing set -eux? The extra -u will cause an error on an undefined variable, which would make it easier to catch undefined variable bugs.

Is that correct or am I misunderstanding something? If it is a bug, would you like me to submit a pull request? I'm not sure how you manage contributors, pull requests, and code review, but if you point me in the right direction I'd be happy to give it a try.

cc: @sosiouxme

[sosiouxme]

You are right, that value is only defined if given in the deployer template parameter, and not if you put it in the configmap as intended. So this is a bug. It should be ${input_vars[es-pvc-size]}, and the same thing for ES_PVC_PREFIX and the _OPS_ versions below. Apparently I only ever tested it with template parameters. If you would be so kind as to submit a pull request, we would be happy to have it.

[sosiouxme]

This is the reason for what was reported in bug https://bugzilla.redhat.com/show_bug.cgi?id=1371852 too.

[sosiouxme]

Missed the second part. I wouldn't set -u without giving the code a good once-over to see if it's going to lead to any "surprises" (and a good regression test afterward), but it is a good goal to strive for.

[jtconnor]

Thanks @sosiouxme for the quick fix! (and sorry for being too slow to contribute) I also verified the fix in our own openshift cluster.