Skip to content
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

Issue 44126: CommandTasks fail on remote pipeline servers #2689

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/src/org/labkey/api/assay/DefaultDataTransformer.java
Expand Up @@ -202,7 +202,7 @@ public TransformResult transformAndValidate(AssayRunUploadContext<ProviderType>
return result;
}

public static void addStandardParameters(@Nullable HttpServletRequest request, Container container, @Nullable File scriptFile, String apiKey, @NotNull Map<String, String> paramMap)
public static void addStandardParameters(@Nullable HttpServletRequest request, @Nullable Container container, @Nullable File scriptFile, @Nullable String apiKey, @NotNull Map<String, String> paramMap)
{
if (scriptFile != null)
{
Expand All @@ -216,7 +216,7 @@ public static void addStandardParameters(@Nullable HttpServletRequest request, C
paramMap.put(SecurityManager.API_KEY, apiKey);
paramMap.put(BASE_SERVER_URL_REPLACEMENT, AppProps.getInstance().getBaseServerUrl()
+ AppProps.getInstance().getContextPath());
paramMap.put(CONTAINER_PATH, container.getPath());
paramMap.put(CONTAINER_PATH, container == null ? null : container.getPath());
}

@Deprecated
Expand Down
14 changes: 12 additions & 2 deletions api/src/org/labkey/api/pipeline/PipelineJob.java
Expand Up @@ -1651,23 +1651,33 @@ public String getContainerId()

/**
* Gets the <code>User</code> instance from the <code>ViewBackgroundInfo</code>.
* WARNING: Not supported if job is not running in the LabKey Server.
* WARNING: Not supported if job is not running in the LabKey web server.
*
* @return the user who started the job
* @throws IllegalStateException if invoked on a remote pipeline server
*/
public User getUser()
{
if (!PipelineJobService.get().isWebServer())
{
throw new IllegalStateException("User lookup not available on remote pipeline servers");
}
return getInfo().getUser();
}

/**
* Gets the <code>Container</code> instance from the <code>ViewBackgroundInfo</code>.
* WARNING: Not supported if job is not running in the LabKey Server.
* WARNING: Not supported if job is not running in the LabKey web server.
*
* @return the container in which the job was started
* @throws IllegalStateException if invoked on a remote pipeline server
*/
public Container getContainer()
{
if (!PipelineJobService.get().isWebServer())
{
throw new IllegalStateException("User lookup not available on remote pipeline servers");
}
return getInfo().getContainer();
}

Expand Down
7 changes: 6 additions & 1 deletion api/src/org/labkey/api/pipeline/PipelineJobService.java
Expand Up @@ -175,5 +175,10 @@ enum LocationType
RemoteExecutionEngine
}

public FormSchema getFormSchema(Container container);
FormSchema getFormSchema(Container container);

/** @return true if the current instance is the web server, which has access to more resources including the
* primary database, or false if we're on a remote server
*/
boolean isWebServer();
}
7 changes: 5 additions & 2 deletions api/src/org/labkey/api/security/SecurityManager.java
Expand Up @@ -646,6 +646,9 @@ public String getApiKey()
{
return _apikey;
}

@Override
public abstract void close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing a build break in CromwellGctTask (MacCossLabModules)... need to remove catch (IOException) there

}

private static final class HttpSessionTransformSession extends TransformSession
Expand All @@ -657,7 +660,7 @@ private HttpSessionTransformSession(@NotNull HttpServletRequest req, @NotNull Ht
}

@Override
public void close() throws IOException
public void close()
{
SessionApiKeyManager.get().invalidateKey(getApiKey());
}
Expand All @@ -672,7 +675,7 @@ private DatabaseTransformSession(@NotNull User user)
}

@Override
public void close() throws IOException
public void close()
{
ApiKeyManager.get().deleteKey(getApiKey());
}
Expand Down
29 changes: 21 additions & 8 deletions pipeline/src/org/labkey/pipeline/analysis/CommandTaskImpl.java
Expand Up @@ -24,13 +24,15 @@
import org.junit.Test;
import org.labkey.api.assay.DefaultDataTransformer;
import org.labkey.api.collections.RowMapFactory;
import org.labkey.api.data.Container;
import org.labkey.api.data.TSVMapWriter;
import org.labkey.api.exp.PropertyType;
import org.labkey.api.module.Module;
import org.labkey.api.pipeline.AbstractTaskFactory;
import org.labkey.api.pipeline.PipeRoot;
import org.labkey.api.pipeline.PipelineJob;
import org.labkey.api.pipeline.PipelineJobException;
import org.labkey.api.pipeline.PipelineJobService;
import org.labkey.api.pipeline.RecordedAction;
import org.labkey.api.pipeline.RecordedActionSet;
import org.labkey.api.pipeline.TaskId;
Expand Down Expand Up @@ -586,7 +588,7 @@ protected void writeTaskInfo(File file, RecordedAction action) throws IOExceptio
* to replace tokens the script or the command line before executing it.
* The replaced paths will be resolved to paths in the work directory.
*/
protected Map<String, String> createReplacements(@Nullable File scriptFile, String apiKey) throws IOException
protected Map<String, String> createReplacements(@Nullable File scriptFile, @Nullable String apiKey, @Nullable Container container) throws IOException
{
Map<String, String> replacements = new HashMap<>();

Expand Down Expand Up @@ -660,7 +662,7 @@ else if (outputPaths.length == 1)
replacements.put(PipelineJob.PIPELINE_TASK_OUTPUT_PARAMS_PARAM, taskOutputParamsRelativePath);
}

DefaultDataTransformer.addStandardParameters(null, getJob().getContainer(), scriptFile, apiKey, replacements);
DefaultDataTransformer.addStandardParameters(null, container, scriptFile, apiKey, replacements);

return replacements;
}
Expand All @@ -675,7 +677,16 @@ protected String rewritePath(String path)
@NotNull
public RecordedActionSet run() throws PipelineJobException
{
try (TransformSession session = SecurityManager.createTransformSession(getJob().getUser()))
TransformSession session = null;
Container container = null;
if (PipelineJobService.get().isWebServer())
{
// We're inside of the web server so we have access to the DB and can set up a transform session, among
// other resources
session = SecurityManager.createTransformSession(getJob().getUser());
container = getJob().getContainer();
}
try
{
RecordedAction action = new RecordedAction(_factory.getProtocolActionName());

Expand All @@ -702,7 +713,7 @@ public RecordedActionSet run() throws PipelineJobException
if (getJobSupport().getParametersFile() != null)
_wd.inputFile(getJobSupport().getParametersFile(), true);

if (!runCommand(action, session.getApiKey()))
if (!runCommand(action, session == null ? null : session.getApiKey(), container))
return new RecordedActionSet();

// Read output parameters file, record output parameters, and discard it.
Expand Down Expand Up @@ -733,6 +744,10 @@ public RecordedActionSet run() throws PipelineJobException
finally
{
_wd = null;
if (session != null)
{
session.close();
}
}
}

Expand All @@ -741,13 +756,11 @@ public RecordedActionSet run() throws PipelineJobException
* @param action The recorded action.
* @param apiKey API key to use for the duration of the command execution
* @return true if the task was run, false otherwise.
* @throws IOException
* @throws PipelineJobException
*/
// TODO: Add task and job version information to the recorded action.
protected boolean runCommand(RecordedAction action, String apiKey) throws IOException, PipelineJobException
protected boolean runCommand(RecordedAction action, @Nullable String apiKey, @Nullable Container container) throws IOException, PipelineJobException
{
Map<String, String> replacements = createReplacements(null, apiKey);
Map<String, String> replacements = container == null ? Collections.emptyMap() : createReplacements(null, apiKey, container);

ProcessBuilder pb = new ProcessBuilder(_factory.toArgs(this, replacements));
applyEnvironment(pb);
Expand Down
Expand Up @@ -113,25 +113,34 @@ private TaskFactory findCommandFactory(PipelineJob job)
// If this job is not actually running a conversion, then no
// converter command can be determined.
List<File> files = job.getJobSupport(FileAnalysisJobSupport.class).getInputFiles();
LOG.debug("Checking " + files + " for possible converters");
if (files == null || files.size() != 1)
return null;

// Otherwise, find the appropriate converter.
File fileInput = getInputFile(job);
LOG.debug("Checking " + fileInput + " against up to " + _commands.length + " possible converters");
for (TaskId tid : _commands)
{
LOG.debug("Checking " + fileInput + " against " + tid);
TaskFactory<? extends TaskFactorySettings> factory = PipelineJobService.get().getTaskFactory(tid);
for (FileType ft : factory.getInputTypes())
{
LOG.debug("Checking " + fileInput + " against " + tid + ": " + ft);
try
{
// If we have a match based on the file type and the factory says that it's a participant based
// on the search protocol parameters, use it
if (ft.isType(fileInput) && factory.isParticipant(job))
{
LOG.debug("Matched");
return factory;
}
LOG.debug("No match");
}
catch (IOException ignored)
catch (IOException e)
{
LOG.debug("Exception when checking " + fileInput + ", reporting that " + tid + " is not available", e);
// Consider this command out of the running, caller will report an error if there are no other options
}
}
Expand Down
Expand Up @@ -589,6 +589,12 @@ public FormSchema getFormSchema(Container container)
return new FormSchema(fields);
}

@Override
public boolean isWebServer()
{
return ModuleLoader.getServletContext() != null;
}

public void setJobStore(PipelineStatusFile.JobStore jobStore)
{
_jobStore = jobStore;
Expand Down
4 changes: 2 additions & 2 deletions pipeline/src/org/labkey/pipeline/api/ScriptTaskImpl.java
Expand Up @@ -82,7 +82,7 @@ private ScriptEngine getScriptEngine(Container c, LabKeyScriptEngineManager mgr,
// TODO: Rhino engine. A non-ExternalScriptEngine won't use the PARAM_REPLACEMENT_MAP binding.
// CONSIDER: Use ScriptEngineReport to generate a script prolog
@Override
protected boolean runCommand(RecordedAction action, String apiKey) throws IOException, PipelineJobException
protected boolean runCommand(RecordedAction action, @Nullable String apiKey, @Nullable Container container) throws IOException, PipelineJobException
{
// Get the script engine
LabKeyScriptEngineManager mgr = LabKeyScriptEngineManager.get();
Expand Down Expand Up @@ -160,7 +160,7 @@ else if (factory._scriptPath != null)
if (_factory.getTimeout() != null && _factory.getTimeout() > 0)
bindings.put(ExternalScriptEngine.TIMEOUT, _factory.getTimeout());

Map<String, String> replacements = createReplacements(scriptFile, apiKey);
Map<String, String> replacements = createReplacements(scriptFile, apiKey, container);
bindings.put(ExternalScriptEngine.PARAM_REPLACEMENT_MAP, replacements);

// Write task properties file into the work directory
Expand Down