[pgbench] pin_node is NOT optional in ripsaw_v1alpha1_pgbench_cr.yaml about benchmark-operator

[jtaleric]

Did you try with pin_node removed from the CR?

          # pin_node is an optional kubernetes hostname to which the pgbench pod will be pinned
          pin_node: 

When looking at the code.

{% if item.1.pin_node is defined and item.1.pin_node|length and item.1.pin_node is not sameas "" %}
      nodeSelector:
        kubernetes.io/hostname: '{{ item.1.pin_node }}'
{% endif %}

We might want to check the length of pin_node against a value, for example :

{% if item.1.pin_node is defined and item.1.pin_node|length > 1 and item.1.pin_node is not sameas "" %}
      nodeSelector:
        kubernetes.io/hostname: '{{ item.1.pin_node }}'
{% endif %}

[dustinblack]

Tested these conditions:

1. pin_node not defined: passed
2. pin_node defined with a valid node name (from oc get nodes): passed
3. pin_node defined but empty: failed
4. pin_node defined as '': passed
5. pin_node defined as false: failed

5 above was just an extra test, so I'm sure 3 is what got you.

In the case of 3, we get the error:

object of type 'NoneType' has no len()

So I don't think there will be a difference in checking item.1.pin_node|length vs item.1.pin_node|length > 0 since it's simply angry that you can't check length on a NoneType variable (defined but without a value).

I'll look at this again tomorrow after some sleep and coffee. I'm sure there is a simple solution in the if statement that I'm not seeing right now.

[jtaleric]

Instead of us having to duplicate logic (like we currently are), how about we create role/pretasks that runs before all the other roles.

The responsibility of this role is to check for things like this in the CR, reducing the duplicate checks we currently have to do.

In this case we would need

item.1.ping_node is not none

I don't think this will eliminate all checks in other roles, but it will reduce things, additionally catch the error early... @dustinblack @aakarshg @dry923 thoughts?

[dustinblack]

@jtaleric So this might for instance convert any defined but empty variables from the CR into blank values ('') instead, so that the validation logic within the main role is simplified?

[dustinblack]

Tested with simplified if logic:

{% if item.1.pin_node is defined and item.1.pin_node %}

And the results look good:

1. pin_node not defined: passed
2. pin_node defined with a valid node name (from oc get nodes): passed
3. pin_node defined but empty: passed << winning
4. pin_node defined as '': passed
5. pin_node defined as false: passed << winning

[dustinblack]

Fixed with PR #145