YARN-8429. Improve diagnostic message when artifact is not set properly.

Contributed by Gour Saha

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/main/java/org/apache/hadoop/yarn/service/exceptions/RestApiErrorMessages.java

@@ -50,6 +50,10 @@ public interface RestApiErrorMessages {
       "Artifact id (like docker image name) is either empty or not provided";
   String ERROR_ARTIFACT_ID_FOR_COMP_INVALID =
       ERROR_ARTIFACT_ID_INVALID + ERROR_SUFFIX_FOR_COMPONENT;
+  String ERROR_ARTIFACT_PATH_FOR_COMP_INVALID = "For component %s with %s "
+      + "artifact, path does not exist: %s";
+  String ERROR_CONFIGFILE_DEST_FILE_FOR_COMP_NOT_ABSOLUTE = "For component %s "
+      + "with %s artifact, dest_file must be a relative path: %s";
 
   String ERROR_RESOURCE_INVALID = "Resource is not provided";
   String ERROR_RESOURCE_FOR_COMP_INVALID =
@@ -89,7 +93,7 @@ public interface RestApiErrorMessages {
   String ERROR_ABSENT_NUM_OF_INSTANCE =
       "Num of instances should appear either globally or per component";
   String ERROR_ABSENT_LAUNCH_COMMAND =
-      "Launch_command is required when type is not DOCKER";
+      "launch_command is required when type is not DOCKER";
 
   String ERROR_QUICKLINKS_FOR_COMP_INVALID = "Quicklinks specified at"
       + " component level, needs corresponding values set at service level";

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/main/java/org/apache/hadoop/yarn/service/provider/AbstractClientProvider.java

@@ -68,18 +68,18 @@ public static final Set<String> createApplicationTags(String appName,
    * Validate the artifact.
    * @param artifact
    */
-  public abstract void validateArtifact(Artifact artifact, FileSystem
+  public abstract void validateArtifact(Artifact artifact, String compName,
-      fileSystem) throws IOException;
+      FileSystem fileSystem) throws IOException;
 
-  protected abstract void validateConfigFile(ConfigFile configFile, FileSystem
+  protected abstract void validateConfigFile(ConfigFile configFile,
-      fileSystem) throws IOException;
+      String compName, FileSystem fileSystem) throws IOException;
 
   /**
    * Validate the config files.
    * @param configFiles config file list
    * @param fs file system
    */
-  public void validateConfigFiles(List<ConfigFile> configFiles,
+  public void validateConfigFiles(List<ConfigFile> configFiles, String compName,
       FileSystem fs) throws IOException {
     Set<String> destFileSet = new HashSet<>();
 
@@ -128,7 +128,7 @@ public void validateConfigFiles(List<ConfigFile> configFiles,
       }
 
       if (StringUtils.isEmpty(file.getDestFile())) {
-        throw new IllegalArgumentException("Dest_file is empty.");
+        throw new IllegalArgumentException("dest_file is empty.");
       }
 
       if (destFileSet.contains(file.getDestFile())) {
@@ -144,7 +144,7 @@ public void validateConfigFiles(List<ConfigFile> configFiles,
       }
 
       // provider-specific validation
-      validateConfigFile(file, fs);
+      validateConfigFile(file, compName, fs);
     }
   }
 }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/main/java/org/apache/hadoop/yarn/service/provider/defaultImpl/DefaultClientProvider.java

@@ -17,30 +17,36 @@
  */
 package org.apache.hadoop.yarn.service.provider.defaultImpl;
 
-import org.apache.hadoop.fs.FileSystem;
-import org.apache.hadoop.yarn.service.provider.AbstractClientProvider;
-import org.apache.hadoop.yarn.service.api.records.Artifact;
-import org.apache.hadoop.yarn.service.api.records.ConfigFile;
-
 import java.io.IOException;
 import java.nio.file.Paths;
 
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.yarn.service.api.records.Artifact;
+import org.apache.hadoop.yarn.service.api.records.ConfigFile;
+import org.apache.hadoop.yarn.service.exceptions.RestApiErrorMessages;
+import org.apache.hadoop.yarn.service.provider.AbstractClientProvider;
+
+import com.google.common.annotations.VisibleForTesting;
+
 public class DefaultClientProvider extends AbstractClientProvider {
 
   public DefaultClientProvider() {
   }
 
   @Override
-  public void validateArtifact(Artifact artifact, FileSystem fileSystem) {
+  public void validateArtifact(Artifact artifact, String compName,
+      FileSystem fileSystem) {
   }
 
   @Override
-  protected void validateConfigFile(ConfigFile configFile, FileSystem
+  @VisibleForTesting
-      fileSystem) throws IOException {
+  public void validateConfigFile(ConfigFile configFile, String compName,
+      FileSystem fileSystem) throws IOException {
     // validate dest_file is not absolute
     if (Paths.get(configFile.getDestFile()).isAbsolute()) {
-      throw new IllegalArgumentException(
+      throw new IllegalArgumentException(String.format(
-          "Dest_file must not be absolute path: " + configFile.getDestFile());
+          RestApiErrorMessages.ERROR_CONFIGFILE_DEST_FILE_FOR_COMP_NOT_ABSOLUTE,
+          compName, "no", configFile.getDestFile()));
     }
   }
 }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/main/java/org/apache/hadoop/yarn/service/provider/docker/DockerClientProvider.java

@@ -35,19 +35,20 @@ public DockerClientProvider() {
   }
 
   @Override
-  public void validateArtifact(Artifact artifact, FileSystem fileSystem) {
+  public void validateArtifact(Artifact artifact, String compName,
+      FileSystem fileSystem) {
     if (artifact == null) {
-      throw new IllegalArgumentException(
+      throw new IllegalArgumentException(String.format(
-          RestApiErrorMessages.ERROR_ARTIFACT_INVALID);
+          RestApiErrorMessages.ERROR_ARTIFACT_FOR_COMP_INVALID, compName));
     }
     if (StringUtils.isEmpty(artifact.getId())) {
-      throw new IllegalArgumentException(
+      throw new IllegalArgumentException(String.format(
-          RestApiErrorMessages.ERROR_ARTIFACT_ID_INVALID);
+          RestApiErrorMessages.ERROR_ARTIFACT_ID_FOR_COMP_INVALID, compName));
     }
   }
 
   @Override
-  protected void validateConfigFile(ConfigFile configFile, FileSystem
+  protected void validateConfigFile(ConfigFile configFile, String compName,
-      fileSystem) throws IOException {
+      FileSystem fileSystem) throws IOException {
   }
 }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/main/java/org/apache/hadoop/yarn/service/provider/tarball/TarballClientProvider.java

@@ -36,30 +36,33 @@ public TarballClientProvider() {
   }
 
   @Override
-  public void validateArtifact(Artifact artifact, FileSystem fs)
+  public void validateArtifact(Artifact artifact, String compName,
-      throws IOException {
+      FileSystem fs) throws IOException {
     if (artifact == null) {
-      throw new IllegalArgumentException(
+      throw new IllegalArgumentException(String.format(
-          RestApiErrorMessages.ERROR_ARTIFACT_INVALID);
+          RestApiErrorMessages.ERROR_ARTIFACT_FOR_COMP_INVALID, compName));
     }
     if (StringUtils.isEmpty(artifact.getId())) {
-      throw new IllegalArgumentException(
+      throw new IllegalArgumentException(String.format(
-          RestApiErrorMessages.ERROR_ARTIFACT_ID_INVALID);
+          RestApiErrorMessages.ERROR_ARTIFACT_ID_FOR_COMP_INVALID, compName));
     }
     Path p = new Path(artifact.getId());
     if (!fs.exists(p)) {
-      throw new IllegalArgumentException( "Artifact tarball does not exist "
+      throw new IllegalArgumentException(String.format(
-          + artifact.getId());
+          RestApiErrorMessages.ERROR_ARTIFACT_PATH_FOR_COMP_INVALID, compName,
+          Artifact.TypeEnum.TARBALL.name(), artifact.getId()));
     }
   }
 
   @Override
-  protected void validateConfigFile(ConfigFile configFile, FileSystem
+  protected void validateConfigFile(ConfigFile configFile, String compName,
-      fileSystem) throws IOException {
+      FileSystem fileSystem) throws IOException {
     // validate dest_file is not absolute
     if (Paths.get(configFile.getDestFile()).isAbsolute()) {
-      throw new IllegalArgumentException(
+      throw new IllegalArgumentException(String.format(
-          "Dest_file must not be absolute path: " + configFile.getDestFile());
+          RestApiErrorMessages.ERROR_CONFIGFILE_DEST_FILE_FOR_COMP_NOT_ABSOLUTE,
+          compName, Artifact.TypeEnum.TARBALL.name(),
+          configFile.getDestFile()));
     }
   }
 }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/main/java/org/apache/hadoop/yarn/service/utils/ServiceApiUtil.java

@@ -282,7 +282,7 @@ private static void validateComponent(Component comp, FileSystem fs,
 
     AbstractClientProvider compClientProvider = ProviderFactory
         .getClientProvider(comp.getArtifact());
-    compClientProvider.validateArtifact(comp.getArtifact(), fs);
+    compClientProvider.validateArtifact(comp.getArtifact(), comp.getName(), fs);
 
     if (comp.getLaunchCommand() == null && (comp.getArtifact() == null || comp
         .getArtifact().getType() != Artifact.TypeEnum.DOCKER)) {
@@ -299,7 +299,7 @@ private static void validateComponent(Component comp, FileSystem fs,
               + ": " + comp.getNumberOfContainers(), comp.getName()));
     }
     compClientProvider.validateConfigFiles(comp.getConfiguration()
-        .getFiles(), fs);
+        .getFiles(), comp.getName(), fs);
 
     MonitorUtils.getProbe(comp.getReadinessCheck());
   }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/test/java/org/apache/hadoop/yarn/service/TestServiceApiUtil.java

@@ -227,14 +227,16 @@ public void testArtifacts() throws IOException {
     // no artifact id fails with default type
     Artifact artifact = new Artifact();
     app.setArtifact(artifact);
-    Component comp = ServiceTestUtils.createComponent("comp1");
+    String compName = "comp1";
+    Component comp = ServiceTestUtils.createComponent(compName);
 
     app.setComponents(Collections.singletonList(comp));
     try {
       ServiceApiUtil.validateAndResolveService(app, sfs, CONF_DNS_ENABLED);
       Assert.fail(EXCEPTION_PREFIX + "service with no artifact id");
     } catch (IllegalArgumentException e) {
-      assertEquals(ERROR_ARTIFACT_ID_INVALID, e.getMessage());
+      assertEquals(String.format(ERROR_ARTIFACT_ID_FOR_COMP_INVALID, compName),
+          e.getMessage());
     }
 
     // no artifact id fails with SERVICE type
@@ -252,7 +254,8 @@ public void testArtifacts() throws IOException {
       ServiceApiUtil.validateAndResolveService(app, sfs, CONF_DNS_ENABLED);
       Assert.fail(EXCEPTION_PREFIX + "service with no artifact id");
     } catch (IllegalArgumentException e) {
-      assertEquals(ERROR_ARTIFACT_ID_INVALID, e.getMessage());
+      assertEquals(String.format(ERROR_ARTIFACT_ID_FOR_COMP_INVALID, compName),
+          e.getMessage());
     }
 
     // everything valid here

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/test/java/org/apache/hadoop/yarn/service/providers/TestAbstractClientProvider.java

@@ -45,12 +45,12 @@ public class TestAbstractClientProvider {
 
   private static class ClientProvider extends AbstractClientProvider {
     @Override
-    public void validateArtifact(Artifact artifact, FileSystem fileSystem)
+    public void validateArtifact(Artifact artifact, String compName,
-        throws IOException {
+        FileSystem fileSystem) throws IOException {
     }
 
     @Override
-    protected void validateConfigFile(ConfigFile configFile,
+    protected void validateConfigFile(ConfigFile configFile, String compName,
         FileSystem fileSystem) throws IOException {
     }
   }
@@ -62,33 +62,34 @@ public void testConfigFiles() throws IOException {
     FileStatus mockFileStatus = mock(FileStatus.class);
     when(mockFs.exists(anyObject())).thenReturn(true);
 
+    String compName = "sleeper";
     ConfigFile configFile = new ConfigFile();
     List<ConfigFile> configFiles = new ArrayList<>();
     configFiles.add(configFile);
 
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
       Assert.fail(EXCEPTION_PREFIX + "null file type");
     } catch (IllegalArgumentException e) {
     }
 
     configFile.setType(ConfigFile.TypeEnum.TEMPLATE);
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
       Assert.fail(EXCEPTION_PREFIX + "empty src_file for type template");
     } catch (IllegalArgumentException e) {
     }
 
     configFile.setSrcFile("srcfile");
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
       Assert.fail(EXCEPTION_PREFIX + "empty dest file");
     } catch (IllegalArgumentException e) {
     }
 
     configFile.setDestFile("destfile");
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
     } catch (IllegalArgumentException e) {
       Assert.fail(NO_EXCEPTION_PREFIX + e.getMessage());
     }
@@ -99,21 +100,21 @@ public void testConfigFiles() throws IOException {
     configFile.setDestFile("path/destfile2");
     configFiles.add(configFile);
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
       Assert.fail(EXCEPTION_PREFIX + "dest file with multiple path elements");
     } catch (IllegalArgumentException e) {
     }
 
     configFile.setDestFile("/path/destfile2");
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
     } catch (IllegalArgumentException e) {
       Assert.fail(NO_EXCEPTION_PREFIX + e.getMessage());
     }
 
     configFile.setDestFile("destfile");
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
       Assert.fail(EXCEPTION_PREFIX + "duplicate dest file");
     } catch (IllegalArgumentException e) {
     }
@@ -125,14 +126,14 @@ public void testConfigFiles() throws IOException {
     configFile.setDestFile("path/destfile3");
     configFiles.add(configFile);
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
       Assert.fail(EXCEPTION_PREFIX + "dest file with multiple path elements");
     } catch (IllegalArgumentException e) {
     }
 
     configFile.setDestFile("/path/destfile3");
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
       Assert.fail(EXCEPTION_PREFIX + "src file should be specified");
     } catch (IllegalArgumentException e) {
     }
@@ -140,7 +141,7 @@ public void testConfigFiles() throws IOException {
     //should succeed
     configFile.setSrcFile("srcFile");
     configFile.setDestFile("destfile3");
-    clientProvider.validateConfigFiles(configFiles, mockFs);
+    clientProvider.validateConfigFiles(configFiles, compName, mockFs);
 
     when(mockFileStatus.isDirectory()).thenReturn(true);
     when(mockFs.getFileStatus(new Path("srcFile")))
@@ -154,7 +155,7 @@ public void testConfigFiles() throws IOException {
     configFiles.add(configFile);
 
     try {
-      clientProvider.validateConfigFiles(configFiles, mockFs);
+      clientProvider.validateConfigFiles(configFiles, compName, mockFs);
       Assert.fail(EXCEPTION_PREFIX + "src file is a directory");
     } catch (IllegalArgumentException e) {
     }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/test/java/org/apache/hadoop/yarn/service/providers/TestDefaultClientProvider.java

@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.yarn.service.providers;
+
+import static org.mockito.Matchers.anyObject;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.yarn.service.api.records.ConfigFile;
+import org.apache.hadoop.yarn.service.exceptions.RestApiErrorMessages;
+import org.apache.hadoop.yarn.service.provider.defaultImpl.DefaultClientProvider;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestDefaultClientProvider {
+  private static final String EXCEPTION_PREFIX = "Should have thrown "
+      + "exception: ";
+  private static final String NO_EXCEPTION_PREFIX = "Should not have thrown "
+      + "exception: ";
+
+  @Test
+  public void testConfigFile() throws IOException {
+    DefaultClientProvider defaultClientProvider = new DefaultClientProvider();
+    FileSystem mockFs = mock(FileSystem.class);
+    when(mockFs.exists(anyObject())).thenReturn(true);
+
+    String compName = "sleeper";
+    ConfigFile configFile = new ConfigFile();
+    configFile.setDestFile("/var/tmp/a.txt");
+
+    try {
+      defaultClientProvider.validateConfigFile(configFile, compName, mockFs);
+      Assert.fail(EXCEPTION_PREFIX + " dest_file must be relative");
+    } catch (IllegalArgumentException e) {
+      String actualMsg = String.format(
+          RestApiErrorMessages.ERROR_CONFIGFILE_DEST_FILE_FOR_COMP_NOT_ABSOLUTE,
+          compName, "no", configFile.getDestFile());
+      Assert.assertEquals(actualMsg, e.getLocalizedMessage());
+    }
+
+    configFile.setDestFile("../a.txt");
+    try {
+      defaultClientProvider.validateConfigFile(configFile, compName, mockFs);
+    } catch (IllegalArgumentException e) {
+      Assert.fail(NO_EXCEPTION_PREFIX + e.getLocalizedMessage());
+    }
+  }
+}