add repository field for stacks, clean up clippy wanrings #517

nhudson · 2024-01-29T22:30:35Z

Add new field repository to the Stack struct to allow setting a different repository if wanted.
- Update stack templates
Clean up clippy warnings

nhudson · 2024-01-29T22:34:11Z

tembo-operator/src/extensions/toggle.rs

@@ -405,7 +405,7 @@ async fn check_for_extensions_enabled_with_load(
        for extension in trunk_installed_extensions_not_in_all_actually_installed_extensions.clone()
        {
            let found =
-                check_for_so_files(cdb, ctx.clone(), &*pod_name, extension.name.clone()).await?;
+                check_for_so_files(cdb, ctx.clone(), &pod_name, extension.name.clone()).await?;


Fix clippy warning

nhudson · 2024-01-29T22:34:14Z

tembo-operator/src/extensions/toggle.rs

@@ -430,7 +430,7 @@ async fn check_for_extensions_enabled_with_load(
                        found = true;
                        for desired_location in desired_extension.locations {
                            let location_status = ExtensionInstallLocationStatus {
-                                enabled: Some(desired_location.enabled.clone()),
+                                enabled: Some(desired_location.enabled),


Fix clippy warning

nhudson · 2024-01-29T22:39:14Z

tembo-operator/src/trunk.rs

@@ -277,7 +277,7 @@ pub async fn get_trunk_project_metadata_for_version(
        let response_body = response.text().await?;
        let project_metadata: Vec<TrunkProjectMetadata> = serde_json::from_str(&response_body)?;
        // There will only be one index here, so we can safely assume index 0
-        let project_metadata = match project_metadata.get(0) {
+        let project_metadata = match project_metadata.first() {


Fix clippy warning

nhudson · 2024-01-29T22:39:17Z

tembo-operator/src/trunk.rs

@@ -367,7 +367,7 @@ pub async fn is_control_file_absent(
    //  where there are multiple extensions
    let control_file_absent = project_metadata
        .extensions
-        .get(0)
+        .first()


Fix clippy warning

nhudson · 2024-01-29T22:39:23Z

tembo-operator/src/trunk.rs

+    let loadable_library_name = extension_metadata
+        .loadable_libraries
+        .as_ref()
+        .and_then(|loadable_libraries| loadable_libraries.iter().find(|l| l.requires_restart))
+        .map(|loadable_library| loadable_library.library_name.clone());


Fix clippy warning

nhudson · 2024-01-29T22:39:29Z

tembo-operator/src/trunk.rs

+        let a = semver::Version::parse(a).unwrap();
+        let b = semver::Version::parse(b).unwrap();


Fix clippy warning

nhudson · 2024-01-29T22:39:32Z

tembo-operator/src/trunk.rs

@@ -550,17 +543,17 @@ mod tests {
        let extension_name = "auto_explain".to_string();
        let result = extension_name_matches_trunk_project(extension_name).await;
        assert!(result.is_ok());
-        assert_eq!(result.unwrap(), true);
+        assert!(result.unwrap());


Fix clippy warning

nhudson · 2024-01-29T22:39:35Z

tembo-operator/src/trunk.rs


        let extension_name = "pgml".to_string();
        let result = extension_name_matches_trunk_project(extension_name).await;
        assert!(result.is_ok());
-        assert_eq!(result.unwrap(), false);
+        assert!(!result.unwrap());


Fix clippy warning

nhudson · 2024-01-29T22:39:38Z

tembo-operator/src/trunk.rs


        let extension_name = "vector".to_string();
        let result = extension_name_matches_trunk_project(extension_name).await;
        assert!(result.is_ok());
-        assert_eq!(result.unwrap(), false);
+        assert!(!result.unwrap());


Fix clippy warning

nhudson · 2024-01-29T22:39:41Z

tembo-operator/src/trunk.rs

@@ -592,7 +585,7 @@ mod tests {
        let version = "15.3.0".to_string();
        let result = is_control_file_absent(trunk_project, version).await;
        assert!(result.is_ok());
-        assert_eq!(result.unwrap(), true);
+        assert!(result.unwrap());


Fix clippy warning

nhudson · 2024-01-29T22:39:43Z

tembo-operator/src/trunk.rs

@@ -618,11 +611,11 @@ mod tests {
    fn test_is_semver() {
        let version = "1.2.3".to_string();
        let result = is_semver(version);
-        assert_eq!(result, true);
+        assert!(result);


Fix clippy warning

nhudson · 2024-01-29T22:39:48Z

tembo-operator/src/trunk.rs


        let version = "1.2".to_string();
        let result = is_semver(version);
-        assert_eq!(result, false);
+        assert!(!result);


Fix clippy warning

nhudson · 2024-01-29T22:40:01Z

tembo-operator/tests/integration_tests.rs

+                    } else if has_extension {
+                        println!(
+                            "CoreDB {} has extension {} enabled in status",
+                            name, extension
+                        );
+                        return Ok(());


Fix clippy warning

nhudson · 2024-01-29T22:40:04Z

tembo-operator/tests/integration_tests.rs

@@ -2332,7 +2329,7 @@ mod test {
        });
        let params = PatchParams::apply("tembo-integration-test");
        let patch = Patch::Apply(&coredb_json);
-        let coredb_resource = coredbs.patch(name, &params, &patch).await.unwrap();
+        let _coredb_resource = coredbs.patch(name, &params, &patch).await.unwrap();


Fix clippy warning

nhudson · 2024-01-29T22:40:08Z

tembo-operator/tests/integration_tests.rs

@@ -2346,7 +2343,7 @@ mod test {
        // Check status.extensions.locations is not empty
        let coredb_resource = coredbs.get(name).await.unwrap();
        for extension in coredb_resource.status.unwrap().extensions.unwrap() {
-            assert!(extension.locations.len() > 0);
+            assert!(!extension.locations.is_empty());


Fix clippy warning

sjmiller609

Thanks for addressing my concerns. There is one other thing that I think would be nice to do, maybe reducing one duplication of configuration and helping test image updates in stack yamls.

sjmiller609 · 2024-01-29T23:25:55Z

tembo-operator/src/defaults.rs

 }

 pub fn default_llm_image() -> String {
-    "quay.io/tembo/ml-cnpg:15.3.0-1-63e32a1".to_owned()
+    "ml-cnpg:15.3.0-1-63e32a1".to_owned()


Is it possible for the values in this file to come from the stack yamls? So it's like if someone updates the image in a stack yaml, then the operator tests run in CI and it's using the new image from the stack yaml for which there are operator tests?

If we don't do that, then our operator tests use the image configured here in the defaults, but a future change to stack yaml, then following that to update in control plane, can result in us use image defaults that have not been tested by the functional testing in the operator code

nhudson · 2024-01-31T22:31:33Z

conductor/src/lib.rs

+
+    // Watch for deletion
+    let wp = WatchParams {
+        field_selector: Some(format!("metadata.name={}", name)),
+        ..WatchParams::default()
+    };
+    let stream = coredb_api
+        .watch(&wp, "0")
+        .await
+        .map_err(ConductorError::KubeError)?;
+
+    let mut pinned_stream = Box::pin(stream);
+
+    while let Some(status) = pinned_stream.try_next().await? {
+        if let WatchEvent::Deleted(_) = status {
+            info!("CoreDB {} deleted", name);
+            return Ok(());
+        }
+    }
+


Adding a WatchEvent to conductor. This will ensure that the function will not return unless there is an error or if the CoreDB CR is actually deleted.

I think there could be an issue with this, because any waiting logic in conductor blocks the processing of other events. This means in the future event that a namespace takes awhile to delete, then this replica of conductor will stop processing all other events including new instances, update instances, and so on. That's why I think it is problematic to do "wait" style logic in conductor or operator, rather "requeue" style, so that waits instead handled like interrupts.

nhudson · 2024-01-31T22:31:54Z

conductor/src/lib.rs

+
+    // Watch for deletion
+    let wp = WatchParams {
+        field_selector: Some(format!("metadata.name={}", name)),
+        ..WatchParams::default()
+    };
+    let stream = ns_api
+        .watch(&wp, "0")
+        .await
+        .map_err(ConductorError::KubeError)?;
+
+    let mut pinned_stream = Box::pin(stream);
+
+    while let Some(status) = pinned_stream.try_next().await? {
+        if let WatchEvent::Deleted(_) = status {
+            info!("Namespace {} deleted", name);
+            return Ok(());
+        }
+    }


Adding a WatchEvent to conductor. This will ensure that the function will not return unless there is an error or if the Namespace is actually deleted.

add repository field for stacks, clean up clippy wanrings

7ffef5e

nhudson self-assigned this Jan 29, 2024

nhudson commented Jan 29, 2024

View reviewed changes

nhudson marked this pull request as ready for review January 29, 2024 22:52

nhudson requested review from sjmiller609, ianstanton and ChuckHend as code owners January 29, 2024 22:52

update stack container image versions

f056b3f

sjmiller609 approved these changes Jan 29, 2024

View reviewed changes

nhudson added 2 commits January 30, 2024 08:13

use container image from stack as defaults

7412b2d

Merge branch 'main' into nhudson/TEM-2538

4728615

sjmiller609 approved these changes Jan 30, 2024

View reviewed changes

nhudson added 4 commits January 31, 2024 12:34

adjust conductor tests so they use the correct spec values when updating

5953314

fix tembo-operator env variables

cbf1394

remove restart action, as per the test the pod will restart on it's own

4d64912

make sure resources are deleted before returning

443c835

fix tests to check for cf template deletion

9b7f0fd

nhudson commented Jan 31, 2024

View reviewed changes

delete cf stack if it exists

3f1b833

sjmiller609 self-requested a review January 31, 2024 23:09

nhudson added 2 commits January 31, 2024 18:16

remove watch from delete functions

0825b39

clean up clippy

9d6d4f0

sjmiller609 approved these changes Feb 1, 2024

View reviewed changes

nhudson merged commit 763aca7 into main Feb 1, 2024
19 checks passed

nhudson deleted the nhudson/TEM-2538 branch February 1, 2024 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add repository field for stacks, clean up clippy wanrings #517

add repository field for stacks, clean up clippy wanrings #517

nhudson commented Jan 29, 2024 •

edited

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

nhudson Jan 29, 2024

sjmiller609 left a comment

sjmiller609 Jan 29, 2024

nhudson Jan 31, 2024

sjmiller609 Jan 31, 2024

nhudson Jan 31, 2024

		let a = semver::Version::parse(a).unwrap();
		let b = semver::Version::parse(b).unwrap();

add repository field for stacks, clean up clippy wanrings #517

add repository field for stacks, clean up clippy wanrings #517

Conversation

nhudson commented Jan 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjmiller609 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhudson commented Jan 29, 2024 •

edited