hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jason Lowe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-7654) Support ENTRY_POINT for docker container
Date Fri, 11 May 2018 16:27:00 GMT

    [ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message