testsys: `aws-k8s` migration testing with `cargo make test` #2273

ecpullen · 2022-07-14T16:27:04Z

Issue number:
#2149

Description of changes:
Introduces migrations to cargo make test. The added documentation in TESTING.md will help any user set up the necessary infrastructure to run migration testing with all aws-k8s variants.

Testing done:
Used cargo make -e TESTSYS_TEST=migration test to run a migration test on aws-k8s-1.21 and all tests passed successfully.

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.

Makefile.toml

TESTING.md

tools/Cargo.lock

ecpullen · 2022-07-21T21:14:37Z

^ addressed @jpculp comments

arnaldo2792 · 2022-07-19T16:52:24Z

tools/testsys/src/aws_resources.rs

+            migrate_to_version: self
+                .migrate_to_version
+                .as_ref()
+                .context("You must provide a target version for upgrade downgrade testing.")?


A little late to comment on this, but I just realized that in all the Bottlerocket repo, we don't use anyhow and instead we use snafu. I think we should be consistent and move at least this part of testsys to snafu. I'm not blocking on this though but an issue should be good.

I will add an issue for this.

tools/testsys/src/aws_resources.rs

arnaldo2792 · 2022-07-21T18:47:02Z

tools/testsys/src/aws_resources.rs

+/// An enum to differentiate between upgrade and downgrade tests. `MigrationVersion::Starting` will
+/// create a migration to the starting version.


Nit: comments related to each enum should be on top of the enum.

arnaldo2792 · 2022-07-21T18:49:50Z

tools/testsys/src/aws_resources.rs

+            MigrationVersion::Starting => migration.starting_version.to_string(),
+            MigrationVersion::Migrated => migration.migrate_to_version.to_string(),


starting_version and migrate_to_version are both String, why the need of to_string()?

tools/testsys/src/aws_resources.rs

Makefile.toml

TESTING.md

ecpullen · 2022-07-21T22:54:33Z

^ addresses @arnaldo2792 comments

arnaldo2792 · 2022-07-22T19:13:27Z

TESTING.md

+In order to accomplish this a few artifacts need to be created:
+* `TUF` repos that are publicly accessible
+* A previous release of bottlerocket signed with available keys
+* The ami id from the previous release


Nit: sed 's/ami/AMI'

arnaldo2792 · 2022-07-22T19:14:14Z

TESTING.md

+Migration testing starts testsys at a starting version, or a provided initial image id and migrates instances to the target version.
+In order to accomplish this a few artifacts need to be created:
+* `TUF` repos that are publicly accessible
+* A previous release of bottlerocket signed with available keys


Nit: sed 's/bottlerocket/Bottlerocket'

arnaldo2792 · 2022-07-22T19:15:23Z

TESTING.md

+* `TUF` repos that are publicly accessible
+* A previous release of bottlerocket signed with available keys
+* The ami id from the previous release
+* Build images and repos for current changes


Nit: a link to the documentation that explains how to build and publish repos would be nice

arnaldo2792 · 2022-07-22T19:24:27Z

TESTING.md

+
+#### Prepare `Infra.toml`
+
+A publicly available s3 bucket is needed to complete the next steps, once a bucket is created the steps may be continued.


This is not true, a contributor can use whatever they want to store their TUF repositories, so it will be best to say "the URL of a TUF repository" or something along those lines.

The example steps I provide include syncing tuf repos with the pre-update metadata. I was trying to make the process of onboarding to testsys as easy as possible by providing a sample workflow from start to finish. I can add to the beginning that it is not required to have, but this example will use it.

I think it will be best to point whomever reads this guide to this document so that they are aware that they are free to decide where to store their TUF repo. I would definitely remove the "publicly" available part since that isn't a good practice, and we shouldn't advocate for that. It will be best to say "use the URL of an accessible TUF repo"; and for your example, assume the contributor followed the link you will add, and that they have a URL that is on top of a properly configured S3 bucket (that's what ultimately happened in the document I linked).

arnaldo2792 · 2022-07-22T19:25:22Z

TESTING.md

+#### Prepare `Infra.toml`
+
+A publicly available s3 bucket is needed to complete the next steps, once a bucket is created the steps may be continued.
+`Infra.toml` is used by testsys to determine tuf repo locations, so `metadata_base_url` and `targets_base_url` need to be set based on the s3 bucket. The examples below assume `metadata_base_url` is the top level directory of the bucket and `targets_base_url` is a directory named `targets` in the top level directory. 


Nit: a line per sentence

arnaldo2792 · 2022-07-22T22:07:23Z

TESTING.md

+#### Starting bottlerocket images
+
+In this example we will use `v1.8.0` as our starting bottlerocket version, but any tag from bottlerocket will work. 
+The following bash will checkout the proper branch from git and create the build images and repos for testing.


Suggested change

The following bash will checkout the proper branch from git and create the build images and repos for testing.

The following bash script will checkout the proper branch from git and create the build images and TUF repos for testing.

arnaldo2792 · 2022-07-22T22:08:19Z

TESTING.md

+
+```shell
+export TESTSYS_STARTING_VERSION="1.8.0"
+git checkout v${TESTSYS_STARTING_VERSION}


Suggested change

git checkout v${TESTSYS_STARTING_VERSION}

git checkout "v${TESTSYS_STARTING_VERSION}"

arnaldo2792 · 2022-07-22T22:08:52Z

TESTING.md

+cargo make repo
+```
+
+Next, the repos need to be added to the `TUF` repos (s3 bucket). Make sure to change `S3_BUCKET_NAME` to the `TUF` repo bucket name.


I think all mentions of a S3 bucket should be removed, folks in the community may have their own way to update their TUF repos.

See above comment. Should we remove the step by step guide so that we can remove the s3 bucket bits?

arnaldo2792 · 2022-07-22T22:10:15Z

TESTING.md

+
+```shell
+WORKING_BRANCH="develop"
+git checkout ${WORKING_BRANCH}


Nit:

Suggested change

git checkout ${WORKING_BRANCH}

git checkout "${WORKING_BRANCH}"

arnaldo2792 · 2022-07-22T22:10:57Z

TESTING.md

+git checkout ${WORKING_BRANCH}
+```
+
+Now, [Release.toml](Release.toml) may need to be updated.


For this testing it must, right?

For using develop yes. But if someone wanted to try building 1.6.0 and 1.6.1 to see how testsys migrations work they wouldn't need to.

Makefile.toml

TESTING.md

tools/testsys/Cargo.toml

TESTING.md

ecpullen · 2022-07-27T21:44:36Z

^^ Address @etungsten comments
^ rebase to develop

Makefile.toml

TESTING.md

ecpullen · 2022-08-02T21:53:38Z

^ spell check on markdown and automatically determine correct AMI ID using previous bottlerocket release tag.

bcressey · 2022-08-05T21:13:00Z

TESTING.md

+The last step with the older Bottlerocket version is retrieving the AMI that was created.
+
+```shell
+BUILDSYS_ARCH="x86_64"
+BUILDSYS_VARIANT="aws-k8s-1.21"
+REGION="us-west-2"
+
+AMIS_JSON="build/images/${BUILDSYS_ARCH}-${BUILDSYS_VARIANT}/latest/bottlerocket-${BUILDSYS_VARIANT}-${BUILDSYS_ARCH}-amis.json"


Is this step still needed for something?

bcressey · 2022-08-05T21:13:55Z

TESTING.md

+cargo make repo
+```
+
+Next, the local repos need to be added to the TUF repos. 


Suggested change

Next, the local repos need to be added to the TUF repos.

Remember to sync your TUF repos with the new metadata and targets.

bcressey · 2022-08-05T21:24:40Z

tools/testsys/src/aws_resources.rs

+    /// Creates 5 `Test` crds for migration testing. Consisting of a `quick` test, a migration to
+    /// the target version, a `quick` test to validate the migration, a migration to the starting
+    /// version, and a `quick` test to validate the second migration.


nit: this sort of comment seems like it could get outdated pretty quickly - it's specific in both the number and order of the tests.

It might be better to document each of the steps inline, since there's a natural separation after each depends_on.push() to talk about what & why each CRD is added.

bcressey · 2022-08-05T21:27:39Z

tools/testsys/src/aws_resources.rs

+        let initial = self.sonobuoy_crd("-1-initial", SonobuoyMode::Quick, None, testsys_images)?;
+        depends_on.push(initial.name().context("Crd missing name")?);
+        let start_migrate = self.migration_crd(
+            format!("{}-2-migrate", self.cluster_name()),


Not a big deal here but I wonder if we could do the numbering somewhere else, so it's automatically part of the name in the output based on the position in the vector.

bcressey · 2022-08-05T21:32:18Z

tools/testsys/src/aws_resources.rs

+            Some(depends_on.clone()),
+            testsys_images,
+        )?;
+        depends_on.push(start_migrate.name().context("Crd missing name")?);


Also not a big deal here but should "name" be a required field or have a default value?

I wonder if the cases where the name is optional are worth the added friction in what feels like the normal case here. You could potentially have a NamedCrd type and add a From impl to Crd for it.

The crd itself (for tests and resources) has the name field as an Option so somewhere there needs to be an unwrap() or context() to read it unless we default with "", but that has the potential to cause issues if the field was missing for some reason.

bcressey · 2022-08-05T21:39:10Z

tools/testsys/src/aws_resources.rs

+    /// Return the name of the instance provider that the migration agents should use to get the
+    /// instance ids.
+    fn instance_provider(&self) -> String;


This seems like a place where stronger typing could be useful. It sort of sticks out as having not very much to do with migration testing.

I was wondering if an instance provider trait would be helpful or not.

tools/testsys/src/aws_resources.rs

tools/testsys/src/run.rs

ecpullen force-pushed the cargo-make-updown branch from 2e1754e to 9dc1229 Compare July 14, 2022 16:31

ecpullen changed the title ~~Cargo make updown~~ aws-k8s migration testing with cargo make test Jul 14, 2022

ecpullen changed the title ~~aws-k8s migration testing with cargo make test~~ testsys: aws-k8s migration testing with cargo make test Jul 14, 2022

ecpullen force-pushed the cargo-make-updown branch from 9dc1229 to 7c8803f Compare July 15, 2022 11:49

jpculp reviewed Jul 19, 2022

View reviewed changes

ecpullen force-pushed the cargo-make-updown branch from 7c8803f to 2cec493 Compare July 21, 2022 21:13

arnaldo2792 requested changes Jul 21, 2022

View reviewed changes

ecpullen force-pushed the cargo-make-updown branch from 2cec493 to 373ed74 Compare July 21, 2022 22:53

ecpullen requested a review from arnaldo2792 July 21, 2022 22:54

arnaldo2792 requested changes Jul 22, 2022

View reviewed changes

ecpullen force-pushed the cargo-make-updown branch from 373ed74 to 8f3033a Compare July 25, 2022 17:30

ecpullen mentioned this pull request Jul 25, 2022

testsys: convert error from anyhow to snafu #2297

Closed

ecpullen requested a review from arnaldo2792 July 25, 2022 18:10

etungsten reviewed Jul 27, 2022

View reviewed changes

ecpullen force-pushed the cargo-make-updown branch 2 times, most recently from d512ebc to 0463854 Compare July 27, 2022 21:44

ecpullen requested a review from etungsten July 27, 2022 21:47

etungsten approved these changes Jul 27, 2022

View reviewed changes

bcressey reviewed Aug 1, 2022

View reviewed changes

Makefile.toml Outdated Show resolved Hide resolved

TESTING.md Outdated Show resolved Hide resolved

TESTING.md Outdated Show resolved Hide resolved

TESTING.md Outdated Show resolved Hide resolved

TESTING.md Outdated Show resolved Hide resolved

ecpullen force-pushed the cargo-make-updown branch from 0463854 to ef9d87f Compare August 2, 2022 21:51

ecpullen requested a review from bcressey August 2, 2022 21:53

ecpullen force-pushed the cargo-make-updown branch from ef9d87f to 8c91570 Compare August 3, 2022 17:36

bcressey reviewed Aug 5, 2022

View reviewed changes

ecpullen force-pushed the cargo-make-updown branch from 8c91570 to 97dcd67 Compare August 8, 2022 15:59

ecpullen requested a review from bcressey August 8, 2022 19:44

ecpullen self-assigned this Aug 9, 2022

bcressey approved these changes Aug 12, 2022

View reviewed changes

ecpullen force-pushed the cargo-make-updown branch from 452c8ce to 8505fc0 Compare August 12, 2022 20:26

testsys: migration support for aws-k8s variants

e0b94ff

ecpullen force-pushed the cargo-make-updown branch from 8505fc0 to e0b94ff Compare August 12, 2022 20:27

arnaldo2792 approved these changes Aug 12, 2022

View reviewed changes

ecpullen merged commit cc1e7fe into bottlerocket-os:develop Aug 12, 2022

ecpullen deleted the cargo-make-updown branch August 12, 2022 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testsys: `aws-k8s` migration testing with `cargo make test` #2273

testsys: `aws-k8s` migration testing with `cargo make test` #2273

ecpullen commented Jul 14, 2022

ecpullen commented Jul 21, 2022

arnaldo2792 Jul 19, 2022

ecpullen Jul 21, 2022

arnaldo2792 Jul 21, 2022

arnaldo2792 Jul 21, 2022

ecpullen commented Jul 21, 2022

arnaldo2792 Jul 22, 2022

arnaldo2792 Jul 22, 2022

arnaldo2792 Jul 22, 2022

arnaldo2792 Jul 22, 2022

ecpullen Jul 22, 2022

arnaldo2792 Jul 23, 2022

arnaldo2792 Jul 22, 2022

arnaldo2792 Jul 22, 2022

arnaldo2792 Jul 22, 2022

arnaldo2792 Jul 22, 2022

ecpullen Jul 22, 2022

arnaldo2792 Jul 22, 2022

arnaldo2792 Jul 22, 2022

ecpullen Jul 22, 2022

ecpullen commented Jul 27, 2022

ecpullen commented Aug 2, 2022

bcressey Aug 5, 2022

bcressey Aug 5, 2022

bcressey Aug 5, 2022

bcressey Aug 5, 2022

bcressey Aug 5, 2022

ecpullen Aug 5, 2022

bcressey Aug 5, 2022

ecpullen Aug 5, 2022

		/// An enum to differentiate between upgrade and downgrade tests. `MigrationVersion::Starting` will
		/// create a migration to the starting version.

		MigrationVersion::Starting => migration.starting_version.to_string(),
		MigrationVersion::Migrated => migration.migrate_to_version.to_string(),


		#### Prepare `Infra.toml`

		A publicly available s3 bucket is needed to complete the next steps, once a bucket is created the steps may be continued.

	The following bash will checkout the proper branch from git and create the build images and repos for testing.
	The following bash script will checkout the proper branch from git and create the build images and TUF repos for testing.

	git checkout v${TESTSYS_STARTING_VERSION}
	git checkout "v${TESTSYS_STARTING_VERSION}"

	git checkout ${WORKING_BRANCH}
	git checkout "${WORKING_BRANCH}"

	Next, the local repos need to be added to the TUF repos.
	Remember to sync your TUF repos with the new metadata and targets.

testsys: aws-k8s migration testing with cargo make test #2273

testsys: aws-k8s migration testing with cargo make test #2273

Conversation

ecpullen commented Jul 14, 2022

ecpullen commented Jul 21, 2022

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 Jul 21, 2022

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 Jul 27, 2022

ecpullen commented Aug 2, 2022

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

testsys: `aws-k8s` migration testing with `cargo make test` #2273

testsys: `aws-k8s` migration testing with `cargo make test` #2273