Fix bug: Get wrong log for retried task & frontend crash (#5177) (#5190)

When calling getTaskAttempts API. The task attempt list returned and sorted by DESC order of attemptId. Previous code treat the list as ASC order and return wrong log content for retried task. Fix this issue, as well as frontend bug.
microsoft · Dec 23, 2020 · f04d999 · f04d999
1 parent 15990fc
commit f04d999
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 56 deletions.
diff --git a/src/rest-server/src/controllers/v2/job.js b/src/rest-server/src/controllers/v2/job.js
@@ -302,7 +302,7 @@ const getLogs = asyncHandler(async (req, res) => {
     res.json(data);
   } catch (error) {
     logger.error(`Got error when retrieving log list, error: ${error}`);
-    throw error.code === 'NoTaskLogErr'
+    throw error.code === 'NoTaskLogError'
       ? error
       : createError(
           'Internal Server Error',

diff --git a/src/rest-server/src/models/v2/job/log.js b/src/rest-server/src/models/v2/job/log.js
@@ -58,9 +58,11 @@ const getLogListFromLogManager = async (
     'NoTaskLogError',
     `Log of task is not found.`,
   );
-  const taskStatus = taskDetail.data.attempts[Number(taskAttemptId)];
-  if (!taskStatus) {
-    logger.error(`Failed to find task to retrive log`);
+  const taskStatus = taskDetail.data.attempts.find(
+    (attempt) => attempt.attemptId === Number(taskAttemptId),
+  );
+  if (!taskStatus || !taskStatus.containerIp) {
+    logger.error(`Failed to find task to retrive log or task not started`);
     throw NoTaskLogErr;
   }
 

diff --git a/src/webportal/src/app/job/job-view/fabric/job-detail/components/task-role-container-list.jsx b/src/webportal/src/app/job/job-view/fabric/job-detail/components/task-role-container-list.jsx
@@ -134,30 +134,25 @@ PortTooltipContent.propTypes = {
   ports: PropTypes.object,
 };
 
-const LogDialogContent = ({ urlLists }) => {
-  const lists = [];
-  for (const p of urlLists) {
-    lists.push(p);
-  }
-  if (lists.length === 0) {
+const LogDialogContent = ({ urls }) => {
+  if (isEmpty(urls)) {
     return <Stack>No log file generated or log files be rotated</Stack>;
   }
-  const urlpairs = lists.map((lists, index) => (
-    <Stack key={`log-list-${index}`}>
-      <Link
-        href={lists.uri}
-        target='_blank'
-        styles={{ root: [FontClassNames.mediumPlus] }}
-      >
-        <Icon iconName='TextDocument'></Icon> {lists.name}
-      </Link>
-    </Stack>
-  ));
-  return urlpairs;
+  const urlPairs = [];
+  for (const [key, url] of Object.entries(urls)) {
+    urlPairs.push(
+      <Stack key={`log-list-${key}`}>
+        <Link href={url} styles={{ root: [FontClassNames.mediumPlus] }}>
+          <Icon iconName='TextDocument'></Icon> {key}
+        </Link>
+      </Stack>,
+    );
+  }
+  return urlPairs;
 };
 
 LogDialogContent.propTypes = {
-  urlLists: PropTypes.array,
+  urlLists: PropTypes.object,
 };
 
 export default class TaskRoleContainerList extends React.Component {
@@ -410,10 +405,15 @@ export default class TaskRoleContainerList extends React.Component {
       .then(({ fullLogUrls, _ }) => {
         this.setState({
           hideAllLogsDialog: !hideAllLogsDialog,
-          fullLogUrls: fullLogUrls,
+          fullLogUrls: this.convertObjectFormat(fullLogUrls),
         });
       })
-      .catch(() => this.setState({ hideAllLogsDialog: !hideAllLogsDialog }));
+      .catch(() =>
+        this.setState({
+          hideAllLogsDialog: !hideAllLogsDialog,
+          fullLogUrls: {},
+        }),
+      );
   }
 
   getTaskPropertyFromColumnKey(item, key) {
@@ -531,13 +531,7 @@ export default class TaskRoleContainerList extends React.Component {
         >
           <Stack gap='m'>
             <Text variant='xLarge'>All Logs:</Text>
-            <LogDialogContent
-              urlLists={
-                !isNil(fullLogUrls) && !isNil(fullLogUrls.locations)
-                  ? fullLogUrls.locations
-                  : []
-              }
-            />
+            <LogDialogContent urls={fullLogUrls} />
           </Stack>
           <DialogFooter>
             <PrimaryButton

diff --git a/src/webportal/src/app/job/job-view/fabric/task-attempt/task-attempt-list.jsx b/src/webportal/src/app/job/job-view/fabric/task-attempt/task-attempt-list.jsx
@@ -93,30 +93,25 @@ PortTooltipContent.propTypes = {
   ports: PropTypes.object,
 };
 
-const LogDialogContent = ({ urlLists }) => {
-  const lists = [];
-  for (const p of urlLists) {
-    lists.push(p);
-  }
-  if (lists.length === 0) {
+const LogDialogContent = ({ urls }) => {
+  if (isEmpty(urls)) {
     return <Stack>No log file generated or log files be rotated</Stack>;
   }
-  const urlpairs = lists.map((lists, index) => (
-    <Stack key={`log-list-${index}`}>
-      <Link
-        href={lists.uri}
-        target='_blank'
-        styles={{ root: [FontClassNames.mediumPlus] }}
-      >
-        <Icon iconName='TextDocument'></Icon> {lists.name}
-      </Link>
-    </Stack>
-  ));
-  return urlpairs;
+  const urlPairs = [];
+  for (const [key, url] of Object.entries(urls)) {
+    urlPairs.push(
+      <Stack key={`log-list-${key}`}>
+        <Link href={url} styles={{ root: [FontClassNames.mediumPlus] }}>
+          <Icon iconName='TextDocument'></Icon> {key}
+        </Link>
+      </Stack>,
+    );
+  }
+  return urlPairs;
 };
 
 LogDialogContent.propTypes = {
-  urlLists: PropTypes.array,
+  urlLists: PropTypes.object,
 };
 
 export default class TaskAttemptList extends React.Component {
@@ -223,10 +218,15 @@ export default class TaskAttemptList extends React.Component {
       .then(({ fullLogUrls, _ }) => {
         this.setState({
           hideAllLogsDialog: !hideAllLogsDialog,
-          fullLogUrls: fullLogUrls,
+          fullLogUrls: this.convertObjectFormat(fullLogUrls),
         });
       })
-      .catch(() => this.setState({ hideAllLogsDialog: !hideAllLogsDialog }));
+      .catch(() =>
+        this.setState({
+          hideAllLogsDialog: !hideAllLogsDialog,
+          fullLogUrls: {},
+        }),
+      );
   }
 
   showContainerTailLog(logListUrl, logType) {
@@ -432,9 +432,7 @@ export default class TaskAttemptList extends React.Component {
         >
           <Stack gap='m'>
             <Text variant='xLarge'>All Logs:</Text>
-            <LogDialogContent
-              urlLists={!isNil(fullLogUrls) ? fullLogUrls.locations : []}
-            />
+            <LogDialogContent urls={fullLogUrls} />
           </Stack>
           <DialogFooter>
             <PrimaryButton