-
Notifications
You must be signed in to change notification settings - Fork 22
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
env_docker: make --cpus customizable #68
env_docker: make --cpus customizable #68
Conversation
Facilitate debugging flaky tests by providing the ability to customize --cpus parameter through environment variables. Signed-off-by: Giedrius Statkevičius <[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.
Generally LGTM! I think this would be great for debugging. 💪🏻
Just one small question.
env_docker.go
Outdated
func (e *DockerEnvironment) buildDockerRunArgs(name string, ports map[string]int, opts StartOptions) []string { | ||
args := []string{"--rm", "--net=" + e.networkName, "--name=" + dockerNetworkContainerHost(e.networkName, name), "--hostname=" + name} | ||
|
||
// Mount the docker env working directory into the container. It's shared across all containers to allow easier scenarios. | ||
args = append(args, "-v", fmt.Sprintf("%s:%s:z", e.dir, e.dir)) | ||
|
||
dockerCPUs := os.Getenv(dockerCPUEnvName) |
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.
Any reason why this needs to be an env var instead of an EnvironmentOption
? 🙂
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.
Because it wouldn't require any code changes to run a test with limited CPU time and I could imagine that there are some platforms where such an option wouldn't be available. I can convert it to an EnvironmentOption
, not a problem. I don't know what's the correct approach here 😄
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.
Yeah, I thought it was something along those lines. I think it's fine as we have such a pattern with E2E_TEMP_DIR
env var as well.
How about having both EnvironmentOption
, and env var, and EnvironmentOption
can override it? That way you can make certain tests permanently limited. And maybe we can name env var as E2E_DOCKER_CPUS
? 🙂
Not a blocker, we can iterate as well.
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.
Thanks for the review! I have changed the environment variable's name and added the ability to do the same via environment options. @saswatamcode PTAL when you have time 🙇
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Giedrius Statkevičius <[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!
Facilitate debugging flaky tests by providing the ability to customize --cpus parameter through environment variables.