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?
| // 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