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

Refactor service bus stress tests to use stress test package format #16689

Merged
merged 12 commits into from
Aug 13, 2021

Conversation

benbp
Copy link
Member

@benbp benbp commented Jul 31, 2021

More details on the stress test package layout: https://github.com/Azure/azure-sdk-tools/blob/main/tools/stress-cluster/chaos/README.md#creating-a-stress-test

This PR or a follow-on could use some replacement docs/links that outline how to build/test, since I deleted the current ones which were mostly specific to the prior script/container infrastructure.

I don't intend to merge this yet, but I wanted to put it up as an example of the stress test package format written for a real stress test, instead of just the examples in the tools repo.

The stress tests can be built and deployed via either the pipeline (to the prod cluster), or to the test cluster with eng/common/scripts/stress-testing/deploy-stress-tests.ps1 -Repository 'javascript/service-bus' -PushImages -Login

Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

We should bring back the sample.env file - even if the resource creation is automated it's still good to see what the automation creates so you can easily fill it in with your pre-existing resources.

This is looking great though!

@@ -0,0 +1,10 @@
scenarios:
- scenarioBatchReceive
Copy link
Member

Choose a reason for hiding this comment

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

A comment in this file would useful, just to give some context on where this file gets consumed, etc..

Copy link
Contributor

Choose a reason for hiding this comment

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

This says optional list of scenarios, but I'm not sure I understand why it's optional for service bus. This just ensures that helm will create 1 instance of the job for each scenario, right? (Is it just optional because someone using this as an example may not need it?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, you don't need to supply a list of scenarios, you can just define the job and we don't to the foreach loop. The yaml comment is more intended for what I predict is the next stress test author copy/pasting from this one.

@@ -0,0 +1 @@
node_modules
Copy link
Member

Choose a reason for hiding this comment

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

Not familiar with this - does .helmignore complement .gitignore?

If not, then we probably also want to avoid accidentally packaging up .env files, for instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added it to be safe. Normally helmignore is just for helm package, which we don't do in this context. However, running helm upgrade|install|template will do a recursive walk, which creates annoying messages when there are symlinks in the node_modules directory:

walk.go:74: found symbolic link in path: /home/ben/sdk/azure-sdk-for-js/sdk/servicebus/service-bus/test/stress/node_modules/.bin/semver resolves to /home/
ben/sdk/azure-sdk-for-js/sdk/servicebus/service-bus/test/stress/node_modules/semver/bin/semver

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the helm ignore to just ignore app/ since I moved things in there related to your other comment (given the manifests only reference the docker file, which contains the files).

@benbp
Copy link
Member Author

benbp commented Aug 11, 2021

We should bring back the sample.env file - even if the resource creation is automated it's still good to see what the automation creates so you can easily fill it in with your pre-existing resources.

I created one with only the keys/values used by the tests so it's easier to fill in for a local context. There's also an example of the container injected env file here.

@benbp
Copy link
Member Author

benbp commented Aug 11, 2021

/check-enforcer reset

@benbp
Copy link
Member Author

benbp commented Aug 11, 2021

/check-enforcer evaluate

@benbp benbp force-pushed the benbp/eh-stress-test branch from 24d490c to 36c9ec0 Compare August 11, 2021 17:46
@benbp benbp closed this Aug 11, 2021
@benbp benbp reopened this Aug 11, 2021
Copy link
Contributor

@chradek chradek left a comment

Choose a reason for hiding this comment

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

I think this looks great, and should be merged. I just had one question that's really just for my own clarification about the values.yaml file that I posted there.

@benbp
Copy link
Member Author

benbp commented Aug 13, 2021

/check-enforcer override

@benbp benbp merged commit fef5493 into Azure:main Aug 13, 2021
@benbp benbp deleted the benbp/eh-stress-test branch August 13, 2021 16:30
@benbp benbp self-assigned this Aug 13, 2021
@benbp benbp added the Central-EngSys This issue is owned by the Engineering System team. label Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team. Service Bus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants