-
Notifications
You must be signed in to change notification settings - Fork 512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the testsys
cli and implement cargo make test
for aws-k8s
#2165
Conversation
testsys
cli and implement cargo make test
for aws-k8s
^ Removes inconsistencies between |
^ Cargo fmt |
^ rebase to develop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪐
420dde6
to
399abbe
Compare
^ Addresses all comments. Adds new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please update the PR description to include testing of the new cargo make
tasks. A demonstration of the invocation and what get printed out would be nice.
11a746c
to
027ceeb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been clear in documentation that the testing feature is in rapid iteration, so none of my comments are blocking. This is great! 🚀
Makefile.toml
Outdated
''' | ||
] | ||
|
||
[tasks.test-setup] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name isn't working for me. Most of the other targets are verb-noun
and it's not entirely clear what test-setup
might be doing. It's not a blocker but here are a couple of ideas:
setup-test-cluster
install-test-system
tools/testsys/src/status.rs
Outdated
use model::test_manager::{SelectionParams, TestManager}; | ||
use terminal_size::{Height, Width}; | ||
|
||
/// Check the status of a TestSys object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Check the status of a TestSys object. | |
/// Check the status of TestSys objects. |
tools/testsys/src/run.rs
Outdated
pub(crate) struct Image { | ||
pub(crate) id: String, | ||
#[serde(rename = "name")] | ||
pub(crate) _name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here? Is it required by k8s? Maybe document why there is an unused sctruct field.
tools/testsys/src/run.rs
Outdated
#[derive(Debug, Deserialize)] | ||
#[serde(rename_all = "lowercase")] | ||
pub(crate) enum TestType { | ||
/// Run conformance testing on a given arch and variant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these could be more generic and more specific at the same time:
/// Run conformance testing on a given arch and variant | |
/// Conformance testing is a full integration test that asserts that Bottlerocket is working for | |
/// customer workloads. For k8s variants, for example, this will run the full suite of sonobuoy | |
/// conformance tests. |
tools/testsys/src/run.rs
Outdated
pub(crate) enum TestType { | ||
/// Run conformance testing on a given arch and variant | ||
Conformance, | ||
/// Run a quick test on a given arch and variant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've mentioned ECS here even though it's not yet supported. But ECS is a fast-follow, and I think this is better than forgetting to augment the documentation with the ECS example.
/// Run a quick test on a given arch and variant | |
/// Run a quick test that ensures a basic workload can run on Bottlerocket. For example, on k8s | |
/// variance this will run sonobuoy in "quick" mode. For ECS variants, this will run a simple | |
/// ECS task. |
tools/testsys/src/logs.rs
Outdated
#[clap(long = "state", conflicts_with = "test")] | ||
resource_state: Option<ResourceState>, | ||
|
||
/// Include logs from dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused, right? Delete.
tools/testsys/src/delete.rs
Outdated
DeleteEvent::Starting(crd) => debug!("Starting delete for {}", crd.name()), | ||
DeleteEvent::Deleted(crd) => info!("Delete finished for {}", crd.name()), | ||
DeleteEvent::Failed(crd) => warn!("Delete failed for {}", crd.name()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think these should should be regular print statements instead of logging statements. i.e. I think these are part of the normal CLI user interface.
2426ddc
to
bf75a29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round done! I have to come back and review a few more things, but great job!
TESTING.md
Outdated
|
||
## Unit Tests | ||
|
||
Unit tests are easy, you can run them from the root of the repo with `cargo make unit-tests`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The phrase Units tests are easy
made me think that the doc suggests that unit tests are easy to write, not to run, should the line be more specific? Like "It is easy to execute unit tests, you can run them..."
TESTING.md
Outdated
## Unit Tests | ||
|
||
Unit tests are easy, you can run them from the root of the repo with `cargo make unit-tests`. | ||
Note that some code in Bottlerocket is conditionally compiled based on variant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe mention the side effect of the conditional compilation? "thus some tests won't be executed", or tell the users that some of their test might not run if the configuration isn't set properly?
#[clap( | ||
long = "controller-uri", | ||
env = "TESTSYS_CONTROLLER_IMAGE", | ||
default_value = "public.ecr.aws/bottlerocket-test-system/controller:v0.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this will bite us in the future, we will have to remember to update this value whenever we release a new version of the images.
tools/testsys/src/run.rs
Outdated
let file = File::open(&self.ami_input).context("Unable to open amis.json")?; | ||
let ami_input: HashMap<String, Image> = serde_json::from_reader(file) | ||
.context(format!("Unable to deserialize '{}'", self.ami_input))?; | ||
ensure!(!ami_input.is_empty(), "amis.json is empty"); | ||
let bottlerocket_ami = &ami_input | ||
.get(®ion) | ||
.context(format!("ami not found for region '{}'", region))? | ||
.id; | ||
debug!("Using ami '{}'", bottlerocket_ami); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this could be a standalone function, that takes ami_input
, and returns the AMI id.
tools/testsys/src/aws_resources.rs
Outdated
let cluster_name = self.cluster_name(cluster_suffix); | ||
let mut ec2_config = Ec2Config { | ||
node_ami: self.ami.clone(), | ||
// TODO - configurable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: is there a task for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This just creates an ec2 instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I wasn't clear, I meant for the TODO, is there a task for the TODO?
tools/testsys/src/run.rs
Outdated
// This is used to deserialize amis.json | ||
#[serde(rename = "name")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't serde
ignore unknown fields like this by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👢
Issue number:
Closes #2147
Closes #2148
Description of changes:
This pr adds a simplified cli (
testsys
) that utilizes thebottlerocket-test-system
to run sonobuoy tests onaws-k8s
variants. The tests can be run usingcargo make test
.The testsys cli comes with the ability to:
aws-k8s
sonobuoy tests with cluster and instance creation.Cargo make targets:
test-tools
- Install the testsys clisetup-test
- Install testsys to the testsys cluster (requiresKUBECONFIG
to be set)test
- Run a testsys test on aws-k8s variantsclean-test
- Delete all testsys tests and resources from the testsys clusteruninstall-test
- Uninstall testsys components from the testsys clusterwatch-test
- Watch the status of all tests and resourcescargo make watch-test --arch x86_64
to target a specific arch/variant for testslog-test
- Stream the logs from a testsys test.cargo make test-logs x86-64-aws-k8s-121-test
--follow
can be used to continue the log stream as they become availabletestsys
- This is a wrapper for all testsys callsDeveloper workflow
Using
x86_64
andaws-k8s-1.21
to simplify things.Checkout bottlerocket and make changes
Build bottlerocket:
cargo make
Publish amis:
cargo make ami
Install testsys to the testsys cluster:
cargo make setup-test
Start the tests:
cargo make test
<\details>
Check the status of the test:
cargo make watch-test
<\details>
Check the logs from a test:
cargo make log-test x86-64-aws-k8s-121-test --follow
Clean up tests and resources:
cargo make clean-test
<\details>
Uninstall testsys from the cluster:
cargo make uninstall-test
<\details>
cargo make testsys --help
Previous description
``` testsys A program for running and controlling Bottlerocket tests in a Kubernetes cluster using https://github.com/bottlerocket-os/bottlerocket-test-systemUSAGE:
testsys [OPTIONS]
OPTIONS:
-h, --help Print help information
--kubeconfig Path to the kubeconfig file for the testsys cluster. Can also
be passed with the KUBECONFIG environment variable
SUBCOMMANDS:
add Add a testsys object to the testsys cluster
delete Delete all tests and resources from a testsys cluster
help Print this message or the help of the given subcommand(s)
install The install subcommand is responsible for putting all of the necessary
components for testsys in a k8s cluster
logs Stream the logs of an object from a testsys cluster
restart-test Restart a test. This will delete the test object from the testsys cluster
and replace it with a new, identical test object with a clean state
run Run a set of tests on a given arch and variant
status Check the status of a TestSys object