-
Notifications
You must be signed in to change notification settings - Fork 95
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 arguments to quilkin run
#574
Changes from 18 commits
760c4ea
9535633
d0a9898
3902891
f8713f2
f50c98c
2fdb198
463f5ac
caa1335
63f245a
69f4074
f464f51
976a4c7
6df9ffb
58ce0a4
4e5af5f
f783e8b
f45d971
c386053
c46b3d7
4731b83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,16 +46,6 @@ steps: | |
args: | ||
- build | ||
id: build | ||
# Run the built images for 5 seconds to make sure that the entrypoint and default config works out of the box | ||
- name: gcr.io/cloud-builders/docker | ||
dir: ./build | ||
entrypoint: bash | ||
args: | ||
- '-c' | ||
- 'timeout --signal=INT --preserve-status 5s docker run --rm ${_REPOSITORY}quilkin:$(make version)' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than deleting, could make it (something like)
To make it run in Although one could argue the integration tests also test this (although this would provide a nicer error message if something basic was broken). I could go either way on this one. |
||
id: test-quilkin-image | ||
waitFor: | ||
- build | ||
- name: us-docker.pkg.dev/$PROJECT_ID/ci/make-docker | ||
dir: ./build | ||
args: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,38 +18,50 @@ mod generate_config_schema; | |
mod manage; | ||
mod run; | ||
|
||
use std::path::PathBuf; | ||
use std::{ | ||
path::{Path, PathBuf}, | ||
sync::Arc, | ||
}; | ||
|
||
use tokio::{signal, sync::watch}; | ||
|
||
use crate::Config; | ||
|
||
pub use self::{ | ||
generate_config_schema::GenerateConfigSchema, | ||
manage::{Manage, Providers}, | ||
run::Run, | ||
}; | ||
|
||
const VERSION: &str = env!("CARGO_PKG_VERSION"); | ||
const ETC_CONFIG_PATH: &str = "/etc/quilkin/quilkin.yaml"; | ||
|
||
/// The Command-Line Interface for Quilkin. | ||
#[derive(clap::Parser)] | ||
#[non_exhaustive] | ||
pub struct Cli { | ||
#[clap( | ||
short, | ||
long, | ||
env = "QUILKIN_CONFIG", | ||
default_value = "quilkin.yaml", | ||
help = "The YAML configuration file." | ||
)] | ||
config: PathBuf, | ||
#[clap( | ||
short, | ||
long, | ||
env, | ||
help = "Whether Quilkin will report any results to stdout/stderr." | ||
)] | ||
quiet: bool, | ||
/// The path to the configuration file for the Quilkin instance. | ||
#[clap(short, long, env = "QUILKIN_CONFIG", default_value = "quilkin.yaml")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was actually thinking of coming back around and fleshing out the help for the rest of the commands for a nice command line reference. Curious if you didn't want a command line reference here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait - when I run the command, I see help everywhere, Wait.... does clap take the help from the comment above it? If so, that's a super neat trick. |
||
pub config: PathBuf, | ||
/// The port to bind for the admin server | ||
#[clap(long, env = "QUILKIN_ADMIN_ADDRESS")] | ||
pub admin_address: Option<std::net::SocketAddr>, | ||
/// Whether to spawn the admin server or not. | ||
#[clap(long, env = "QUILKIN_NO_ADMIN")] | ||
pub no_admin: bool, | ||
/// Whether Quilkin will report any results to stdout/stderr. | ||
#[clap(short, long, env)] | ||
pub quiet: bool, | ||
#[clap(subcommand)] | ||
command: Commands, | ||
pub command: Commands, | ||
} | ||
|
||
/// The various Quilkin commands. | ||
#[derive(clap::Subcommand)] | ||
enum Commands { | ||
Run(run::Run), | ||
GenerateConfigSchema(generate_config_schema::GenerateConfigSchema), | ||
Manage(manage::Manage), | ||
pub enum Commands { | ||
Run(Run), | ||
GenerateConfigSchema(GenerateConfigSchema), | ||
Manage(Manage), | ||
} | ||
|
||
impl Cli { | ||
|
@@ -78,25 +90,68 @@ impl Cli { | |
"Starting Quilkin" | ||
); | ||
|
||
let config = Arc::new(Self::read_config(self.config)?); | ||
let _admin_task = if self.no_admin { | ||
config.admin.remove(); | ||
None | ||
} else { | ||
if let Some(address) = self.admin_address { | ||
config | ||
.admin | ||
.store(Arc::new(crate::config::Admin { address })); | ||
} | ||
Some(tokio::spawn(crate::admin::server(config.clone()))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty sure this is the offending line causing the double creation of the admin server, since both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing some debugging (code), it looks like the markmandel@09de12a#diff-b2812f19576dd53d0c35b107a322f58a00fce6977f4b5976e1961853982af3ccR93 let config = Arc::new(Self::read_config(self.config)?); Has a You can see it in my debug logs:
I'll keep digging into this tomorrow unless you get to it first. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've found the issue. I'll send a PR to your PR today. |
||
}; | ||
|
||
let (shutdown_tx, shutdown_rx) = watch::channel::<()>(()); | ||
|
||
#[cfg(target_os = "linux")] | ||
let mut sig_term_fut = signal::unix::signal(signal::unix::SignalKind::terminate())?; | ||
|
||
tokio::spawn(async move { | ||
#[cfg(target_os = "linux")] | ||
let sig_term = sig_term_fut.recv(); | ||
#[cfg(not(target_os = "linux"))] | ||
let sig_term = std::future::pending(); | ||
|
||
let signal = tokio::select! { | ||
_ = signal::ctrl_c() => "SIGINT", | ||
_ = sig_term => "SIGTERM", | ||
}; | ||
|
||
tracing::info!(%signal, "shutting down from signal"); | ||
// Don't unwrap in order to ensure that we execute | ||
// any subsequent shutdown tasks. | ||
shutdown_tx.send(()).ok(); | ||
}); | ||
|
||
match &self.command { | ||
Commands::Run(runner) => runner.run(&self).await, | ||
Commands::Manage(manager) => manager.manage(&self).await, | ||
Commands::Run(runner) => runner.run(config, shutdown_rx.clone()).await, | ||
Commands::Manage(manager) => manager.manage(config).await, | ||
Commands::GenerateConfigSchema(generator) => generator.generate_config_schema(), | ||
} | ||
} | ||
|
||
/// Searches for the configuration file, and panics if not found. | ||
fn read_config(&self) -> Config { | ||
std::fs::File::open(&self.config) | ||
.or_else(|error| { | ||
if cfg!(unix) { | ||
std::fs::File::open("/etc/quilkin/quilkin.yaml") | ||
} else { | ||
Err(error) | ||
fn read_config<A: AsRef<Path>>(path: A) -> Result<Config, eyre::Error> { | ||
let path = path.as_ref(); | ||
let from_reader = |file| Config::from_reader(file).map_err(From::from); | ||
|
||
match std::fs::File::open(path) { | ||
Ok(file) => (from_reader)(file), | ||
Err(error) if error.kind() == std::io::ErrorKind::NotFound => { | ||
tracing::debug!(path=%path.display(), "provided path not found"); | ||
match cfg!(unix).then(|| std::fs::File::open(ETC_CONFIG_PATH)) { | ||
Some(Ok(file)) => (from_reader)(file), | ||
Some(Err(error)) if error.kind() == std::io::ErrorKind::NotFound => { | ||
tracing::debug!(path=%path.display(), "/etc path not found"); | ||
Ok(Config::default()) | ||
} | ||
Some(Err(error)) => Err(error.into()), | ||
None => Ok(Config::default()), | ||
} | ||
}) | ||
.map_err(eyre::Error::from) | ||
.and_then(|file| Config::from_reader(file).map_err(From::from)) | ||
.unwrap() | ||
} | ||
Err(error) => Err(error.into()), | ||
} | ||
} | ||
} |
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 replicated the issue as well with running the container from this PR. I'd recommend leaving in this test, as it also will replicate the breaking issue with the Agones integration tests, but output the logs within cloud build, and make it much easier to debug if there is a problem with the base image.
We may want to consider running the container in a few different common configuration options might also be useful in a few tests like the above, just to prove it all still runs even without any traffic flowing through.