testsys: Refactor code base #2560

ecpullen · 2022-11-08T00:50:38Z

Changes the way TestSys crd's are created while keeping the same interface.

Issue number:

Description of changes:

The basis of the changes revolve around CrdCreator. The CrdCreator is responsible for creating all CRDs for a given variant family (aws-k8s, aws-ecs). The CrdCreator has a few functions that need to be implemented that enables support for the standard testing types. To add a new testing family, only a CrdCreator needs to be created.
CRD creation is done based on structs instead of function parameters (OrchestratorInput, BottlerocketInput, etc).
Additional labels are added, and support for conflicting resources has been streamlined using CrdInput::existing_resources().
EC2 CRD creation has been de-duped for aws-k8s and aws-ecs

Testing done:

Created ECS and EKS testing with:

cargo make -e BUILDSYS_VARIANT="aws-ecs-1" test
cargo make -e TESTSYS_TEST="migration" test
cargo make test

After testing was complete the following status was left:

 NAME                                TYPE           STATE           PASSED       SKIPPED      FAILED
 x86-64-aws-ecs-1-test               Test           passed          1            0            0
 x86-64-aws-k8s-123                  Resource       completed
 x86-64-aws-k8s-123-1-initial        Test           passed          1            7051         0
 x86-64-aws-k8s-123-2-migrate        Test           passed          2            0            0
 x86-64-aws-k8s-123-3-migrated       Test           passed          1            7051         0
 x86-64-aws-k8s-123-4-migrate        Test           passed          2            0            0
 x86-64-aws-k8s-123-5-final          Test           passed          1            7051         0
 x86-64-aws-k8s-123-test             Test           passed          1            7051         0

Note: The instance providers are not in the status because DestructionPolicy::OnTestSuccess was used.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

ecpullen · 2022-11-09T19:05:51Z

^ Convert error from anyhow to Snafu.

ecpullen · 2022-11-09T22:33:03Z

^ Add documentation

etungsten

Refactor looks good to me. The testing looks good as well.

webern

Some recommendations. The biggest one is that I think we should stick with cluster over orchestrator, though I don't remember the reasoning behind the choice.

webern · 2022-11-10T22:49:36Z

tools/testsys/src/sonobuoy.rs

+/// Create a Sonobuoy CRD for K8s conformance and quick testing.
+pub(crate) fn sonobuoy_crd(test_input: TestInput) -> Result<Test> {
+    let cluster_resource_name = test_input
+        .orchestrator_crd_name


I'm leaning toward using the word cluster instead of orchestrator since the word cluster is being used everywhere else already and introducing a new word is cognitively dissonant.

webern · 2022-11-10T22:53:04Z

tools/testsys/src/error.rs

+    #[snafu(display("{} was missing from {}", item, what))]
+    Missing { item: String, what: String },
+
+    #[snafu(context(false), display("{}", source))]


Is context(false) because the Error type is not std::error::Error? I haven't used that feature before.

#[snafu(context(false), display("{}", source))]

context(false) makes it so you don't need to add the .context() for error conversion.

webern · 2022-11-10T23:11:44Z

tools/testsys/src/run.rs

+#[serde(untagged)]
+pub(crate) enum TestType {
+    Known(KnownTestType),
+    Unknown(String),


It's not clear looking at this definition what the String represents. As a doc comment for Unknown.

webern · 2022-11-10T23:24:14Z

tools/testsys/src/aws_resources.rs

+use std::fs::File;
+
+/// Get the AMI for the given `region` from the `ami_input` file.
+pub(crate) fn ami(ami_input: &str, region: &str) -> Result<String> {


It feels like we should have a common struct for serializing/deserializing this

There is but it is private in Pubsys

webern · 2022-11-10T23:28:44Z

tools/testsys/src/aws_resources.rs

    let config = aws_config::from_env()
        .region(Region::new(region.into()))
        .load()
        .await;
    let ec2_client = aws_sdk_ec2::Client::new(&config);
+    // Find all images named `name` on `arch` in the `region`.


The owners 'self' default assumes that this is being used against developer-built images, right? we can pass in the AMI-ID for other use cases?

webern · 2022-11-10T23:29:30Z

tools/testsys/src/aws_resources.rs

    }
 }
+
+#[derive(Clone, Debug, Deserialize)]
+pub(crate) struct AmiImage {


What was the purpose of this type? Validation?

Easy deserialization. That was not added in the refactor.

webern · 2022-11-16T17:28:34Z

tools/testsys/src/aws_ecs.rs

+        get_ami_id(
+            format!(
+                "bottlerocket-{}-{}-{}-{}",
+                crd_input.variant, crd_input.arch, crd_input.starting_version.as_ref().context(error::InvalidSnafu{what: "The starting version must be provided for migration testing"})?, self.migrate_starting_commit.as_ref().context(error::InvalidSnafu{what: "The commit for the starting version must be provided if the starting image id is not"})?


Can you wrap this? Sadly rustfmt can't seem to handle things inside of macro calls.

webern · 2022-11-16T17:30:12Z

tools/testsys/src/aws_k8s.rs

+        get_ami_id(
+            format!(
+                "bottlerocket-{}-{}-{}-{}",
+                crd_input.variant, crd_input.arch, crd_input.starting_version.as_ref().context(error::InvalidSnafu{what: "The starting version must be provided for migration testing"})?, self.migrate_starting_commit.as_ref().context(error::InvalidSnafu{what: "The commit for the starting version must be provided if the starting image id is not"})?


I'm seeing repeated code here. Maybe put this in an ami_name function.

webern · 2022-11-16T17:31:04Z

tools/testsys/src/aws_k8s.rs

+            .crd_input
+            .existing_crds(
+                &labels,
+                &["testsys/cluster", "testsys/type", "testsys/region"],


Maybe we should think about constants for these. Technically even the array could be a constant but maybe that's not necessary.

I think in this case, it's not necessary.

webern · 2022-11-16T17:31:45Z

tools/testsys/src/crds.rs

+            &self.repo_config.metadata_base_url,
+            &self.repo_config.targets_url,
+        ) {
+            debug!(


Can we enable debug log level through cargo make currently? Aside: it might be nice to have a globally respected log level for Bottlerocket build and test processes that is respected throughout Makefile.toml.

-e RUST_LOG=debug can be used to see those logs.

ecpullen · 2022-11-16T20:26:33Z

^

Changed orchestrator -> cluster
Added doc comment for TestType::Unknown
Manual text wrapping for large format! block
Created ami_name function

Changes the way TestSys crd's are created while keeping the same interface.

ecpullen force-pushed the refactor branch 2 times, most recently from abe13b1 to 0cae019 Compare November 8, 2022 01:01

ecpullen requested a review from webern November 8, 2022 01:14

ecpullen force-pushed the refactor branch from 0cae019 to 16754dd Compare November 9, 2022 19:05

ecpullen force-pushed the refactor branch 2 times, most recently from 9b684ca to 497aecb Compare November 9, 2022 22:32

ecpullen force-pushed the refactor branch from 497aecb to aa32174 Compare November 10, 2022 22:51

ecpullen marked this pull request as ready for review November 10, 2022 23:19

ecpullen mentioned this pull request Nov 11, 2022

error when trying to use cargo make test with ecs bottlerocket-os/bottlerocket-test-system#653

Closed

ecpullen requested a review from etungsten November 14, 2022 17:38

etungsten approved these changes Nov 15, 2022

View reviewed changes

webern approved these changes Nov 16, 2022

View reviewed changes

ecpullen force-pushed the refactor branch from aa32174 to f15793b Compare November 16, 2022 20:24

testsys: Refactor code base

20a197c

Changes the way TestSys crd's are created while keeping the same interface.

ecpullen force-pushed the refactor branch from f15793b to 20a197c Compare November 16, 2022 21:19

ecpullen merged commit 56fab98 into bottlerocket-os:develop Nov 18, 2022

ecpullen deleted the refactor branch November 18, 2022 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testsys: Refactor code base #2560

testsys: Refactor code base #2560

ecpullen commented Nov 8, 2022 •

edited

ecpullen commented Nov 9, 2022

ecpullen commented Nov 9, 2022

etungsten left a comment

webern left a comment

webern Nov 10, 2022

webern Nov 10, 2022

ecpullen Nov 16, 2022

webern Nov 10, 2022

webern Nov 10, 2022

ecpullen Nov 16, 2022

webern Nov 10, 2022

ecpullen Nov 16, 2022

webern Nov 10, 2022

ecpullen Nov 16, 2022

webern Nov 16, 2022

webern Nov 16, 2022

webern Nov 16, 2022

ecpullen Nov 16, 2022

webern Nov 16, 2022

ecpullen Nov 16, 2022

ecpullen commented Nov 16, 2022

testsys: Refactor code base #2560

testsys: Refactor code base #2560

Conversation

ecpullen commented Nov 8, 2022 • edited

ecpullen commented Nov 9, 2022

ecpullen commented Nov 9, 2022

etungsten left a comment

Choose a reason for hiding this comment

webern 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

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

ecpullen commented Nov 16, 2022

ecpullen commented Nov 8, 2022 •

edited