From b61c14d3c3768ee61caad013a0472cde823f0e7d Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 15 Sep 2023 16:17:28 +0200 Subject: [PATCH] refactor!: Remove legacy node selector on RoleGroup --- src/commons/affinity.rs | 285 +----------------------------------- src/product_config_utils.rs | 22 --- src/role_utils.rs | 8 +- 3 files changed, 3 insertions(+), 312 deletions(-) diff --git a/src/commons/affinity.rs b/src/commons/affinity.rs index f5ca97be3..001f1104d 100644 --- a/src/commons/affinity.rs +++ b/src/commons/affinity.rs @@ -2,10 +2,9 @@ use std::collections::BTreeMap; use k8s_openapi::{ api::core::v1::{ - NodeAffinity, NodeSelector, NodeSelectorRequirement, NodeSelectorTerm, PodAffinity, - PodAffinityTerm, PodAntiAffinity, WeightedPodAffinityTerm, + NodeAffinity, PodAffinity, PodAffinityTerm, PodAntiAffinity, WeightedPodAffinityTerm, }, - apimachinery::pkg::apis::meta::v1::{LabelSelector, LabelSelectorRequirement}, + apimachinery::pkg::apis::meta::v1::LabelSelector, }; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -41,79 +40,6 @@ pub struct StackableAffinity { pub node_selector: Option, } -impl StackableAffinityFragment { - #[deprecated( - since = "0.36.0", - note = "During https://github.com/stackabletech/issues/issues/323 we moved from the previous selector field on a rolegroup to a more generic affinity handling. \ -We still need to support the old selector field, which has some custom magic (see the code in this function). \ -So we need a way to transform the old into the mechanism which this function offers. \ -It will be removed once we stop supporting the old mechanism." - )] - pub fn add_legacy_selector(&mut self, label_selector: &LabelSelector) { - tracing::warn!("Deprecated field `selector` was specified. Please use the new `affinity` field instead, as support for `selector` will be removed in the future. See https://docs.stackable.tech/home/stable/contributor/adr/ADR026-affinities.html for details"); - - let node_labels = label_selector.match_labels.clone(); - let node_label_exprs = label_selector.match_expressions.clone(); - - let node_affinity = node_label_exprs.map(|node_label_exprs| NodeAffinity { - required_during_scheduling_ignored_during_execution: Some(NodeSelector { - node_selector_terms: vec![NodeSelectorTerm { - match_expressions: Some( - node_label_exprs - .into_iter() - .map( - |LabelSelectorRequirement { - key, - operator, - values, - }| { - NodeSelectorRequirement { - key, - operator, - values, - } - }, - ) - .collect(), - ), - ..NodeSelectorTerm::default() - }], - }), - ..NodeAffinity::default() - }); - - if let Some(node_labels) = node_labels { - self.node_selector - .get_or_insert(StackableNodeSelector { - node_selector: BTreeMap::new(), - }) - .node_selector - .extend(node_labels); - } - - if let Some(NodeAffinity { - required_during_scheduling_ignored_during_execution: - Some(NodeSelector { - node_selector_terms, - }), - .. - }) = node_affinity - { - self.node_affinity - .get_or_insert(NodeAffinity { - preferred_during_scheduling_ignored_during_execution: None, - required_during_scheduling_ignored_during_execution: None, - }) - .required_during_scheduling_ignored_during_execution - .get_or_insert(NodeSelector { - node_selector_terms: Vec::new(), - }) - .node_selector_terms - .extend(node_selector_terms); - } - } -} - #[derive(Clone, Debug, Eq, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub struct StackableNodeSelector { @@ -183,7 +109,6 @@ mod tests { api::core::v1::{NodeSelector, NodeSelectorRequirement, NodeSelectorTerm}, apimachinery::pkg::apis::meta::v1::LabelSelectorRequirement, }; - use rstest::rstest; use crate::config::fragment; @@ -430,210 +355,4 @@ mod tests { } ); } - - #[rstest] - #[case::legacy_selector_labels_specified( - r#" - matchLabels: - disktype: ssd - "#, - r#" - nodeSelector: - generation: new - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - "#, - r#" - nodeSelector: - disktype: ssd - generation: new - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - "# - )] - #[case::legacy_selector_expression_specified( - r#" - matchExpressions: - - key: topology.kubernetes.io/continent - operator: In - values: - - europe - "#, - r#" - nodeSelector: - generation: new - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - "#, - r#" - nodeSelector: - generation: new - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - - matchExpressions: - - key: topology.kubernetes.io/continent - operator: In - values: - - europe - - "# - )] - #[case::legacy_selector_expression_and_labels_specified( - r#" - matchLabels: - disktype: ssd - matchExpressions: - - key: topology.kubernetes.io/continent - operator: In - values: - - europe - "#, - r#" - nodeSelector: - generation: new - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - "#, - r#" - nodeSelector: - generation: new - disktype: ssd - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - - matchExpressions: - - key: topology.kubernetes.io/continent - operator: In - values: - - europe - - "# - )] - #[case::legacy_selector_labels_no_new_labels( - r#" - matchLabels: - disktype: ssd - "#, - r#" - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - "#, - r#" - nodeSelector: - disktype: ssd - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - "# - )] - #[case::legacy_selector_labels_no_new_labels( - r#" - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - africa-east1 - "#, - r#" - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - "#, - r#" - nodeAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - nodeSelectorTerms: - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - antarctica-east1 - - antarctica-west1 - - matchExpressions: - - key: topology.kubernetes.io/zone - operator: In - values: - - africa-east1 - "# - )] - fn test_add_legacy_selector_fn( - #[case] legacy_selector: &str, - #[case] new_selector: &str, - #[case] expected_result: &str, - ) { - let legacy_selector: LabelSelector = - serde_yaml::from_str(legacy_selector).expect("illegal test input for legacy selector"); - - let mut new_selector: StackableAffinityFragment = - serde_yaml::from_str(new_selector).expect("illegal test input for new selector"); - - let expected_result: StackableAffinityFragment = - serde_yaml::from_str(expected_result).expect("illegal test input for expected result"); - - // Merge legacy and new node selectors - #[allow(deprecated)] - new_selector.add_legacy_selector(&legacy_selector); - - assert_eq!(expected_result, new_selector); - } } diff --git a/src/product_config_utils.rs b/src/product_config_utils.rs index 7105c4d1f..2527a1e98 100644 --- a/src/product_config_utils.rs +++ b/src/product_config_utils.rs @@ -626,7 +626,6 @@ mod tests { build_config_override(file_name, GROUP_CONF_OVERRIDE), build_env_override(GROUP_ENV_OVERRIDE), build_cli_override(GROUP_CLI_OVERRIDE)), - selector: None, }}, }, (true, true, true, false) => Role { @@ -640,7 +639,6 @@ mod tests { replicas: Some(1), config: build_common_config( build_test_config(GROUP_CONFIG, GROUP_ENV, GROUP_CLI), None, None, None), - selector: None, }}, }, (true, true, false, true) => Role { @@ -657,7 +655,6 @@ mod tests { build_config_override(file_name, GROUP_CONF_OVERRIDE), build_env_override(GROUP_ENV_OVERRIDE), build_cli_override(GROUP_CLI_OVERRIDE)), - selector: None, }}, }, (true, true, false, false) => Role { @@ -674,7 +671,6 @@ mod tests { None, None, None), - selector: None, }}, }, (true, false, true, true) => Role { @@ -691,7 +687,6 @@ mod tests { build_config_override(file_name, GROUP_CONF_OVERRIDE), build_env_override(GROUP_ENV_OVERRIDE), build_cli_override(GROUP_CLI_OVERRIDE)), - selector: None, }}, }, (true, false, true, false) => Role { @@ -704,7 +699,6 @@ mod tests { role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), - selector: None, }}, }, (true, false, false, true) => Role { @@ -722,7 +716,6 @@ mod tests { build_env_override(GROUP_ENV_OVERRIDE), build_cli_override(GROUP_CLI_OVERRIDE) ), - selector: None, }}, }, (true, false, false, false) => Role { @@ -735,7 +728,6 @@ mod tests { role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), - selector: None, }}, }, (false, true, true, true) => Role { @@ -752,7 +744,6 @@ mod tests { build_config_override(file_name, GROUP_CONF_OVERRIDE), build_env_override(GROUP_ENV_OVERRIDE), build_cli_override(GROUP_CLI_OVERRIDE)), - selector: None, }}, }, (false, true, true, false) => Role { @@ -769,7 +760,6 @@ mod tests { None, None, None), - selector: None, }}, }, (false, true, false, true) => Role { @@ -781,7 +771,6 @@ mod tests { build_config_override(file_name, GROUP_CONF_OVERRIDE), build_env_override(GROUP_ENV_OVERRIDE), build_cli_override(GROUP_CLI_OVERRIDE)), - selector: None, }}, }, (false, true, false, false) => Role { @@ -793,7 +782,6 @@ mod tests { None, None, None), - selector: None, }}, }, (false, false, true, true) => Role { @@ -810,7 +798,6 @@ mod tests { build_config_override(file_name, GROUP_CONF_OVERRIDE), build_env_override(GROUP_ENV_OVERRIDE), build_cli_override(GROUP_CLI_OVERRIDE)), - selector: None, }}, }, (false, false, true, false) => Role { @@ -823,7 +810,6 @@ mod tests { role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), - selector: None, }}, }, (false, false, false, true) => Role { @@ -835,7 +821,6 @@ mod tests { build_config_override(file_name, GROUP_CONF_OVERRIDE), build_env_override(GROUP_ENV_OVERRIDE), build_cli_override(GROUP_CLI_OVERRIDE)), - selector: None, }}, }, (false, false, false, false) => Role { @@ -843,7 +828,6 @@ mod tests { role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), - selector: None, }}, }, } @@ -1047,7 +1031,6 @@ mod tests { build_config_override(file_name, "file"), build_env_override(GROUP_ENV_OVERRIDE), None), - selector: None, }}, }; @@ -1106,7 +1089,6 @@ mod tests { None, None ), - selector: None, }, role_group_2.to_string() => RoleGroup { replicas: Some(1), @@ -1116,7 +1098,6 @@ mod tests { None, None ), - selector: None, }} }), role_2.to_string() => (vec![PropertyNameKind::Cli], Role { @@ -1134,7 +1115,6 @@ mod tests { None, None ), - selector: None, }}, }) }; @@ -1196,7 +1176,6 @@ mod tests { None, None ), - selector: None, }} } ), @@ -1211,7 +1190,6 @@ mod tests { None, None ), - selector: None, }} } ), diff --git a/src/role_utils.rs b/src/role_utils.rs index e576728df..9db6c7c30 100644 --- a/src/role_utils.rs +++ b/src/role_utils.rs @@ -93,9 +93,7 @@ use crate::{ product_config_utils::Configuration, }; use derivative::Derivative; -use k8s_openapi::{ - api::core::v1::PodTemplateSpec, apimachinery::pkg::apis::meta::v1::LabelSelector, -}; +use k8s_openapi::api::core::v1::PodTemplateSpec; use kube::{runtime::reflector::ObjectRef, Resource}; use schemars::{schema::Schema, visit::Visitor, JsonSchema}; use serde::{Deserialize, Serialize}; @@ -207,7 +205,6 @@ impl Role { pod_overrides: group.config.pod_overrides, }, replicas: group.replicas, - selector: group.selector, }, ) }) @@ -225,9 +222,6 @@ pub struct RoleGroup { #[serde(flatten)] pub config: CommonConfiguration, pub replicas: Option, - // TODO Can be removed after we stop supporting this field. - // See ADR 26 Affinities - pub selector: Option, } impl RoleGroup {