fix(tgnms): Planning - show error messages, launch_error state

Summary: * creates a LAUNCH_ERROR plan state. LAUNCH_ERROR means that the plan failed before it could be launched, meaning it won't have an FBID and can be retried. ERROR means that the plan failed after launching. * Shows the launch error messages in the UI Reviewed By: tommyhuynh Differential Revision: D33247811 fbshipit-source-id: 0d2d3aad00dd1aca258caf41a8d229947c3a045c
terragraph · Jan 31, 2022 · 4bdc2c7 · 4bdc2c7
1 parent 02118f9
commit 4bdc2c7
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 39 deletions.
diff --git a/tgnms/fbcnms-projects/tgnms/app/features/planning/components/PlanEditor.js b/tgnms/fbcnms-projects/tgnms/app/features/planning/components/PlanEditor.js
@@ -117,7 +117,13 @@ export default function PlanEditor({
       startPlanTask.success();
     } catch (err) {
       startPlanTask.error();
-      startPlanTask.setMessage(err.message);
+      if (err.response?.data?.errors != null) {
+        startPlanTask.setMessage(
+          `Plan failed to launch: ${err.response.data.errors.join('\n')}`,
+        );
+      } else {
+        startPlanTask.setMessage(err.message);
+      }
     }
   }, [onPlanLaunched, plan, startPlanTask]);
 
@@ -200,6 +206,13 @@ export default function PlanEditor({
           <CircularProgress data-testid="launch-loading-circle" size={20} />
         </Grid>
       )}
+      {startPlanTask.isError && (
+        <Grid item>
+          <Typography className={classes.error} variant="caption">
+            {startPlanTask.message}
+          </Typography>
+        </Grid>
+      )}
     </Grid>
   );
 }

diff --git a/tgnms/fbcnms-projects/tgnms/server/network_plan/__tests__/planning-test.js b/tgnms/fbcnms-projects/tgnms/server/network_plan/__tests__/planning-test.js
@@ -860,12 +860,6 @@ describe('POST plan/:id/launch', () => {
         NETWORK_PLAN_STATE.RUNNING,
       );
     });
-    test.todo(
-      'if file upload fails, transitions the plan into the ERROR state',
-    );
-    test.todo(
-      'if anp api returns errors after local file upload success, transitions the plan into the ERROR state',
-    );
   });
   describe('with only FBID files', () => {
     test('creates and launches plan', async () => {
@@ -921,9 +915,9 @@ describe('POST plan/:id/launch', () => {
         const {body: launchBody} = await request(setupApp())
           .post(`/network_plan/plan/${plan.id}/launch`)
           .expect(500);
-        expect(launchBody.state).toBe(NETWORK_PLAN_STATE.ERROR);
+        expect(launchBody.state).toBe(NETWORK_PLAN_STATE.LAUNCH_ERROR);
         expect(nullthrows(await network_plan.findByPk(plan.id)).state).toBe(
-          NETWORK_PLAN_STATE.ERROR,
+          NETWORK_PLAN_STATE.LAUNCH_ERROR,
         );
       },
     );
@@ -1055,7 +1049,7 @@ describe('POST plan/:id/launch', () => {
         .post(`/network_plan/plan/${plan.id}/launch`)
         .expect(500);
       expect(nullthrows(await network_plan.findByPk(plan.id)).state).toBe(
-        NETWORK_PLAN_STATE.ERROR,
+        NETWORK_PLAN_STATE.LAUNCH_ERROR,
       );
     });
   });

diff --git a/tgnms/fbcnms-projects/tgnms/server/network_plan/planningService.js b/tgnms/fbcnms-projects/tgnms/server/network_plan/planningService.js
@@ -338,7 +338,7 @@ export default class PlanningService {
           );
           await this.waitForFileReadyState(anpFile);
         } catch (err) {
-          console.error(err);
+          throw err;
         } finally {
           if (fd != null) {
             fs.close(fd, err => {
@@ -406,10 +406,15 @@ export default class PlanningService {
         state: NETWORK_PLAN_STATE.RUNNING,
       };
     } catch (err) {
+      const errors = [];
       console.error(err);
-      planRow.state = NETWORK_PLAN_STATE.ERROR;
+      const apiMsg = err.response?.data?.error?.message;
+      if (apiMsg != null) {
+        errors.push(apiMsg);
+      }
+      planRow.state = NETWORK_PLAN_STATE.LAUNCH_ERROR;
       await planRow.save();
-      return {state: planRow.state};
+      return {state: planRow.state, message: err.message, errors};
     }
   }
 
@@ -565,12 +570,24 @@ export default class PlanningService {
     if (fbid == null) {
       throw new Error('Plan has not been launched yet');
     }
-    const inputs = this.anpApi.getPlanInputFiles(fbid);
+    const inputs = await this.anpApi.getPlanInputFiles(fbid);
     return inputs;
   }
 
   // Fetch output files from ANP
   async getPlanOutputFiles(id: number) {
+    const fbid = await this.getPlanFBID(id);
+    const outputs = await this.anpApi.getPlanOutputFiles(fbid);
+    return outputs;
+  }
+
+  async getPlanErrors(id: number): Promise<{error_message: string}> {
+    const fbid = await this.getPlanFBID(id);
+    const errors = await this.anpApi.getPlanErrors(fbid);
+    return errors;
+  }
+
+  async getPlanFBID(id: number): Promise<string> {
     const row = await network_plan.findByPk(id);
     if (row == null) {
       throw new Error(`Plan not found: ${id}`);
@@ -579,8 +596,7 @@ export default class PlanningService {
     if (fbid == null) {
       throw new Error('Plan has not been launched yet');
     }
-    const outputs = this.anpApi.getPlanOutputFiles(fbid);
-    return outputs;
+    return fbid;
   }
 
   async downloadFileStream(id: number) {
@@ -843,23 +859,6 @@ export default class PlanningService {
     }
   }
 
-  async getPlanErrors({
-    id,
-  }: {
-    id: number,
-  }): Promise<Array<{error_message: string}>> {
-    const planRow = await network_plan.findByPk(id);
-    if (planRow == null) {
-      throw new Error('Plan does not exist');
-    }
-    const plan = planRow.toJSON();
-    if (plan.fbid == null || plan.fbid.trim() === '') {
-      return [];
-    }
-    const response = await this.anpApi.getPlanErrors(plan.fbid);
-    return response;
-  }
-
   /**
    * This is called by multer before a file is uploaded, return false or
    * throw an error to prevent the file being uploaded.

diff --git a/tgnms/fbcnms-projects/tgnms/server/network_plan/routes.js b/tgnms/fbcnms-projects/tgnms/server/network_plan/routes.js
@@ -17,7 +17,7 @@ import {
   FACEBOOK_OAUTH_URL,
 } from '../config';
 import {Api} from '../Api';
-import {NETWORK_PLAN_STATE} from '@fbcnms/tg-nms/shared/dto/NetworkPlan';
+import {ERROR_NETWORK_PLAN_STATES} from '@fbcnms/tg-nms/shared/dto/NetworkPlan';
 import {createErrorHandler} from '../helpers/apiHelpers';
 const multer = require('multer');
 
@@ -113,10 +113,7 @@ export default class NetworkPlanRoutes extends Api {
       return planningService
         .startLaunchPlan({id: parseInt(req.params.id)})
         .then(result => {
-          if (result.state === NETWORK_PLAN_STATE.ERROR) {
-            if (result.message) {
-              this.logger.error(result.message);
-            }
+          if (ERROR_NETWORK_PLAN_STATES.has(result.state)) {
             return res.status(500).json(result);
           }
           return res.json(result);
@@ -238,7 +235,7 @@ export default class NetworkPlanRoutes extends Api {
 
     router.get('/plan/:id/errors', (req, res) => {
       return planningService
-        .getPlanErrors({id: parseInt(req.params.id)})
+        .getPlanErrors(parseInt(req.params.id))
         .then(x => res.json(x))
         .catch(err => res.status(500).send(err.message));
     });

diff --git a/tgnms/fbcnms-projects/tgnms/shared/dto/NetworkPlan.js b/tgnms/fbcnms-projects/tgnms/shared/dto/NetworkPlan.js
@@ -71,6 +71,11 @@ export const RUNNING_NETWORK_PLAN_STATES = new Set<string>([
   NETWORK_PLAN_STATE.RUNNING,
 ]);
 
+export const ERROR_NETWORK_PLAN_STATES = new Set<string>([
+  NETWORK_PLAN_STATE.ERROR,
+  NETWORK_PLAN_STATE.LAUNCH_ERROR,
+]);
+
 export type NetworkPlanStateType = $Keys<typeof NETWORK_PLAN_STATE>;
 
 export type PlanFolder = {|