-
Notifications
You must be signed in to change notification settings - Fork 3
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
OpenSearch Domain capacity now configurable #58
Conversation
Signed-off-by: Chris Helma <[email protected]>
README.md
Outdated
|
||
``` | ||
./manage_arkime.py create-cluster --name MyCluster --expected-traffic 10 | ||
./manage_arkime.py create-cluster --name MyCluster --expected-traffic 1 --spi-days 30 --replicas 2 |
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'm afraid of copy paste, lets make this replicas 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.
Will do
manage_arkime.py
Outdated
@@ -58,15 +58,27 @@ def destroy_demo_traffic(ctx): | |||
@click.option( | |||
"--expected-traffic", | |||
help=("The amount of traffic, in gigabits-per-second, you expect your Arkime Cluster to receive." |
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.
to be more correct should be something like "The average amount of traffic, ..."
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.
Will change
def cmd_create_cluster(profile: str, region: str, name: str, expected_traffic: float): | ||
class MustProvideAllParams(Exception): | ||
def __init__(self): | ||
super().__init__("If you specify one of the optional capacity parameters, you must specify all of them.") |
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 because create vs update which we will have later?
If keeping this, 3 changes
- remove the stack trace, we shouldn't show stack traces when its a command line arg issue, it confuses people
- the error message should include ALL the ones required OR the ones missing. (Guessing ALL is easier)
- the "default" print out of help is confusing, since there aren't defaults i guess, or only defaults when none are set?
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.
It has to do with how we're storing the application state in SSM for the cluster and its capacity. Definitely possible for this constraint to be loosened but it's just more code that needs to be written.
The short version is that I want our capacity plan to be stable between deployments even if the underlying code for the capacity plan changes until the user specifically indicates they want to update the capacity (users should not be "surprised" by an infrastructure change). This means we need to store the calculated capacity plan rather than just the inputs to the capacity plan (expected traffic, SPI days, replicas). In this scenario, we don't actually need to store those inputs as long as the user provide all three of them when they want a change. This is the approach I took for the first pass.
If we store the inputs in SSM as well, it will enable us to accept a single one of the parameters and generate a capacity plan using the existing values of the other ones, but it makes the code more complex.
Let's talk it through.
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.
@awick and I talked it through and agreed the path forward is to store the inputs as well and accept the additional complexity. Will update.
Signed-off-by: Chris Helma <[email protected]>
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.
LGTM - Still deleting and redeploying everything.
At some point we should put delete protection on OS and S3
Good idea; made a new issue for that: #59 |
Description
Issues
Testing
License
I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.