MIDRC-768 Check if instance type is available in AZ when creating subnets for Nextflow #111
Conversation
Please find the detailed integration test report here Please find the ci env pod logs here |
paulineribeyre
changed the title
Please find the detailed integration test report here Please find the ci env pod logs here |
hatchery/nextflow.go
Outdated
| }, | ||
| { | ||
| Name: aws.String("instance-type"), | ||
| // TODO: Should this be configurable? |
There was a problem hiding this comment.
Do we need this TODO: comment? To whom is this question pointed out to?
hatchery/nextflow.go
Outdated
| return nil, fmt.Errorf("Error describing instance type offerings: %v", err) | ||
| } | ||
| if len(result.InstanceTypeOfferings) > 0 { | ||
| Config.Logger.Printf("Debug: Zone: %v has instance type g4dn.xlarge available. Using that for subnet", *zone.ZoneName) |
There was a problem hiding this comment.
Maybe change this from the hardcoded g4dn.xlarge to {instanceType} variable.
hatchery/nextflow.go
Outdated
| // TODO: read instance type from config. (MIDRC-751) | ||
| subnetId, err := setupSubnet(subnetName, subnetString, vpcid, ec2svc, "t2.micro") |
There was a problem hiding this comment.
This hardcoded string will be changed to a configurable value as a part of a future ticket. Right?
There was a problem hiding this comment.
yes, see comment. MIDRC-751
There was a problem hiding this comment.
looking at the changes this doesn't look right: setupSubnet's instanceType parameter is the type we want for nextflow jobs instances, it's not the squid instance type we want to configure in MIDRC-751 - it should be g4dn.xlarge not t2.micro. I think you got it confused with this?
so i think it should be:
setupSubnet(... availableInstanceTypes=["g4dn.xlarge"]) <-- get list from config
[...]
// TODO: read instance type from config. (MIDRC-751)
launchSquidInstance(... instanceType="t2.micro")
There was a problem hiding this comment.
You are right pauline. This instance type is just indicating the value for the subnet. t2.micro is sent as an argument to setupSubnet only within the setupSquid method (here) whereas setupSubnet method while creating Nextflow Resources uses the instance type that is defined in the hatchery config (here)
hatchery/nextflow.go
Outdated
| // Make sure the selected AZ has the instance type from nextflow configuration available. | ||
| var selectedZone string | ||
| for _, zone := range describeZonesOutput.AvailabilityZones { | ||
| if *zone.State == "available" { |
There was a problem hiding this comment.
Instead of adding an if condition here checking for availability of an Availability Zone, can we add a filter in the above line?
describeZonesOutput, err := ec2Svc.DescribeAvailabilityZones(&ec2.DescribeAvailabilityZonesInput{})
Note: I haven't looked too deep into the docs, but if the input is used as a filter, maybe that is better than us filtering it ourselves.
|
Please find the ci env pod logs here |
|
Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
There was a problem hiding this comment.
Thanks for updating this PR @nss10. I still think this comment hasn't been addressed, i could have misunderstood but could you have a look at it?
hatchery/nextflow.go
Outdated
| // TODO: Should this be configurable? | ||
| Values: []*string{aws.String(instanceType)}, |
There was a problem hiding this comment.
let's remove the todo
Please find the detailed integration test report here Please find the ci env pod logs here |
Please find the detailed integration test report here |
paulineribeyre
force-pushed
the
feat/instance-type-az
branch
from
bf31384 to
e0c8046
Compare
| } | ||
| Config.Logger.Printf("Created new license-user-map item: %v", newItem) | ||
|
|
||
| } | ||
|
|
||
| allpaymodels, err := getPayModelsForUser(userName) | ||
| if err != nil { | ||
| Config.Logger.Printf(err.Error()) | ||
| Config.Logger.Print(err.Error()) |
There was a problem hiding this comment.
If linting issues are also being addressed, shouldn't Line:495 also use Print instead of Printf?
There was a problem hiding this comment.
That one has been fixed too
Please find the detailed integration test report here Please find the ci env pod logs here |
Link to JIRA ticket if there is one:
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes