Fix node labeling. Issue #305

[spinolacastro]

No description provided.

[openshift-bot]

Can one of the admins verify this patch?

[brenton]

Thanks Diego. @sdodson or I will try this out and work with you to get it merged!

[sdodson]

Perhaps 'Label nodes' instead, also why are we only labeling schedulable nodes?

[spinolacastro]

Ok, I'll change the title.
I haven't thought about that, i'll tag all nodes.

[sdodson]

If we remove the labels it fails to unset them, this is probably not a very common pattern but I'm sure someone will do it and expect it to work. We may need to iterate over the current labels removing them.

from oc label --help I guess we need to add - to the existing labels to delete them.

  // Update pod 'foo' by removing a label named 'bar' if it exists.
  // Does not require the --overwrite flag.
  $ oc label pods foo bar-

Current error

changed: [ose3-master.example.com] => (item=['ose3-node1.example.com', {'region': 'west', 'zone': 'infra'}])
failed: [ose3-master.example.com] => (item=['ose3-node1.example.com', {}]) => {"changed": true, "cmd": ["oc", "label", "--overwrite", "node", "ose3-node1.example.com"], "delta": "0:00:00.037087", "end": "2015-08-04 16:32:02.213093", "item": ["ose3-node1.example.com", {}], "rc": 1, "start": "2015-08-04 16:32:02.176006", "warnings": []}

[spinolacastro]

Agreed!
Perhaps we can use some kind of template against the object, oc update/delete/create -f labels.yml and save the previous/new labels as a fact?

[spinolacastro]

@sdodson would you mind taking a look? it's supposed to work even the labels are empty.

[detiber]

@sdodson @spinolacastro overall the change looks good.

The big question I have is on the behavior that we expect... Do we really want openshift-ansible updating node labels after initial installation? We need to keep in mind that changing node labels will not re-deploy applications that are already deployed based on the previous labels.

We should probably either document this edge case (including how to evacuate and re-schedule pods that are deployed on the nodes that have had their labels changed), or we should lock down node labeling to only handle the case of initial node installation/configuration.

I'm not overly tied to either outcome, we just need to make sure that user expectations are properly set for the chosen method.

[sdodson]

@detiber I wouldn't expect ansible to be responsible for triggering items to be rescheduled on labeling changes. I'd say we document that you'll need to handle rescheduling on your own.

Unrelated I could see the need for an adhoc playbook that re-balances clusters.

[detiber]

@sdodson 👍 on both accounts. I mainly wanted to highlight that users could expect that relabeling the nodes would lead to workloads being rescheduled based on node labels, which would not happen.

[sdodson]

@spinolacastro Can you add a note to roles/openshift_node/README.md mentioning that re-labeling nodes doesn't trigger re-scheduling? I guess evacuating the node and letting the scheduler sort it out.

Also mention that removing a label from a node isn't currently implemented.

oadm manage-node --schedulable=false ${NODE}
oadm manage-node --evacuate ${NODE}
oadm manage-node --schedulable=true ${NODE}

Then squash things and I think this is good enough to merge with the documented caveats.

[spinolacastro]

Ok, i'll write some notes.

[sdodson]

@twiest @wshearn This should be ready to merge.

[wshearn]

Yep, looks good to me, merging

[detiber]

@sdodson @spinolacastro running into an issue since this was merged...

TASK: [openshift_manage_node | Label nodes] *********************************** 
fatal: [ec2-52-20-104-133.compute-1.amazonaws.com] => Failed to template {% if 'openshift_node_labels' in hostvars[item] %} True {% else %} False {% endif %}: host not found: ip-172-18-1-110.ec2.internal

the internal node name does not necessarily match up with the inventory_hostname... working on a fix currently.

[detiber]

@sdodson @spinolacastro @wshearn currently testing this fix: #467