diff --git a/CHANGELOG.md b/CHANGELOG.md index 468f63ded..58bac61d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,8 +14,10 @@ All notable changes to this project will be documented in this file. - Move `stackable_operator::label_selector::convert_label_selector_to_query_string` into `kvp` module. The conversion functionality now is encapsulated in a new trait `LabelSelectorExt`. An instance of a `LabelSelector` can now be converted into a query string by calling the associated function `ls.to_query_string()` ([#684]). +- BREAKING: Remove legacy node selector on `RoleGroup` ([#652]). [#684]: https://github.com/stackabletech/operator-rs/pull/684 +[#652]: https://github.com/stackabletech/operator-rs/pull/652 ## [0.58.1] - 2023-12-12 diff --git a/src/commons/affinity.rs b/src/commons/affinity.rs index 5c94db2aa..6bbaf8ff0 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}; @@ -43,79 +42,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 { @@ -185,7 +111,6 @@ mod tests { api::core::v1::{NodeSelector, NodeSelectorRequirement, NodeSelectorTerm}, apimachinery::pkg::apis::meta::v1::LabelSelectorRequirement, }; - use rstest::rstest; use crate::config::fragment; @@ -432,210 +357,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 59c3dccc5..ff810340f 100644 --- a/src/product_config_utils.rs +++ b/src/product_config_utils.rs @@ -636,7 +636,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 { @@ -651,7 +650,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 { @@ -669,7 +667,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 { @@ -687,7 +684,6 @@ mod tests { None, None, None), - selector: None, }}, }, (true, false, true, true) => Role { @@ -705,7 +701,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 { @@ -719,7 +714,6 @@ mod tests { role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), - selector: None, }}, }, (true, false, false, true) => Role { @@ -738,7 +732,6 @@ mod tests { build_env_override(GROUP_ENV_OVERRIDE), build_cli_override(GROUP_CLI_OVERRIDE) ), - selector: None, }}, }, (true, false, false, false) => Role { @@ -752,7 +745,6 @@ mod tests { role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), - selector: None, }}, }, (false, true, true, true) => Role { @@ -770,7 +762,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 { @@ -788,7 +779,6 @@ mod tests { None, None, None), - selector: None, }}, }, (false, true, false, true) => Role { @@ -801,7 +791,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 { @@ -814,7 +803,6 @@ mod tests { None, None, None), - selector: None, }}, }, (false, false, true, true) => Role { @@ -832,7 +820,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 { @@ -846,7 +833,6 @@ mod tests { role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), - selector: None, }}, }, (false, false, false, true) => Role { @@ -859,7 +845,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 { @@ -868,7 +853,6 @@ mod tests { role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), - selector: None, }}, }, } @@ -1073,7 +1057,6 @@ mod tests { build_config_override(file_name, "file"), build_env_override(GROUP_ENV_OVERRIDE), None), - selector: None, }}, }; @@ -1137,7 +1120,6 @@ mod tests { None, None ), - selector: None, }, role_group_2.to_string() => RoleGroup { replicas: Some(1), @@ -1147,7 +1129,6 @@ mod tests { None, None ), - selector: None, }} }), role_2.to_string() => (vec![PropertyNameKind::Cli], Role { @@ -1166,7 +1147,6 @@ mod tests { None, None ), - selector: None, }}, }) }; @@ -1233,7 +1213,6 @@ mod tests { None, None ), - selector: None, }} } ), @@ -1249,7 +1228,6 @@ mod tests { None, None ), - selector: None, }} } ), diff --git a/src/role_utils.rs b/src/role_utils.rs index f9072f7eb..85733ef35 100644 --- a/src/role_utils.rs +++ b/src/role_utils.rs @@ -94,9 +94,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::JsonSchema; use serde::{Deserialize, Serialize}; @@ -218,7 +216,6 @@ where pod_overrides: group.config.pod_overrides, }, replicas: group.replicas, - selector: group.selector, }, ) }) @@ -250,9 +247,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 {