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

Ability to set a different target directory when generating configs #7

Closed
wants to merge 2 commits into from

Conversation

karencfv
Copy link

@karencfv karencfv commented Aug 8, 2024

Related: #6 oxidecomputer/omicron#5999

Overview

New --target-dir flag to allow deployments to live in a directory of their choosing.

New functionality:

$ cargo run gen-config --path . --num-keepers 3 --num-replicas 2 --target-dir testing
   Compiling clickward v0.1.0 (/Users/karcar/src/clickward)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.55s
     Running `target/debug/main gen-config --path . --num-keepers 3 --num-replicas 2 --target-dir testing`
$ tree testing
testing
├── clickhouse-1
│   └── clickhouse-config.xml
├── clickhouse-2
│   └── clickhouse-config.xml
├── clickward-metadata.json
├── keeper-1
│   └── keeper-config.xml
├── keeper-2
│   └── keeper-config.xml
└── keeper-3
    └── keeper-config.xml

6 directories, 6 files
$ cargo run deploy --path . --target-dir testing
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `target/debug/main deploy --path . --target-dir testing`
Deploying keeper: ./testing/keeper-1
Deploying keeper: ./testing/keeper-3
Deploying keeper: ./testing/keeper-2
Deploying clickhouse server: ./testing/clickhouse-1
Deploying clickhouse server: ./testing/clickhouse-2
$ cargo run teardown --path . --target-dir testing
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/main teardown --path . --target-dir testing`
Stopping keeper: ./testing/keeper-1 at pid 43724
Stopping keeper: ./testing/keeper-2 at pid 43762
Stopping keeper: ./testing/keeper-3 at pid 43743
Stopping clickhouse server clickhouse-1: pid - 43781, child pid - 43802
Stopping clickhouse server clickhouse-2: pid - 43800, child pid - 43801

Retained old functionality as well:

$ cargo run gen-config --path . --num-keepers 3 --num-replicas 2
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `target/debug/main gen-config --path . --num-keepers 3 --num-replicas 2`
$ tree deployment
deployment
├── clickhouse-1
│   └── clickhouse-config.xml
├── clickhouse-2
│   └── clickhouse-config.xml
├── clickward-metadata.json
├── keeper-1
│   └── keeper-config.xml
├── keeper-2
│   └── keeper-config.xml
└── keeper-3
    └── keeper-config.xml

6 directories, 6 files
$ cargo run deploy --path . 
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `target/debug/main deploy --path .`
Deploying keeper: ./deployment/keeper-1
Deploying keeper: ./deployment/keeper-3
Deploying keeper: ./deployment/keeper-2
Deploying clickhouse server: ./deployment/clickhouse-1
Deploying clickhouse server: ./deployment/clickhouse-2
$ cargo run teardown --path .
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `target/debug/main teardown --path .`
Stopping keeper: ./deployment/keeper-1 at pid 43988
Stopping keeper: ./deployment/keeper-2 at pid 44026
Stopping keeper: ./deployment/keeper-3 at pid 44007
Stopping clickhouse server clickhouse-1: pid - 44045, child pid - 44065
Stopping clickhouse server clickhouse-2: pid - 44064, child pid - 44066

Why do we need this?

  • In production environments, we'll need the ability to set the name of the directory where the configuration files will live.
  • In testing environments, users may want to have several configurations living side by side to iterate.

Caveats

I've tried to keep as much old functionality there as possible. The flags and code may look neater if I get rid of a default directory name altogether, but it would lose a bit of the simplicity. Happy to change this if it doesn't make sense

@andrewjstone
Copy link
Collaborator

I don't think this is necessary. I left you more detail in chat, but the short of it is that we shouldn't be using the clickward cli at all in production. We should just be using the config generation bits.

It also seems like all this does is work around the deployment directory that gets automatically nested under the path command line parameter. The path parameter is the intended target_dir, so this all feels redundant. I nested the deployment dir that so that I could just be lazy and run clickward out of its current directory during development and then go ahead and just rm deployment to clean it up. That definitely is not strictly needed and I suppose it is a bit unconventional.

While we don't need to use this CLI for production, the automatic subdirectory is a touch weird. I'm not against removing the deployment subdirectory altogether and just dictating the location to put files by setting path such that it's not . I don't want to add essentially the same argument named differently though.

@andrewjstone
Copy link
Collaborator

andrewjstone commented Aug 8, 2024

Related to removing the deployment subdir:

  • If we do generate config in a path directory that is non-empty, we should probably disallow it as it will clutter what is already in there.
  • If the directory doesn't already exist we should probably create it

These are the other reasons for using a deployment dir. All things being equal, I think I'd leave things as is, but if you want to change it, just make sure that you handle these to cases as well. Thanks!

@karencfv
Copy link
Author

karencfv commented Aug 8, 2024

Leaving a note here after a conversation in chat. I'll close this PR in favour of just taking the code generation bits into Omicron :)

@karencfv karencfv closed this Aug 8, 2024
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.

2 participants