-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 Driver.Prestart method #2054
Conversation
The Driver.Prestart method currently does very little but lays the foundation for where lifecycle plugins can interleave execution _after_ task environment setup but _before_ the task starts. Currently Prestart does two things: * Any driver specific task environment building * Download Docker images This change also attaches a TaskEvent emitter to Drivers, so they can emit events during task initialization.
@@ -92,6 +92,13 @@ func (d *ExecDriver) Periodic() (bool, time.Duration) { | |||
return true, 15 * time.Second | |||
} | |||
|
|||
func (d *ExecDriver) Prestart(execctx *ExecContext, task *structs.Task) error { | |||
// Set the host environment variables. | |||
filter := strings.Split(d.config.ReadDefault("env.blacklist", config.DefaultEnvBlacklist), ",") |
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.
This should be done in all driver Prestarts. It was missed in some of the newer drivers. We should create a common Prestart method that all drivers can call
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.
Should I just move it into TaskRunner instead?
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 (indirectly) added it to TaskRunner by putting it in the GetTaskEnv factory: aaa70ab
// We don't need to start the container if the container is already running | ||
// since we don't create containers which are already present on the host | ||
// and are running | ||
if !container.State.Running { | ||
// Start the container | ||
err := d.startContainer(container) | ||
if err != nil { | ||
if err := client.StartContainer(container.ID, container.HostConfig); err != nil { |
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 did you change from d.startContainer
?
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.
Yikes. No clue. Thanks for catching this. Fixed.
VaultError string | ||
TaskSignalReason string | ||
TaskSignal string | ||
InitializationMessage string |
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.
Change to DriverMsg
as the emitEvent
function can be used generically by the driver?
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 thoughts on what I should change the TaskInitializing
const to? All of the other consts are verbs, but I don't think it'd be confusing to just have a "Driver" message type.
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.
"Driver" is fine with me
@@ -339,46 +343,57 @@ func (d *DockerDriver) Abilities() DriverAbilities { | |||
} | |||
} | |||
|
|||
func (d *DockerDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, error) { | |||
func (d *DockerDriver) Prestart(ctx *ExecContext, task *structs.Task) error { |
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.
Can you emit a message for when it downloads an image?
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.
Can't believe I missed that in this PR! Added (inside the createImage method so it's not emitted if the image already exists).
@@ -91,6 +91,10 @@ const ( | |||
|
|||
type DockerDriver struct { | |||
DriverContext | |||
|
|||
imageID string | |||
waitClient *docker.Client |
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.
You don't have to pass this as it is a shared variable that is initialized once per client
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.
Tangent: Hm, should I be using the waitClient for image downloading?
taskDir, ok := ctx.AllocDir.TaskDirs[d.DriverContext.taskName] | ||
if !ok { | ||
return nil, fmt.Errorf("Could not find task directory for task: %v", d.DriverContext.taskName) | ||
return fmt.Errorf("Could not find task directory for task: %v", d.DriverContext.taskName) |
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.
taskDir is a variable in the task_runner now. Pass it through the context?
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.
That's in the next PR
@@ -168,6 +168,10 @@ func (d *LxcDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, e | |||
return true, nil | |||
} | |||
|
|||
func (d *LxcDriver) Prestart(ctx *ExecContext, task *structs.Task) error { | |||
return nil |
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.
Shouldn't we be setting the env vars here since the paths will be rooted in the container?
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.
Not sure I follow. TaskEnv is built in TaskRunner.Run before the driver is created and passed into the NewDriver factory func via DriverContext, so it's already built and available at this point.
@@ -950,6 +950,7 @@ func (d *DockerDriver) pullImage(driverConfig *DockerDriverConfig, client *docke | |||
} | |||
} | |||
|
|||
d.emitEvent("Downloading image") |
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.
d.emitEventfmt.Sprintf(`Downloading image "%s:%s"`, repo, tag))
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.
Good call; fixed.
@@ -163,16 +163,19 @@ func (d *JavaDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, | |||
return true, nil | |||
} | |||
|
|||
func (d *JavaDriver) Prestart(ctx *ExecContext, task *structs.Task) error { | |||
// Set the host environment variables. | |||
filter := strings.Split(d.config.ReadDefault("env.blacklist", config.DefaultEnvBlacklist), ",") |
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.
delete
@@ -107,6 +107,13 @@ func (d *RawExecDriver) Fingerprint(cfg *config.Config, node *structs.Node) (boo | |||
return false, nil | |||
} | |||
|
|||
func (d *RawExecDriver) Prestart(ctx *ExecContext, task *structs.Task) error { | |||
// Set the host environment variables. | |||
filter := strings.Split(d.config.ReadDefault("env.blacklist", config.DefaultEnvBlacklist), ",") |
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.
delete
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
The Driver.Prestart method currently does very little but lays the foundation for where lifecycle plugins can interleave execution after task environment setup but before the task starts.
Currently Prestart does two things:
This change also attaches a TaskEvent emitter to Drivers, so they can emit events during task initialization.