Conversation
adwk67
changed the title
adwk67
changed the title
siegfriedweber
left a comment
siegfriedweber
left a comment
There was a problem hiding this comment.
Before going into the details of this PR, I want to discuss the general approach.
There was a problem hiding this comment.
My observations: ValidatedCluster is used in the build step. It is passed to the builder functions. These functions build the resources for a specific role group. Instead of doing a look up in the passed ValidatedCluster, they expect the role group config as separate (redundant) parameter. The builder functions should be generic over the built role. The decisions, what to build for a specific role, is moved to the RoleGroupResolver, a trait which is implemented for the role specific role group configs. This resolver returns a ResolvedRoleGroup which contains calculated resources, e.g. the PVCs, as well as parts copied from the role group config, e.g. the resource requirements. It is partly redundant to the role group config but does not cover it. Every builder function therefore requires the ValidatedCluster, the role group config and the ResolvedRoleGroup. The ResolvedRoleGroup contains the fields common to all role groups but also RoleSpecificValues. The builder functions are therefore not completely agnostic to the role, but have to perform matching. These observations were not obvious to me.
The goal (of this and the next PR) is to create a RoleGroupBuilder struct which contains the complete context which is required for the builder functions. There is a trade-off between a context, that provides everything (and often too much) for the functions, and multiple small structs which provide not more than needed. The first is more convenient, the latter can avoid wrong usage.
The ResolvedRoleGroup could become the RoleGroupBuilder context, but I would fix the following points:
- Assuming that the builder functions should stay role agnostic: All decisions should be centralized in the
RoleGroupResolver. That is, that no matching outside the resolver should be necessary. This will be tackled in the following points. - The log configuration is spread over
RoleGroupLoggingandRoleSpecificValues. Whether or not the log configuration for a container should be built, should be decided inresolve()and therefore the logging fields should be moved toRoleGroupLoggingas optional. The consumers just have to create the resources if the configuration is set, and not care about the role. - The remaining fields in
RoleSpecificValues(listenerVolumeandstorage) should be moved toResolvedRoleGroupas optional. Again, the decision whether or not the according resources should be built, is already taken inresolve().
The next steps probably belong to the next PR, but I want to mention them here for the big picture:
4. Duplicate all fields from the role group configuration in ResolvedRoleGroup. The config is already here, but the overrides and replicas are missing. With this step, it is not necessary anymore to additionally provide the role group configurations to the builder functions. The C parameter can then also be dropped from RoleGroupBuilder.
5. Add references to the remaining context fields like ValidatedCluster, KubernetesClusterInfo and RoleGroupName to ResolvedRoleGroup.
6. Rename ResolvedRoleGroup to RoleGroupBuilder.
After these steps, the structures could look as follows:
pub struct RoleGroupLogging {
pub hdfs: ContainerLogConfig,
pub vector: Option<ContainerLogConfig>,
pub zkfc: Option<ContainerLogConfig>,
pub format_namenodes: Option<ContainerLogConfig>,
pub format_zookeeper: Option<ContainerLogConfig>,
pub wait_for_namenodes: Option<ContainerLogConfig>,
}
// former `ResolvedRoleGroup`
pub struct RoleGroupBuilder<'a> {
pub cluster: &'a ValidatedCluster,
pub cluster_info: &'a KubernetesClusterInfo,
pub role: HdfsNodeRole,
pub role_group_name: RoleGroupName,
pub selector_labels: Labels,
pub common: CommonNodeConfig,
pub resources: ResourceRequirements,
pub volume_claim_templates: Vec<PersistentVolumeClaim>,
pub listener_volume: Option<Volume>,
pub datanode_storage: Option<DataNodeStorageConfig>,
pub logging: RoleGroupLogging,
pub replicas: Option<u16>,
pub config_overrides: v1alpha1::HdfsConfigOverrides,
pub env_overrides: EnvVarSet,
pub pod_overrides: PodTemplateSpec,
pub product_specific_common_config: JavaCommonConfig,
}The builder functions will then be methods of this RoleGroupBuilder:
impl<'a> RoleGroupBuilder<'a> {
pub(crate) fn build_statefulset(&self) -> Result<StatefulSet, Error> {
...
}
}instead of
pub(crate) fn build_rolegroup_statefulset(
validated: &ValidatedCluster,
cluster_info: &KubernetesClusterInfo,
role: &HdfsNodeRole,
role_group_name: &RoleGroupName,
rolegroup_config: &ValidatedRoleGroupConfig,
) -> Result<StatefulSet, Error> {
...
}(This was deliberately written by hand and not by AI.)
There was a problem hiding this comment.
When you say "the next PR" are you referring to #833? (as I think at least points 4 and 5 are addressed there)
There was a problem hiding this comment.
Thanks for your feedback: in reading through it several times it's clear that the flow is still non-obvious, even after we have made changes to try and make things more readable. So we agree on that. But I think that centralising things too consistently will come back to bite us. I would prefer to have a builder per role: this stops us from using Option for "non-optional for role X but not relevant to role Y" alongside "truly optional and the user didn't set it", and it prevents us from erasing the role we are building only to have to rediscover it later on. If we went that route, then points 1-3 are largely redundant. Points 4-6 are largely already in #833 - if you want I can push chnages to #833 that reflect the above, so we can compare. What do you think?
There was a problem hiding this comment.
Yes, I am referring to #833 and points 4 and 5 are somehow addressed there.
But it's mostly the fragmentation that bothers me.
For example, before your changes, the build_log4j_configs required a AnyNodeConfig that decided which configs to build:
pub fn build_log4j_configs(merged_config: &AnyNodeConfig) -> Vec<(&'static str, String)> {
add_log4j_config_if_automatic(
Some(merged_config.hdfs_logging()),
...
);
add_log4j_config_if_automatic(
merged_config
.as_namenode()
.map(|nn| nn.logging.for_container(&NameNodeContainer::Zkfc)),
...
);
}
impl AnyNodeConfig {
pub fn as_namenode(&self) -> Option<&NameNodeConfig> {
if let Self::Name(node) = self {
Some(node)
} else {
None
}
}
pub fn hdfs_logging(&'_ self) -> Cow<'_, ContainerLogConfig> {
match self {
AnyNodeConfig::Name(node) => node.logging.for_container(&NameNodeContainer::Hdfs),
AnyNodeConfig::Data(node) => node.logging.for_container(&DataNodeContainer::Hdfs),
AnyNodeConfig::Journal(node) => node.logging.for_container(&JournalNodeContainer::Hdfs),
}
}
}Not perfect, but quite easy to understand.
With your changes, build_log4j_configs now requires two parameters. Both, RoleGroupLogging and RoleSpecificValues contain logging configuration specific to the role. The contents of RoleGroupLogging are decided in RoleGroupResolver::resolve() depending on the role and for the RoleSpecificValues the caller has to match themselves. Interestingly, both parameters come from the same ResolvedRoleGroup. It is weird that RoleSpecificValues are not already resolved there.
pub fn build_log4j_configs(
logging: &RoleGroupLogging,
role: &RoleSpecificValues,
) -> Vec<(&'static str, String)> {
add_log4j_config_if_automatic(
&logging.hdfs,
...
);
match role {
RoleSpecificValues::Name {
zkfc,
...
} => {
add_log4j_config_if_automatic(
zkfc,
...
);
}
}
}Then there are the MergedRoleGroupConfigs, but the logging configs are merged as well and stored in different places.
Additionally, the RoleSpecificValues contain downcast methods (listener_volume and datanode_storage) like the old AnyNodeConfig, but less generic.
The C from the ResolvedRoleGroup was somewhat moved to the RoleGroupBuilder which in turn resolves the role specific config to ResolvedRoleGroup.
In summary, it was much clearer before what was stored where and where the decision for the role was taken.
2 participants
Description
Part of https://github.com/stackabletech/internal-issues/issues/167.
Overview
controller/mods.rs, where the two role-keyed maps are replaced with 3 pairs of<role>_configand<role>_role_group_configs, e.g.journalnode_configandjournalnode_role_group_configsAnyNodeConfigand the associated downcastsRoleGroupResources::stateful_setsneeds to be ordered such that STSs are rolled out in the correct/expected orderDefinition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker