refactor: Extract apply and update status steps - #856
Conversation
|
…an be inspected before building discovery config map
| && router_listener | ||
| .status | ||
| .as_ref() | ||
| .and_then(|status| status.ingress_addresses.as_ref()?.first()) | ||
| .is_some() |
There was a problem hiding this comment.
nit: This is an example of boolean blindness. The address is fetched here, but since it never reaches the called function, that function has to fetch it a second time and return an almost meaningless Result. Doing the check directly in (maybe_)build_discovery_configmaps and returning an Option<ConfigMap> would avoid both.
| .context(AuthenticationClassRetrievalSnafu)?; | ||
|
|
||
| let cluster_name = get_cluster_name(druid).context(ClusterIdentitySnafu)?; | ||
| let router_listener = match group_listener_name(&cluster_name, &DruidRole::Router) { |
There was a problem hiding this comment.
nit: Here you have to guess a role that provides the group listener. You could avoid that by pulling the shared logic into a "general_group_listener_name" function and renaming group_listener_name to "maybe_group_listener_for_role" (which calls general_group_listener_name when the role has one). This call site could then just use general_group_listener_name.
| // The internal Secret is deliberately not tracked in [`ClusterResources`] (applied | ||
| // directly instead of via `add_resources`), so it survives the orphan deletion below: | ||
| // the build step only produces it when it is absent or incomplete, so on most runs no | ||
| // Secret is applied and a tracked one would be deleted as an orphan. |
There was a problem hiding this comment.
Why does the internal Secret need special handling here instead of being applied like every other resource? Re-applying an unchanged Secret (in the case of the existing Secret) should be a no-op, so what breaks if we just route it through add_resources with the rest?
There was a problem hiding this comment.
It can't go through No, you're right, in steady state this is a no-op.add_resources because on most runs we emit None, in which case the tracked secret won't be in the resources added in that run and delete_orphaned_resources would delete it. We could always emit a Secret by copying the existing values through the build step, which would mean handling secret values and not just their keys.
There was a problem hiding this comment.
I mean, we could do that by getting and holding an existing secret, but the operator would still be writing secret values (other than ones it has itself randomly generated), which is what we try to avoid.
| // Apply order is: StatefulSets last (a changed mounted ConfigMap/Secret | ||
| // must exist first, else Pods restart -- commons-operator#111). The ServiceAccount comes | ||
| // first because the Pods reference it at creation time. |
There was a problem hiding this comment.
The internal Secret is now applied after the StatefulSet.
| // but we strive that our operators don't handle Secret contents and it's a one time migration. | ||
|
|
||
| tracing::warn!( | ||
| secret_name, |
There was a problem hiding this comment.
secret_name holds the name of the new Secret, but this log message is about the old immutable Secret.
2 participants
Description
This PR covers the following:
Note
the Applier here deliberately diverges from the airflow shape (apply → apply_config_maps → finish, with orphan deletion only at the end) because the discovery ConfigMaps can only be built from the applied router Listener -this will apply to zookeeper as well..
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker