testsys: `cargo make test` for ECS variants #2348

ecpullen · 2022-08-15T18:23:43Z

Issue number:

Description of changes:

Adds support for ECS variants testing. Everything works the same way as aws-k8s variants except ECS clusters are created instead of EKS, and there is not supported conformance testing for ECS variants.

Testing done:

Tested with quick and migration tests.

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.

arnaldo2792 · 2022-08-18T23:37:10Z

TESTING.md

@@ -137,7 +137,7 @@ If you run `cargo make test` for a variant that is not yet enabled, it will prin
 Check back here and follow the issues relevant to your variant of interest.

 - `aws-k8s` conformance testing is working!
- `aws-ecs`: https://github.com/bottlerocket-os/bottlerocket/issues/2150
+- `aws-ecs`: conformance and migration testing is working!


In your PR description you said that conformance isn't supported

arnaldo2792 · 2022-08-18T23:38:29Z

TESTING.md

@@ -137,7 +137,7 @@ If you run `cargo make test` for a variant that is not yet enabled, it will prin
 Check back here and follow the issues relevant to your variant of interest.


Nit: for the lack of a better place

Could you please add a description in the commit message? You could just use what you used in the PR description. This is useful for the community if they want to know more about the reasoning behind the change, without having to chase down pull requests.

TESTING.md

arnaldo2792 · 2022-08-18T23:51:24Z

TESTING.md

@@ -219,7 +248,8 @@ WORKING_BRANCH="develop"
 git checkout "${WORKING_BRANCH}"
 ```

-Next, build Bottlerocket images and repos and sync TUF repos.
+Next, build Bottlerocket images and repos and sync TUF repos. 
+The architecture and variant can be configured with `-e BUILDSYS_ARCH="aws-k8s-1.21"` and `-e BUILDSYS_VARIANT="x86_64"`.


Nit: You could just say "by setting BUILDSYS_ARCH and BUILDSYS_VARIANT``"

tools/testsys/src/aws_resources.rs

arnaldo2792 · 2022-08-19T00:06:55Z

tools/testsys/src/aws_resources.rs

+        self.variant.replace('.', "")
+    }
+
+    /// Bottlerocket cluster naming convention.


Nit: Maybe an example of how does that look like? Or is the convention documented somewhere?

arnaldo2792 · 2022-08-19T00:07:45Z

tools/testsys/src/aws_resources.rs

+    }
+}
+
+/// In order to easily create migration tests for `aws-k8s` variants we need to implement


This is not for aws-k8s variants

arnaldo2792 · 2022-08-19T00:11:47Z

tools/testsys/src/run.rs

+            if let Some(name) = crd.name() {
+                info!("Successfully added '{}'", name)
+            };


Isn't it safe to assume that at this point name() won't return None and thus you can do unwrap?

arnaldo2792 · 2022-08-19T00:12:12Z

tools/testsys/src/run.rs

@@ -252,6 +278,14 @@ pub(crate) struct TestsysImages {
    )]
    pub(crate) eks_resource: String,

+    /// Ecs resource agent uri. If not provided the latest released resource agent will be used.


Nit: s|Ecs|ECS for all occurrences

arnaldo2792 · 2022-08-19T00:13:12Z

tools/testsys/src/run.rs

@@ -268,6 +302,14 @@ pub(crate) struct TestsysImages {
    )]
    pub(crate) sonobuoy_test: String,

+    /// Ecs test agent uri. If not provided the latest released test agent will be used.


Nit: s|uri|URI

arnaldo2792 · 2022-08-24T18:52:07Z

TESTING.md

@@ -136,8 +136,8 @@ We haven't yet enabled `cargo make test` for every variant, though much of the u
 If you run `cargo make test` for a variant that is not yet enabled, it will print an error message.
 Check back here and follow the issues relevant to your variant of interest.

- `aws-k8s` conformance testing is working!
- `aws-ecs`: https://github.com/bottlerocket-os/bottlerocket/issues/2150
+- `aws-k8s` conformance and migration testing are working!


I'll keep updates in the readme related to the k8s variants in either a different commit, or a different PR.

TESTING.md

arnaldo2792 · 2022-08-24T18:56:20Z

tools/testsys/src/aws_resources.rs

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


Isn't this being formatted by cargo fmt? It seems like a very long line.

tools/testsys/src/aws_resources.rs

arnaldo2792 · 2022-08-24T19:01:41Z

tools/testsys/src/aws_resources.rs

+                            vpc: None,
+                        }
+                        .into_map()
+                        .context("Unable to convert ecs config to map")?,


Pedantic nit: s/ecs/ECS, or if you are referring to the structure, use the whole name EcsClusterConfig, the same applies to s/ec2/EC2

bcressey

LGTM once @arnaldo2792 is happy.

bcressey · 2022-08-26T18:18:51Z

tools/testsys/src/run.rs

@@ -244,31 +268,47 @@ pub(crate) struct Image {

 #[derive(Debug, Parser)]
 pub(crate) struct TestsysImages {
-    /// Eks resource agent uri. If not provided the latest released resource agent will be used.
+    /// Eks resource agent URI. If not provided the latest released resource agent will be used.


nit:

Suggested change

/// Eks resource agent URI. If not provided the latest released resource agent will be used.

/// EKS resource agent URI. If not provided the latest released resource agent will be used.

bcressey · 2022-08-26T18:29:21Z

tools/testsys/src/aws_resources.rs

+    fn kube_arch(&self) -> String {
+        self.arch.replace('_', "-")
+    }
+
+    fn kube_variant(&self) -> String {
+        self.variant.replace('.', "")
+    }


Should we rename these helper functions?

Do you have any suggestions?

arnaldo2792 · 2022-08-30T16:28:28Z

TESTING.md

+### aws-ecs
+
+You need to [build](BUILDING.md) Bottlerocket and create an AMI before you can run a test. 
+The instance type can be configured for nvidia variants by setting `TESTSYS_INSTANCE_TYPE="p3.2xlarge"`.


I would say something along these lines

The default instance type to be used is <blah>, but can be controlled by setting the environment variable `TESTSYS_INSTANCE_TYPE`. This is useful while testing NVIDIA variants, since they require instance types with support for NVIDIA GPUs.

Adds support for ecs variants testing. Everything works the same way as aws-k8s variants except ECS clusters are created instead of EKS, and there is not supported conformance testing for ecs variants.

ecpullen force-pushed the cargo-make-ecs branch 2 times, most recently from bd022af to cb775f0 Compare August 15, 2022 18:28

ecpullen requested review from bcressey and etungsten August 15, 2022 18:29

ecpullen force-pushed the cargo-make-ecs branch from cb775f0 to d5eb872 Compare August 15, 2022 19:00

arnaldo2792 reviewed Aug 19, 2022

View reviewed changes

ecpullen force-pushed the cargo-make-ecs branch from d5eb872 to f65173f Compare August 19, 2022 17:30

ecpullen requested a review from arnaldo2792 August 19, 2022 17:31

ecpullen force-pushed the cargo-make-ecs branch from f65173f to 22791d8 Compare August 19, 2022 17:32

arnaldo2792 reviewed Aug 24, 2022

View reviewed changes

ecpullen force-pushed the cargo-make-ecs branch from 22791d8 to f4c6d05 Compare August 24, 2022 19:52

ecpullen requested a review from arnaldo2792 August 24, 2022 19:52

ecpullen changed the title ~~testsys: cargo make test for ecs variants~~ testsys: cargo make test for ECS variants Aug 26, 2022

bcressey approved these changes Aug 26, 2022

View reviewed changes

ecpullen force-pushed the cargo-make-ecs branch from f4c6d05 to 788616f Compare August 29, 2022 20:20

arnaldo2792 approved these changes Aug 30, 2022

View reviewed changes

testsys: cargo make test for ecs variants

6adf946

Adds support for ecs variants testing. Everything works the same way as aws-k8s variants except ECS clusters are created instead of EKS, and there is not supported conformance testing for ecs variants.

ecpullen force-pushed the cargo-make-ecs branch from 788616f to 6adf946 Compare August 30, 2022 18:35

ecpullen merged commit 006adf0 into bottlerocket-os:develop Aug 31, 2022

ecpullen deleted the cargo-make-ecs branch August 31, 2022 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testsys: `cargo make test` for ECS variants #2348

testsys: `cargo make test` for ECS variants #2348

ecpullen commented Aug 15, 2022 •

edited

Loading

arnaldo2792 Aug 18, 2022

arnaldo2792 Aug 18, 2022

arnaldo2792 Aug 18, 2022

arnaldo2792 Aug 19, 2022

arnaldo2792 Aug 19, 2022

arnaldo2792 Aug 19, 2022

arnaldo2792 Aug 19, 2022

arnaldo2792 Aug 19, 2022

arnaldo2792 Aug 24, 2022

arnaldo2792 Aug 24, 2022

arnaldo2792 Aug 24, 2022

bcressey left a comment

bcressey Aug 26, 2022

bcressey Aug 26, 2022

ecpullen Aug 29, 2022

arnaldo2792 Aug 30, 2022

		@@ -137,7 +137,7 @@ If you run `cargo make test` for a variant that is not yet enabled, it will prin
		Check back here and follow the issues relevant to your variant of interest.

	/// Eks resource agent URI. If not provided the latest released resource agent will be used.
	/// EKS resource agent URI. If not provided the latest released resource agent will be used.

testsys: cargo make test for ECS variants #2348

testsys: cargo make test for ECS variants #2348

Conversation

ecpullen commented Aug 15, 2022 • edited Loading

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

bcressey 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

testsys: `cargo make test` for ECS variants #2348

testsys: `cargo make test` for ECS variants #2348

ecpullen commented Aug 15, 2022 •

edited

Loading