-
Notifications
You must be signed in to change notification settings - Fork 0
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
21.04.1 List of changes done by Lifebit #19
base: 21.04.1-original
Are you sure you want to change the base?
Changes from 8 commits
165b059
5f8c9be
d80f567
8728e71
b55cb31
f4fdf5d
8e437f0
14ad44a
4149f9f
3b3af7c
a7477d4
aa114bb
ff9badc
99eb9b7
9d0754b
3418de1
6ea3ab1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
# Core analyses is the default owner for this repo. | ||
* @lifebit-ai/core-analyses |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,12 +248,6 @@ class BashWrapperBuilder { | |
binding.container_env = null | ||
} | ||
|
||
/* | ||
* staging input files when required | ||
*/ | ||
final stagingScript = copyStrategy.getStageInputFilesScript(inputFiles) | ||
binding.stage_inputs = stagingScript ? "# stage input files\n${stagingScript}" : null | ||
|
||
binding.stdout_file = TaskRun.CMD_OUTFILE | ||
binding.stderr_file = TaskRun.CMD_ERRFILE | ||
binding.trace_file = TaskRun.CMD_TRACE | ||
|
@@ -262,6 +256,13 @@ class BashWrapperBuilder { | |
binding.launch_cmd = getLaunchCommand(interpreter,env) | ||
binding.stage_cmd = getStageCommand() | ||
binding.unstage_cmd = getUnstageCommand() | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont think this change make sense. so dropping it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What change did you drop? then, I'm siging the stagingScript prop being populated as well. This is needed because .getStageInputFilesScript can actually be fetching the files from s3 or directly from lustre mounted file system. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rubengomex I think only but line 263-23 is moved from 254-255, but this line (281) is newly added. that is considered for development |
||
/* | ||
* staging input and unstage output files when required | ||
*/ | ||
final stagingScript = copyStrategy.getStageInputFilesScript(inputFiles) | ||
binding.stage_inputs = stagingScript ? "# stage input files\n${stagingScript}" : null | ||
|
||
binding.unstage_controls = changeDir || shouldUnstageOutputs() ? getUnstageControls() : null | ||
|
||
if( changeDir || shouldUnstageOutputs() ) { | ||
|
@@ -277,7 +278,8 @@ class BashWrapperBuilder { | |
binding.fix_ownership = fixOwnership() ? "[ \${NXF_OWNER:=''} ] && chown -fR --from root \$NXF_OWNER ${workDir}/{*,.*} || true" : null | ||
|
||
binding.trace_script = isTraceRequired() ? getTraceScript(binding) : null | ||
|
||
binding.temp_dir = "\${1:-${copyStrategy.getTempDir(workDir)}}" | ||
|
||
return binding | ||
} | ||
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,12 +58,12 @@ class Const { | |
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure whether this is needed |
||
* The app build time as linux/unix timestamp | ||
*/ | ||
static public final long APP_TIMESTAMP = 1621005654076 | ||
static public final long APP_TIMESTAMP = 1670410083094 | ||
|
||
/** | ||
* The app build number | ||
*/ | ||
static public final int APP_BUILDNUM = 5556 | ||
static public final int APP_BUILDNUM = 5566 | ||
|
||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,11 +39,12 @@ dependencies { | |
compileOnly 'org.pf4j:pf4j:3.4.1' | ||
|
||
compile ('io.nextflow:nxf-s3fs:1.1.0') { transitive = false } | ||
compile ('com.amazonaws:aws-java-sdk-s3:1.11.542') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. already packages are using |
||
compile ('com.amazonaws:aws-java-sdk-ec2:1.11.542') | ||
compile ('com.amazonaws:aws-java-sdk-batch:1.11.542') | ||
compile ('com.amazonaws:aws-java-sdk-iam:1.11.542') | ||
compile ('com.amazonaws:aws-java-sdk-ecs:1.11.542') | ||
compile ('com.amazonaws:aws-java-sdk-s3:1.12.129') | ||
compile ('com.amazonaws:aws-java-sdk-ec2:1.12.129') | ||
compile ('com.amazonaws:aws-java-sdk-batch:1.12.129') | ||
compile ('com.amazonaws:aws-java-sdk-iam:1.12.129') | ||
compile ('com.amazonaws:aws-java-sdk-ecs:1.12.129') | ||
compile ('com.amazonaws:aws-java-sdk-sts:1.12.129') | ||
|
||
testImplementation(testFixtures(project(":nextflow"))) | ||
testImplementation project(':nextflow') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,16 +17,20 @@ | |
|
||
package nextflow.cloud.aws | ||
|
||
import com.amazonaws.AmazonClientException | ||
import com.amazonaws.auth.AWSCredentials | ||
import com.amazonaws.auth.AWSStaticCredentialsProvider | ||
import com.amazonaws.auth.BasicAWSCredentials | ||
import com.amazonaws.auth.BasicSessionCredentials | ||
import com.amazonaws.regions.InstanceMetadataRegionProvider | ||
import com.amazonaws.regions.Region | ||
import com.amazonaws.regions.RegionUtils | ||
import com.amazonaws.services.batch.AWSBatchClient | ||
import com.amazonaws.services.ec2.AmazonEC2Client | ||
import com.amazonaws.services.ecs.AmazonECS | ||
import com.amazonaws.services.ecs.AmazonECSClientBuilder | ||
import com.amazonaws.services.securitytoken.AWSSecurityTokenServiceClientBuilder | ||
import com.amazonaws.services.securitytoken.model.GetCallerIdentityRequest | ||
import groovy.transform.CompileStatic | ||
import groovy.transform.Memoized | ||
import groovy.util.logging.Slf4j | ||
|
@@ -132,37 +136,21 @@ class AmazonClientFactory { | |
* The IAM role name associated to this instance or {@code null} if no role is defined or | ||
* it's not a EC2 instance | ||
*/ | ||
protected String fetchIamRole() { | ||
private String fetchIamRole() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nf-amazon-1.12.0 already have this |
||
try { | ||
def role = getUrl('http://169.254.169.254/latest/meta-data/iam/security-credentials/').readLines() | ||
if( role.size() != 1 ) | ||
throw new IllegalArgumentException("Not a valid EC2 IAM role") | ||
return role.get(0) | ||
def stsClient = AWSSecurityTokenServiceClientBuilder.defaultClient(); | ||
def roleArn = stsClient.getCallerIdentity(new GetCallerIdentityRequest()).getArn() | ||
if(roleArn){ | ||
return roleArn.split('/')[-2] | ||
} | ||
return null | ||
} | ||
catch( IOException e ) { | ||
catch( AmazonClientException e ) { | ||
log.trace "Unable to fetch IAM credentials -- Cause: ${e.message}" | ||
return null | ||
} | ||
} | ||
|
||
/** | ||
* Fetch a remote URL resource text content | ||
* | ||
* @param path | ||
* A valid http/https resource URL | ||
* @param timeout | ||
* Max connection timeout in millis | ||
* @return | ||
* The resource URL content | ||
*/ | ||
protected String getUrl(String path, int timeout=150) { | ||
final url = new URL(path) | ||
final con = url.openConnection() | ||
con.setConnectTimeout(timeout) | ||
con.setReadTimeout(timeout) | ||
return con.getInputStream().text.trim() | ||
} | ||
|
||
/** | ||
* Retrieve the AWS region from the EC2 instance metadata. | ||
* See http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html | ||
|
@@ -171,12 +159,11 @@ class AmazonClientFactory { | |
* The AWS region of the current EC2 instance eg. {@code eu-west-1} or | ||
* {@code null} if it's not an EC2 instance. | ||
*/ | ||
protected String fetchRegion() { | ||
private String fetchRegion() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nf-amazon-1.12.0 already this |
||
try { | ||
def zone = getUrl('http://169.254.169.254/latest/meta-data/placement/availability-zone') | ||
zone ? zone.substring(0,zone.length()-1) : null | ||
return new InstanceMetadataRegionProvider().getRegion() | ||
} | ||
catch (IOException e) { | ||
catch (AmazonClientException e) { | ||
log.debug "Cannot fetch AWS region", e | ||
return null | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,11 +94,13 @@ class AwsBatchExecutor extends Executor implements ExtensionPoint { | |
|
||
protected void validateWorkDir() { | ||
/* | ||
* make sure the work dir is a S3 bucket | ||
* make sure the work dir is a S3 bucket and if we are not usign lustre fsx | ||
*/ | ||
if( !(workDir instanceof S3Path) ) { | ||
def isUsingLustre = session.config.navigate('cloud.fsxFileSystemsMountCommands') | ||
log.debug "Checking workdir validation, isUsingLustre $isUsingLustre" | ||
if( !(workDir instanceof S3Path) && !isUsingLustre ) { | ||
session.abort() | ||
throw new AbortOperationException("When using `$name` executor a S3 bucket must be provided as working directory either using -bucket-dir or -work-dir command line option") | ||
throw new AbortOperationException("When using `$name` executor and we are not using Lustre storage a S3 bucket must be provided as working directory either using -bucket-dir or -work-dir command line option") | ||
} | ||
} | ||
|
||
|
@@ -251,6 +253,20 @@ class AwsBatchExecutor extends Executor implements ExtensionPoint { | |
@PackageScope | ||
ThrottlingExecutor getReaper() { reaper } | ||
|
||
String getInstanceIdByQueueAndTaskArn(String queue, String taskArn) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this |
||
try { | ||
return helper?.getInstanceIdByQueueAndTaskArn(queue, taskArn) | ||
} | ||
catch ( AccessDeniedException e ) { | ||
log.warn "Unable to retrieve AWS Batch instance Id | ${e.message}" | ||
return null | ||
} | ||
catch( Exception e ) { | ||
log.warn "Unable to retrieve AWS batch instance id for queue=$queue; task=$taskArn | ${e.message}", e | ||
return null | ||
} | ||
} | ||
|
||
|
||
CloudMachineInfo getMachineInfoByQueueAndTaskArn(String queue, String taskArn) { | ||
try { | ||
|
@@ -267,14 +283,4 @@ class AwsBatchExecutor extends Executor implements ExtensionPoint { | |
return null | ||
} | ||
} | ||
|
||
} | ||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
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 changes done by @mageshwaran-lifebit