search

canonical / bundle-kubeflow

Charmed Kubeflow
Apache License 2.0
104 stars 50 forks source link

dex_auth non leader units should set active as well #249

Closed ycliuhw closed 4 years ago

ycliuhw commented 4 years ago

While doing some juju testing, found non-leader units are never set to active. https://github.com/juju-solutions/bundle-kubeflow/blob/aac5a218cf29f144aaa6c037bfc51b97d34ecbb9/charms/dex-auth/reactive/dex_auth.py#L47

# this should be set for non leader units to set `active` status.
set_flag("charm.started")
dex-auth/0*                     active       idle        10.224.1.35  5556/TCP
dex-auth/1                      maintenance  idle        10.224.2.45  5556/TCP           fetching resource: oci-image
dex-auth/2                      maintenance  idle        10.224.0.36  5556/TCP           fetching resource: oci-image
johnsca commented 4 years ago

The non-leader "units" can't meaningfully effect change, nor can they actually query the status of the workload pod that they're supposed to represent. I do think that the current maintenance status is not correct, but I think the best that the charms could accomplish as it stands today would be to exit earlier and never call status-set at all so that it stays at unknown. There's still an issue with the status set by the leader unit, though, because active in that case really just means "I've given Juju a pod spec". If the charm knows that its workload has a health / liveness endpoint, or an API that it could try to query, it might be worth improving the charm to do so. However, a lot of that is already handled by Kubernetes and ideally the charm would just be able to see and respond to that.

ycliuhw commented 4 years ago

Agreed, for now, we probably just never call status-set for minion units for less confusion.