[PF-692] Pass workspace bucket zone/region parameter to Leonardo createRuntime… #2441
Conversation
src/components/region-common.js
Outdated
| @@ -1,74 +1,77 @@ | |||
| // Get a { flag: ..., countryName: ... } object representing a google locationType/location input. | |||
| // 'flag' will always be defined (even if it's a question mark. | |||
| // 'regionDescription' is the same as location when locationType is 'multi-region', or a country name when locationType is 'region'. | |||
| // computeZone is generally the 'a' zone for each region, except for those regions where it is not available. | |||
| // The choice to use the 'a' zone is arbitrary, choosing 'b' zone would also work. | |||
| // The region choice for multi-region locations is arbitrary as well. | |||
| export const unknownRegionFlag = '❓' | |||
| export const regionInfo = (location, locationType) => { | |||
| switch (locationType) { | |||
| case 'multi-region': | |||
| switch (location) { | |||
There was a problem hiding this comment.
we have a utility function Utils.switchcase (in utils.js) to clean up the syntax of switches a bit. I know it existed before your changes, but could be nice to clean up. Up to you
src/components/NewRuntimeModal.js
Outdated
| @@ -258,10 +261,12 @@ export const NewRuntimeModal = withModalDrawer({ width: 675 })(class NewRuntimeM | |||
| const shouldDeleteRuntime = oldRuntime && !this.canUpdateRuntime() | |||
| const shouldCreateRuntime = !this.canUpdateRuntime() && newRuntime | |||
| const { name, bucketName, googleProject } = this.getWorkspaceObj() | |||
| const { computeZone, clusterRegion } = regionInfo(bucketLocation, bucketLocationType) | |||
There was a problem hiding this comment.
can we call clusterRegion computeRegion for consistency? (comment applies to region-common.js too)
| @@ -487,13 +499,17 @@ export const NewRuntimeModal = withModalDrawer({ width: 675 })(class NewRuntimeM | |||
| currentPersistentDisk ? Ajax().Disks.disk(currentPersistentDisk.googleProject, currentPersistentDisk.name).details() : null | |||
| ]) | |||
|
|
|||
| const { location, locationType } = await Ajax().Workspaces.workspace(namespace, workspaceName).checkBucketLocation(bucketName) | |||
There was a problem hiding this comment.
we may want to provide a default here in case the remote call fails (google down-time, etc), otherwise there will be some errors that cascade (thinking about the failure mode of this line in particular const { computeZone, clusterRegion } = regionInfo(bucketLocation, bucketLocationType). Unlikely that the app would actually work in the proposed scenario, but I like guarding against javascript crash scenarios wherever possible...
There was a problem hiding this comment.
Played around with it locally and everything passed to leo and in the code looks good. Couple small nits for consistency/redundancy.
Ran the integration tests locally and they were all green, they will not pass here as this PR is from a fork.
The conflict its complaining about will look daunting when you try to rebase, but its just a lot of renaming, which can be referenced here #2447
| default: | ||
| return { flag: unknownRegionFlag, regionDescription: `${locationType}: ${location}` } | ||
| } | ||
| return Utils.switchCase(locationType, |
There was a problem hiding this comment.
If you declare a
const regionDescription = `${locationType}: ${location}`above this, you can simplify a lot of the lines below.
|
This pull request introduces 1 alert when merging ae5967f into 8574dbf - view on LGTM.com new alerts:
|
yonghaoy
changed the title
yonghaoy
changed the title
|
@wnojopra Curious if we should expect any updates on this PR soon? |
|
Thanks for asking @kyuksel . We'd like to merge this PR together with #2420 as they both introduce rationality to Terra. By introducing regional buckets, users could end up accruing egress charges when data is moved between regions. This could happen between a regional bucket and a notebook VM or a workflow VM. To address this, I'm working on two PRs in Cromwell (cromwell/pull/6332 and cromwell/pull/6324)that adds options to minimize egress charges. I'd like to get those changes in before we make regional buckets/VMs in Terra more accessible. TL;DR - pending Cromwell PRs broadinstitute/cromwell#6324 and broadinstitute/cromwell#6332 |
|
Closing because this PR appears to be stale. If you need to finish this work, feel free to reopen. |
In DataBiosphere/leonardo#1933, zone and region parameters were added to createRuntime so Leo can now create instances outside of us-central1. This PR will use the location of the workspace bucket to determine the region to create the instance.
In a future PR I plan to have a UI dropdown that will allow a user to specifically choose the location of their cloud environment (defaulting to their current workspace's bucket location).
I have tested:
In all cases I verified that the instance was created in the specified zone:
