Skip to content

Commit

Permalink
feat: support mounting cloud storage in sessions (#1499) (#1518)
Browse files Browse the repository at this point in the history
* minor: more fine-grained handling of errors in notebook start
* tests: cypress test for new session UI
  • Loading branch information
ciyer authored Mar 1, 2022
1 parent 81b2d50 commit b054df5
Show file tree
Hide file tree
Showing 17 changed files with 746 additions and 36 deletions.
3 changes: 3 additions & 0 deletions client/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,14 @@
"autosuggest",
"backend",
"bool",
"borderless",
"cancellable",
"cancelled",
"cheatsheet",
"checkbox",
"ckeditor",
"cktextarea",
"cloudstorage",
"codemirror",
"craco",
"dagre",
Expand Down Expand Up @@ -222,6 +224,7 @@
"noreferrer",
"nowrap",
"nullable",
"objectstores",
"onloadend",
"papermill",
"pathname",
Expand Down
7 changes: 5 additions & 2 deletions client/src/api-client/notebook-servers.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,11 @@ function addNotebookServersMethods(client) {
}).then(resp => {
return resp.data;
}).catch(error => {
if (error.errorData && error.errorData.messages && error.errorData.messages.error)
throw new Error(error.errorData.messages.error);
if (error.errorData && error.errorData.messages && error.errorData.messages.error) {
const err = new Error(error.errorData.messages.error);
err.cause = error;
throw err;
}
});
};

Expand Down
2 changes: 1 addition & 1 deletion client/src/model/RenkuModels.js
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ const notebooksSchema = new Schema({
commit: { initial: {} },
discard: { initial: false },
options: { initial: {} },

objectStoresConfiguration: { initial: [] },
includeMergedBranches: { initial: false },
displayedCommits: { initial: 25 },
}
Expand Down
32 changes: 27 additions & 5 deletions client/src/notebooks/NotebookStart.present.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
faInfoCircle, faLink, faRedo, faSyncAlt,
} from "@fortawesome/free-solid-svg-icons";

import { NotebooksHelper } from "./index";
import { StatusHelper } from "../model/Model";
import { InfoAlert, SuccessAlert, WarnAlert } from "../utils/components/Alert";
import { ButtonWithMenu } from "../utils/components/Button";
Expand All @@ -40,12 +39,17 @@ import { Loader } from "../utils/components/Loader";
import { ThrottledTooltip } from "../utils/components/Tooltip";
import { Url } from "../utils/helpers/url";
import Time from "../utils/helpers/Time";
import { NotebooksHelper } from "./index";
import { ObjectStoresConfigurationButton, ObjectStoresConfigurationModal } from "./ObjectStoresConfig.present";

// * StartNotebookServer code * //
function StartNotebookServer(props) {
const { deleteAutosave, setCommit, setIgnorePipeline, toggleShowAdvanced } = props.handlers;
const { autosaves, autoStarting, pipelines, message, showObjectStoreModal } = props;
const { branch, commit } = props.filters;
const { autosaves, autoStarting, pipelines, message } = props;
const { objectStoresConfiguration } = props.filters;
const { deleteAutosave, setCommit, setIgnorePipeline, toggleShowAdvanced } = props.handlers;
const { toggleShowObjectStoresConfigModal } = props.handlers;


if (autoStarting)
return (<StartNotebookAutostart {...props} />);
Expand All @@ -66,10 +70,15 @@ function StartNotebookServer(props) {
(<div key="message">{message}</div>) :
null;
const disabled = fetching.branches || fetching.commits;
const s3MountsConfig = props.options.global.cloudstorage?.s3;
const cloudStorageAvailable = s3MountsConfig?.enabled ?? false;
const showAdvancedMessage = cloudStorageAvailable ?
"Do you want to select the branch, commit, or image, or configure cloud storage?" :
"Do you want to select the branch, commit, or image?";

const buttonMessage = props.showAdvanced ?
"Hide branch, commit, and image settings" :
"Do you want to select the branch, commit, or image?";
"Hide advanced settings" :
showAdvancedMessage;

const advancedSelection = (
<Fragment>
Expand All @@ -81,6 +90,19 @@ function StartNotebookServer(props) {
{show.pipelines ? <StartNotebookPipelines {...props}
ignorePipeline={props.ignorePipeline}
setIgnorePipeline={setIgnorePipeline} /> : null}
{cloudStorageAvailable ?
<Fragment>
<ObjectStoresConfigurationButton
objectStoresConfiguration={objectStoresConfiguration}
toggleShowObjectStoresConfigModal={toggleShowObjectStoresConfigModal} />
<ObjectStoresConfigurationModal
objectStoresConfiguration={objectStoresConfiguration}
showObjectStoreModal={showObjectStoreModal}
toggleShowObjectStoresConfigModal={toggleShowObjectStoresConfigModal}
setObjectStoresConfiguration={props.handlers.setObjectStoresConfiguration} />
</Fragment> :
null
}
</Collapse>
<FormGroup>
<Button color="link" className="ps-0 pe-0 pt-2 font-italic btn-sm"
Expand Down
61 changes: 42 additions & 19 deletions client/src/notebooks/Notebooks.container.js
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ class StartNotebookServer extends Component {
ignorePipeline: null,
launchError: null,
showAdvanced: false,
showObjectStoreModal: false,
starting: false
};

Expand All @@ -313,8 +314,10 @@ class StartNotebookServer extends Component {
setDisplayedCommits: this.setDisplayedCommits.bind(this),
setServerOption: this.setServerOptionFromEvent.bind(this),
startServer: this.startServer.bind(this),
setObjectStoresConfiguration: this.setObjectStoresConfiguration.bind(this),
toggleMergedBranches: this.toggleMergedBranches.bind(this),
toggleShowAdvanced: this.toggleShowAdvanced.bind(this)
toggleShowAdvanced: this.toggleShowAdvanced.bind(this),
toggleShowObjectStoresConfigModal: this.toggleShowObjectStoresConfigModal.bind(this)
};
}

Expand Down Expand Up @@ -349,6 +352,10 @@ class StartNotebookServer extends Component {
this.setState({ showAdvanced: !this.state.showAdvanced });
}

toggleShowObjectStoresConfigModal() {
this.setState({ showObjectStoreModal: !this.state.showObjectStoreModal });
}

setIgnorePipeline(value) {
this.setState({ ignorePipeline: value });
}
Expand Down Expand Up @@ -647,6 +654,11 @@ class StartNotebookServer extends Component {
}
}


setObjectStoresConfiguration(value) {
this.coordinator.setObjectStoresConfiguration(value);
}

internalStartServer() {
// The core internal logic extracted here for re-use
const { location, history } = this.props;
Expand Down Expand Up @@ -683,26 +695,36 @@ class StartNotebookServer extends Component {
//* To avoid flickering UI, just set a temporary state and display a loading wheel.
this.setState({ "starting": true, launchError: null });
this.internalStartServer().catch((error) => {
// Some failures just go away. Try again to see if it works the second time.
setTimeout(() => {
this.internalStartServer().catch((error) => {
// crafting notification
const fullError = `An error occurred when trying to start a new session.
Error message: "${error.message}", Stack trace: "${error.stack}"`;
this.notifications.addError(
this.notifications.Topics.SESSION_START,
"Unable to start the session.",
this.props.location.pathname, "Try again",
null, // always toast
fullError);
this.setState({ "starting": false, launchError: error.message });
if (this.autostart && !this.state.autostartTried)
this.setState({ autostartTried: true });
});
}, 3000);
if (error.cause && error.cause.response && error.cause.response.status) {
if (error.cause.response.status === 500) {
// Some failures just go away. Try again to see if it works the second time.
setTimeout(() => {
this.internalStartServer().catch((error) => {
this.handleNotebookStartError(error);
});
}, 3000);
}
else { this.handleNotebookStartError(error); }
}
else { this.handleNotebookStartError(error); }
});
}

handleNotebookStartError(error) {
// crafting notification
const fullError = `An error occurred when trying to start a new session.
Error message: "${error.message}", Stack trace: "${error.stack}"`;
this.notifications.addError(
this.notifications.Topics.SESSION_START,
"Unable to start the session.",
this.props.location.pathname, "Try again",
null, // always toast
fullError);
this.setState({ "starting": false, launchError: error.message });
if (this.autostart && !this.state.autostartTried)
this.setState({ autostartTried: true });
}

toggleMergedBranches() {
const currentSetting = this.model.get("filters.includeMergedBranches");
this.coordinator.setMergedBranches(!currentSetting);
Expand Down Expand Up @@ -734,7 +756,8 @@ class StartNotebookServer extends Component {
fetched: this.props.commits.fetched,
fetching: this.props.commits.fetching,
},
externalUrl: this.props.externalUrl
externalUrl: this.props.externalUrl,
showObjectStoreModal: this.state.showObjectStoreModal
};
return {
handlers: this.handlers,
Expand Down
13 changes: 11 additions & 2 deletions client/src/notebooks/Notebooks.state.js
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,8 @@ class NotebooksCoordinator {
project: null,
branch: { $set: {} },
commit: { $set: {} },
options: { $set: {} }
options: { $set: {} },
objectStoresConfiguration: { $set: [] },
},
pipelines: {
fetched: null,
Expand Down Expand Up @@ -423,6 +424,10 @@ class NotebooksCoordinator {
this.model.set(`filters.options.${option}`, value);
}

setObjectStoresConfiguration(value) {
this.model.set("filters.objectStoresConfiguration", value);
}

getQueryFilters() {
const filters = this.model.get("filters");
return {
Expand Down Expand Up @@ -922,8 +927,12 @@ class NotebooksCoordinator {
// * Change notebook status * //
startServer() {
const options = {
serverOptions: this.model.get("filters.options")
serverOptions: this.model.get("filters.options"),
};
const cloudstorage = this.model.get("filters.objectStoresConfiguration");
if (cloudstorage.length > 0)
options["cloudstorage"] = cloudstorage;

const filters = this.model.get("filters");
const namespace = filters.namespace;
const project = filters.project;
Expand Down
Loading

0 comments on commit b054df5

Please sign in to comment.