[
https://issues.apache.org/jira/browse/YARN-7654?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16472209#comment-16472209
]
Jason Lowe commented on YARN-7654:
----------------------------------
Thanks for updating the patch!
This previous comment appears to have been missed:
{quote}In DockerProviderService#buildContainerLaunchContext it's calling
processArtifact then super.buildContainerLaunchContext, but the parent's
buildContainerLaunchContext calls processArtifact as well. Is the double-call
intentional?
{quote}
Related to the above, it looks like the
Boolean.valueOf is more inefficient than Boolean.parseBoolean if what we want
is a {{boolean}} instead of a {{Boolean}}. valueOf is implemented in terms of
parseBoolean. parseBoolean handles null arguments, so the code can just be a
call to parseBoolean directly.
dockerOverride is still named the opposite of what it means based on how it is
initialized. It will be true when
YARN_CONTAINER_RUNTIME_DOCKER_RUN_OVERRIDE_DISABLE=true which seems backwards.
Example of confusing code:
{code:java}
if (dockerOverride) {
runCommand.setOverrideDisabled(true);
{code}
If we're not going to clean up the DockerRunCommand instance checks then they
need to be not so prevalent. writeEnvFile should take a DockerRunCommand and we
should only be downcasting once, right after we verified the downcast should
succeed.
Nit: printWriter and envWriter are being closed unnecessarily since
try-with-resources will also close it.
DockerRunCommand#addEnv should just be: {{userEnv.putAll(environment)}}
docker_override local in create_container_log_dirs is named backwards. It's
true when we're using the entry point and false when we're overriding the entry
point.
get_max_retries should free max_retries
typo: "spwaning" s/b "spawning"
The documentation states that a replacement container will be launched, but I
think it would be more accurate to state the container launch will be marked as
failed if the retries max out. IIUC a relaunch will not occur unless the
application logic decides to retry that failure.
{quote}The number of steps to go through the preprocessing before writing to
.cmd file complicates how to refactor the code base without breaking things.
{quote}
I'm not seeing that at all. There's only two places in the code that setup
commands in the AbstractLauncher: AbstractProviderService which is calling
addCommands and DockerProviderService which is calling the new setCommands
method. The only reason we need a setCommands method on AbstractLauncher is
because we want to leverage some code in
AbstractProviderService#buildContainerLaunchContext but it does more than we
want. All I'm proposing is to break up that method into the parts we do want to
reuse and the parts we want to override. For example, we can refactor the part
of AbstractProviderService#buildContainerLaunchContext that sets up the launch
command to a separate, overridable method like this:
{code:java}
protected void substituteLaunchCommand(AbstractLauncher launcher,
ComponentInstance instance, Container container,
ContainerLaunchService.ComponentLaunchContext compLaunchContext,
Map<String, String> tokensForSubstitution) {
// substitute launch command
String launchCommand = compLaunchContext.getLaunchCommand();
// docker container may have empty commands
if (!StringUtils.isEmpty(launchCommand)) {
launchCommand = ProviderUtils
.substituteStrWithTokens(launchCommand, tokensForSubstitution);
CommandLineBuilder operation = new CommandLineBuilder();
operation.add(launchCommand);
operation.addOutAndErrFiles(OUT_FILE, ERR_FILE);
launcher.addCommand(operation.build());
}
}
{code}
then DockerProviderService can override substituteLaunchCommand instead of
buildContainerLaunchContext like this:
{code:java}
@Override
protected void substituteLaunchCommand(AbstractLauncher launcher,
ComponentInstance instance, Container container,
ContainerLaunchService.ComponentLaunchContext compLaunchContext,
Map<String, String> tokensForSubstitution) {
Component component = instance.getComponent().getComponentSpec();
Map<String, String> globalTokens =
instance.getComponent().getScheduler().globalTokens;
tokensForSubstitution = ProviderUtils
.initCompTokensForSubstitute(instance, container, compLaunchContext);
tokensForSubstitution.putAll(globalTokens);
// substitute launch command
String launchCommand = component.getLaunchCommand();
// docker container may have empty commands
if (!StringUtils.isEmpty(launchCommand)) {
launchCommand = ProviderUtils
.substituteStrWithTokens(launchCommand, tokensForSubstitution);
CommandLineBuilder operation = new CommandLineBuilder();
operation.add(launchCommand);
boolean overrideDisable = Boolean.parseBoolean(component
.getConfiguration().getEnv(Environment
.YARN_CONTAINER_RUNTIME_DOCKER_RUN_OVERRIDE_DISABLE.name()));
if (!overrideDisable) {
operation.addOutAndErrFiles(OUT_FILE, ERR_FILE);
}
launcher.addCommand(operation.build());
}
}
{code}
Then the code does less wasted work and doesn't need a setCommands method on
the AbstractLauncher.
Note I'm not sure if we really need to rebuild tokensForSubtitution in
DockerProviderService, I'm just preserving what the patch was doing. AFAICT the
only difference between what the patch had DockerProviderService build for
tokens and what AbstractProviderService builds is the latter is doing a pass
adding ${env} forms of every env var to the map. If DockerProviderService is
supposed to be doing that as well then it can just use the tokenProviderService
arg directly rather than building it from scratch.
> Support ENTRY_POINT for docker container
> ----------------------------------------
>
> Key: YARN-7654
> URL: https://issues.apache.org/jira/browse/YARN-7654
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: yarn
> Affects Versions: 3.1.0
> Reporter: Eric Yang
> Assignee: Eric Yang
> Priority: Blocker
> Labels: Docker
> Attachments: YARN-7654.001.patch, YARN-7654.002.patch,
> YARN-7654.003.patch, YARN-7654.004.patch, YARN-7654.005.patch,
> YARN-7654.006.patch, YARN-7654.007.patch, YARN-7654.008.patch,
> YARN-7654.009.patch, YARN-7654.010.patch, YARN-7654.011.patch,
> YARN-7654.012.patch, YARN-7654.013.patch, YARN-7654.014.patch,
> YARN-7654.015.patch, YARN-7654.016.patch, YARN-7654.017.patch,
> YARN-7654.018.patch, YARN-7654.019.patch, YARN-7654.020.patch,
> YARN-7654.021.patch, YARN-7654.022.patch
>
>
> Docker image may have ENTRY_POINT predefined, but this is not supported in
> the current implementation. It would be nice if we can detect existence of
> {{launch_command}} and base on this variable launch docker container in
> different ways:
> h3. Launch command exists
> {code}
> docker run [image]:[version]
> docker exec [container_id] [launch_command]
> {code}
> h3. Use ENTRY_POINT
> {code}
> docker run [image]:[version]
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
