Skip to content
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

Dogfood e2etest; Client/Server Application #617

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

Azoam
Copy link
Contributor

@Azoam Azoam commented Nov 10, 2022

What does this PR do?

  • Adds new functionality
  • Alters existing functionality
  • Fixes a bug
  • Improves documentation or testing

Please briefly describe your changes as well as the motivation behind them:

First iteration of a larger project to create regression-like testing for the chaos controller. The PR contains an updated dogfood which has been tweaked slightly to confirm that the metrics we need to test CPU, Network, and Disk are present in Datadog.

The tester folder is present in this PR to show the current path that will be taken with this project and is by no means done or being used in any capacity.

Code Quality Checklist

  • The documentation is up to date.
  • My code is sufficiently commented and passes continuous integration checks.
  • I have signed my commit (see Contributing Docs).

Testing

  • I leveraged continuous integration testing
    • by depending on existing unit tests or end-to-end tests.
    • by adding new unit tests or end-to-end tests.
  • I manually tested the following steps:
    • colima local testing with datadog agent deployed locally sending metrics to datadog
    • locally.
    • as a canary deployment to a cluster.

@Azoam Azoam requested a review from a team as a code owner November 10, 2022 17:59
aymericDD
aymericDD previously approved these changes Nov 14, 2022
Copy link
Contributor

@aymericDD aymericDD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

dogfood/README.md Outdated Show resolved Hide resolved

For the purposes of testing disruptions/workflows, you should make sure that the datadog agent is properly installed
on the cluster that the client and server are running on. 3 of the major disruptive resources properly send metrics
to Datadog (CPU, Network, Disk). The client contains computation related to these disruptions and can be tested using
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 of the major disruptive resources properly send metrics to Datadog (CPU, Network, Disk).

I dont understand this sentence

Copy link
Contributor Author

@Azoam Azoam Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing, let me know if its still confusing after the next commit
.

func printAndLog(logLine string) {
fmt.Println(logLine)

go func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, why do we want to write all this random data to /mnt/data/logging whenever we're trying to print to console? They dont seem to need to be so coupled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of printAndLog is to give the client Write IO. This is the function that will help us test Disk write disruptions properly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know why we want to have the logging done, but why put it in printAndLog? It seems this could be in gothreads that run separately, I don't see why we kick it off once per print

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this spawned from not knowing what data to write to disk at first. I think you can pull random data from somewhere, but I opted to just use the logs that were already in the code as data to write.

labels: {}
type: Opaque
data:
api-key: PUT_YOUR_BASE64_ENCODED_API_KEY_HERE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the two user input lines, is this all copied from elsewhere? Any chance we could directly import that file, and use a values.yaml for api-key and token?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it also seems like we'd want to put a private api key in a gitignored file for safety?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is mostly for others who need a datadog agent deploy. Its not something we will couple with our deploy considering the datadog agent is already deployed on our clusters.

Network: &v1beta1.NetworkDisruptionSpec{
Hosts: []v1beta1.NetworkDisruptionHostSpec{
{
Host: "chaos-dogfood-server.chaos-demo.svc.cluster.local",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is re-used, could we extract it to a const?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could also extract more of the disruption spec parameters into their own variables / const, as it seems we're re-using most of it (object meta namespace, spec counts, unsafemode, duration, whole hosts). What do you think?

// Globals

var SELECTOR = []string{"app", "chaos-dogfood-client"}
var CONTAINER = "client-deploy"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be consts, I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think you can make string arrays const? but I can make the others.

@Devatoria
Copy link
Contributor

Note: we're keeping the PR open for now, we'll re-adapt it based on recent changes made to e2e tests plan once possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants