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.

Sign up for GitHub

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

Jump to bottom

[RW-2704][risk=low] Split up listCluster API and move initialization logic to the client-side. #3135

Merged
merged 13 commits into from
Feb 27, 2020
Merged

[RW-2704][risk=low] Split up listCluster API and move initialization logic to the client-side. #3135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6a26dd3
Work in progress on API refactor and unified client-side polling / in…
gjuggler Feb 14, 2020
2bf95e8
WIP fleshing out cluster retry loop.
gjuggler Feb 19, 2020
62b03f7
Add and fix unit & controller tests.
gjuggler Feb 20, 2020
9d3001e
A few more text fixes.
gjuggler Feb 20, 2020
0587b2a
Incorporate initializer into reset-cluster-button, use jest.fakeTimer…
gjuggler Feb 21, 2020
0fa7a7a
Fix API tests.
gjuggler Feb 21, 2020
87439e1
Add some class comments.
gjuggler Feb 21, 2020
e84ca22
Fix ng lint error
gjuggler Feb 21, 2020
3bc2e4b
PR feedback.
gjuggler Feb 24, 2020
4b21497
Spotless formatting and test fix.
gjuggler Feb 25, 2020
560306b
Fix ts lint
gjuggler Feb 25, 2020
59a604b
Revert unintentional change to DataSetController (bad rebase).
gjuggler Feb 25, 2020
cf112c3
Final PR feedback.
gjuggler Feb 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 59 additions & 77 deletions api/src/main/java/org/pmiops/workbench/api/ClusterController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,11 @@
import com.google.gson.Gson;
import java.net.MalformedURLException;
import java.net.URL;
import java.sql.Timestamp;
import java.time.Clock;
import java.util.Base64;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -28,6 +26,7 @@
import org.pmiops.workbench.db.model.DbCdrVersion;
import org.pmiops.workbench.db.model.DbUser;
import org.pmiops.workbench.db.model.DbUser.ClusterConfig;
import org.pmiops.workbench.db.model.DbWorkspace;
import org.pmiops.workbench.exceptions.BadRequestException;
import org.pmiops.workbench.exceptions.NotFoundException;
import org.pmiops.workbench.exceptions.ServerErrorException;
Expand All @@ -38,14 +37,12 @@
import org.pmiops.workbench.model.ClusterLocalizeRequest;
import org.pmiops.workbench.model.ClusterLocalizeResponse;
import org.pmiops.workbench.model.ClusterStatus;
import org.pmiops.workbench.model.DefaultClusterResponse;
import org.pmiops.workbench.model.EmptyResponse;
import org.pmiops.workbench.model.ListClusterDeleteRequest;
import org.pmiops.workbench.model.ListClusterResponse;
import org.pmiops.workbench.model.UpdateClusterConfigRequest;
import org.pmiops.workbench.model.WorkspaceAccessLevel;
import org.pmiops.workbench.notebooks.LeonardoNotebooksClient;
import org.pmiops.workbench.notebooks.model.ClusterError;
import org.pmiops.workbench.notebooks.model.StorageLink;
import org.pmiops.workbench.workspaces.WorkspaceService;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -187,115 +184,99 @@ public ResponseEntity<List<ListClusterResponse>> deleteClustersInProject(
return ResponseEntity.ok(clustersInProjectAffected);
}

private DbWorkspace lookupWorkspace(String workspaceNamespace) throws NotFoundException {
return workspaceService
.getByNamespace(workspaceNamespace)
.orElseThrow(() -> new NotFoundException("Workspace not found: " + workspaceNamespace));
jaycarlton marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public ResponseEntity<DefaultClusterResponse> listClusters(
String billingProjectId, String workspaceFirecloudName) {
if (billingProjectId == null) {
throw new BadRequestException("Must specify billing project");
}
public ResponseEntity<Cluster> getCluster(String workspaceNamespace) {
String firecloudWorkspaceName = lookupWorkspace(workspaceNamespace).getFirecloudName();
Copy link
Contributor

@jaycarlton jaycarlton Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

nit: can the body of this function be pushed into the WorkspaceService, and have it return Optional<Cluster>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO it wouldn't be worth it. This controller is <10 lines (including wrapping!) which is way shorter than before, and very much in the spirit of https://github.com/all-of-us/workbench/blob/master/api/doc/code-structure.md#controllers.

(And, upon reading that section again, I would probably disagree that 'controllers should contain no actual logic'. IMO, having some calls to well-factored access enforcement and validity checking service methods is more reasonably the scope of a controller than of a service.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I can go along with that. I just haven't see much of a clean example of how to draw the line. If we're not really MVC, then I don't know what the "C" means.

workspaceService.enforceWorkspaceAccessLevel(
billingProjectId, workspaceFirecloudName, WorkspaceAccessLevel.READER);
workspaceService.validateActiveBilling(billingProjectId, workspaceFirecloudName);
workspaceNamespace, firecloudWorkspaceName, WorkspaceAccessLevel.WRITER);
workspaceService.validateActiveBilling(workspaceNamespace, firecloudWorkspaceName);

DbUser user = this.userProvider.get();
org.pmiops.workbench.notebooks.model.Cluster firecloudCluster =
gjuggler marked this conversation as resolved.
Show resolved Hide resolved
leonardoNotebooksClient.getCluster(
workspaceNamespace, clusterNameForUser(userProvider.get()));

String clusterName = clusterNameForUser(user);
return ResponseEntity.ok(TO_ALL_OF_US_CLUSTER.apply(firecloudCluster));
}

org.pmiops.workbench.notebooks.model.Cluster fcCluster;
try {
fcCluster = this.leonardoNotebooksClient.getCluster(billingProjectId, clusterName);
} catch (NotFoundException e) {
fcCluster =
this.leonardoNotebooksClient.createCluster(
billingProjectId, clusterName, workspaceFirecloudName);
}
@Override
public ResponseEntity<Cluster> createCluster(String workspaceNamespace) {
String firecloudWorkspaceName = lookupWorkspace(workspaceNamespace).getFirecloudName();
jaycarlton marked this conversation as resolved.
Show resolved Hide resolved
workspaceService.enforceWorkspaceAccessLevel(
workspaceNamespace, firecloudWorkspaceName, WorkspaceAccessLevel.WRITER);
workspaceService.validateActiveBilling(workspaceNamespace, firecloudWorkspaceName);

int retries = Optional.ofNullable(user.getClusterCreateRetries()).orElse(0);
if (org.pmiops.workbench.notebooks.model.ClusterStatus.ERROR.equals(fcCluster.getStatus())) {
if (retries <= 2) {
this.userService.setClusterRetryCount(retries + 1);
log.warning("Cluster has errored with logs: ");
if (fcCluster.getErrors() != null) {
for (ClusterError e : fcCluster.getErrors()) {
log.warning(e.getErrorMessage());
}
}
log.warning("Retrying cluster creation.");

this.leonardoNotebooksClient.deleteCluster(billingProjectId, clusterName);
}
} else if (org.pmiops.workbench.notebooks.model.ClusterStatus.RUNNING.equals(
fcCluster.getStatus())
&& retries != 0) {
this.userService.setClusterRetryCount(0);
}
DefaultClusterResponse resp = new DefaultClusterResponse();
resp.setDefaultCluster(TO_ALL_OF_US_CLUSTER.apply(fcCluster));
return ResponseEntity.ok(resp);
org.pmiops.workbench.notebooks.model.Cluster firecloudCluster =
leonardoNotebooksClient.createCluster(
workspaceNamespace, clusterNameForUser(userProvider.get()), firecloudWorkspaceName);
jaycarlton marked this conversation as resolved.
Show resolved Hide resolved

return ResponseEntity.ok(TO_ALL_OF_US_CLUSTER.apply(firecloudCluster));
}

@Override
public ResponseEntity<EmptyResponse> deleteCluster(String projectName, String clusterName) {
this.userService.setClusterRetryCount(0);
this.leonardoNotebooksClient.deleteCluster(projectName, clusterName);
public ResponseEntity<EmptyResponse> deleteCluster(String workspaceNamespace) {
String firecloudWorkspaceName = lookupWorkspace(workspaceNamespace).getFirecloudName();
workspaceService.enforceWorkspaceAccessLevel(
workspaceNamespace, firecloudWorkspaceName, WorkspaceAccessLevel.WRITER);

leonardoNotebooksClient.deleteCluster(
workspaceNamespace, clusterNameForUser(userProvider.get()));
return ResponseEntity.ok(new EmptyResponse());
}

@Override
public ResponseEntity<ClusterLocalizeResponse> localize(
String projectName, String clusterName, ClusterLocalizeRequest body) {
String workspaceNamespace, ClusterLocalizeRequest body) {
gjuggler marked this conversation as resolved.
Show resolved Hide resolved
DbWorkspace dbWorkspace = lookupWorkspace(workspaceNamespace);
workspaceService.enforceWorkspaceAccessLevel(
body.getWorkspaceNamespace(), body.getWorkspaceId(), WorkspaceAccessLevel.READER);
workspaceService.validateActiveBilling(body.getWorkspaceNamespace(), body.getWorkspaceId());
dbWorkspace.getWorkspaceNamespace(),
dbWorkspace.getFirecloudName(),
WorkspaceAccessLevel.WRITER);
workspaceService.validateActiveBilling(
dbWorkspace.getWorkspaceNamespace(), dbWorkspace.getFirecloudName());

FirecloudWorkspace fcWorkspace;
final FirecloudWorkspace firecloudWorkspace;
try {
fcWorkspace =
firecloudWorkspace =
fireCloudService
.getWorkspace(body.getWorkspaceNamespace(), body.getWorkspaceId())
.getWorkspace(dbWorkspace.getWorkspaceNamespace(), dbWorkspace.getFirecloudName())
.getWorkspace();
} catch (NotFoundException e) {
throw new NotFoundException(
String.format(
"workspace %s/%s not found or not accessible",
body.getWorkspaceNamespace(), body.getWorkspaceId()));
dbWorkspace.getWorkspaceNamespace(), dbWorkspace.getFirecloudName()));
}
DbCdrVersion cdrVersion =
workspaceService
.getRequired(body.getWorkspaceNamespace(), body.getWorkspaceId())
.getCdrVersion();
DbCdrVersion cdrVersion = dbWorkspace.getCdrVersion();

// For the common case where the notebook cluster matches the workspace
// namespace, simply name the directory as the workspace ID; else we
// include the namespace in the directory name to avoid possible conflicts
// in workspace IDs.
String gcsNotebooksDir = "gs://" + fcWorkspace.getBucketName() + "/notebooks";
Timestamp now = new Timestamp(clock.instant().toEpochMilli());
long workspaceId =
workspaceService
.getRequired(body.getWorkspaceNamespace(), body.getWorkspaceId())
.getWorkspaceId();
String gcsNotebooksDir = "gs://" + firecloudWorkspace.getBucketName() + "/notebooks";
gjuggler marked this conversation as resolved.
Show resolved Hide resolved
long workspaceId = dbWorkspace.getWorkspaceId();

body.getNotebookNames()
.forEach(
notebook ->
notebookName ->
userRecentResourceService.updateNotebookEntry(
workspaceId, userProvider.get().getUserId(), gcsNotebooksDir + "/" + notebook));
String workspacePath = body.getWorkspaceId();
if (!projectName.equals(body.getWorkspaceNamespace())) {
workspacePath =
body.getWorkspaceNamespace()
+ FireCloudService.WORKSPACE_DELIMITER
+ body.getWorkspaceId();
}
workspaceId,
userProvider.get().getUserId(),
gcsNotebooksDir + "/" + notebookName));

String workspacePath = dbWorkspace.getFirecloudName();
gjuggler marked this conversation as resolved.
Show resolved Hide resolved
String editDir = "workspaces/" + workspacePath;
String playgroundDir = "workspaces_playground/" + workspacePath;
String targetDir = body.getPlaygroundMode() ? playgroundDir : editDir;

leonardoNotebooksClient.createStorageLink(
projectName,
clusterName,
workspaceNamespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename targetDir to notebookLocalDir? Also, collecting lines 269-272 into a helper method would be nice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll defer to future work. No functional changes were made to localize, so I'd like to avoid doing too much bike-shedding here.

clusterNameForUser(userProvider.get()),
new StorageLink()
.cloudStorageDirectory(gcsNotebooksDir)
.localBaseDirectory(editDir)
Expand All @@ -307,7 +288,7 @@ public ResponseEntity<ClusterLocalizeResponse> localize(

// The Welder extension offers direct links to/from playground mode; write the AoU config file
// to both locations so notebooks will work in either directory.
String aouConfigUri = aouConfigDataUri(fcWorkspace, cdrVersion, projectName);
String aouConfigUri = aouConfigDataUri(firecloudWorkspace, cdrVersion, workspaceNamespace);
gjuggler marked this conversation as resolved.
Show resolved Hide resolved
localizeMap.put(editDir + "/" + AOU_CONFIG_FILENAME, aouConfigUri);
localizeMap.put(playgroundDir + "/" + AOU_CONFIG_FILENAME, aouConfigUri);

Expand All @@ -319,8 +300,9 @@ public ResponseEntity<ClusterLocalizeResponse> localize(
Collectors.toMap(
name -> targetDir + "/" + name, name -> gcsNotebooksDir + "/" + name)));
}

leonardoNotebooksClient.localize(projectName, clusterName, localizeMap);
log.info(localizeMap.toString());
leonardoNotebooksClient.localize(
workspaceNamespace, clusterNameForUser(userProvider.get()), localizeMap);

// This is the Jupyer-server-root-relative path, the style used by the Jupyter REST API.
return ResponseEntity.ok(new ClusterLocalizeResponse().clusterLocalDirectory(targetDir));
Expand Down
8 changes: 7 additions & 1 deletion api/src/main/java/org/pmiops/workbench/notebooks/LeonardoNotebooksClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.pmiops.workbench.db.model.DbUser.ClusterConfig;
import org.pmiops.workbench.db.model.DbWorkspace;
import org.pmiops.workbench.db.model.DbWorkspace.BillingMigrationStatus;
import org.pmiops.workbench.exceptions.ExceptionUtils;
import org.pmiops.workbench.notebooks.api.ClusterApi;
import org.pmiops.workbench.notebooks.api.NotebooksApi;
import org.pmiops.workbench.notebooks.api.StatusApi;
Expand Down Expand Up @@ -171,7 +172,12 @@ public void deleteCluster(String googleProject, String clusterName) {
@Override
public Cluster getCluster(String googleProject, String clusterName) {
ClusterApi clusterApi = clusterApiProvider.get();
return retryHandler.run((context) -> clusterApi.getCluster(googleProject, clusterName));
try {
return retryHandler.runAndThrowChecked(
(context) -> clusterApi.getCluster(googleProject, clusterName));
gjuggler marked this conversation as resolved.
Show resolved Hide resolved
} catch (ApiException e) {
throw ExceptionUtils.convertNotebookException(e);
}
}

@Override
Expand Down
3 changes: 3 additions & 0 deletions api/src/main/java/org/pmiops/workbench/workspaces/WorkspaceService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import org.pmiops.workbench.db.model.DbUserRecentWorkspace;
import org.pmiops.workbench.db.model.DbWorkspace;
import org.pmiops.workbench.exceptions.ForbiddenException;
import org.pmiops.workbench.exceptions.NotFoundException;
import org.pmiops.workbench.firecloud.FireCloudService;
import org.pmiops.workbench.firecloud.model.FirecloudWorkspaceACLUpdate;
import org.pmiops.workbench.firecloud.model.FirecloudWorkspaceAccessEntry;
Expand All @@ -33,6 +34,8 @@ public interface WorkspaceService {

List<WorkspaceResponse> getWorkspacesAndPublicWorkspaces();

WorkspaceResponse getWorkspace(String workspaceNamespace) throws NotFoundException;

WorkspaceResponse getWorkspace(String workspaceNamespace, String workspaceId);

List<WorkspaceResponse> getWorkspaces();
Expand Down
16 changes: 15 additions & 1 deletion api/src/main/java/org/pmiops/workbench/workspaces/WorkspaceServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,18 +204,32 @@ public List<WorkspaceResponse> getWorkspacesAndPublicWorkspaces() {
.collect(Collectors.toList());
}

@Transactional
@Override
public WorkspaceResponse getWorkspace(String workspaceNamespace) throws NotFoundException {
DbWorkspace dbWorkspace =
getByNamespace(workspaceNamespace)
.orElseThrow(() -> new NotFoundException("Workspace not found: " + workspaceNamespace));
return getWorkspaceImpl(dbWorkspace);
}

@Transactional
@Override
public WorkspaceResponse getWorkspace(String workspaceNamespace, String workspaceId) {
DbWorkspace dbWorkspace = getRequired(workspaceNamespace, workspaceId);
return getWorkspaceImpl(dbWorkspace);
}

private WorkspaceResponse getWorkspaceImpl(DbWorkspace dbWorkspace) {
FirecloudWorkspaceResponse fcResponse;
FirecloudWorkspace fcWorkspace;

WorkspaceResponse workspaceResponse = new WorkspaceResponse();

// This enforces access controls.
fcResponse = fireCloudService.getWorkspace(workspaceNamespace, workspaceId);
fcResponse =
fireCloudService.getWorkspace(
dbWorkspace.getWorkspaceNamespace(), dbWorkspace.getFirecloudName());
Copy link
Contributor

Choose a reason for hiding this comment

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

at line 241 below, I'm surprised to see us throwing a WorkbenchException exception from the service level; in principle code that's not running in a REST stack at all should be able to call this method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there anything in WorkbenchException that requires REST? My conceptual model is that WorkbenchExcxeption contains typed exceptions that can be thrown at the service level, and then we use @ResponseStatus annotations to allow our controller / web layer to appropriately map these exceptions to response codes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking @ericsong was leading the charge to get these out of the service level. Maybe I misunderstood.

fcWorkspace = fcResponse.getWorkspace();

if (fcResponse.getAccessLevel().equals(WorkspaceService.PROJECT_OWNER_ACCESS_LEVEL)) {
Expand Down
Loading