Repository navigation
YARN-10423. Extend UIv2 to download application diagnostic bundle as a zip folder with each file name - #8766
Hean-Chhinling wants to merge 2 commits into
Conversation
…a zip folder with each file name
|
🎊 +1 overall
This message was automatically generated. |
joseluisll
left a comment
joseluisll
left a comment
There was a problem hiding this comment.
Thanks for picking this back up, @Hean-Chhinling. A one-click diagnostic bundle in UI2 will be useful. Some general points first, then inline comments.
Dependencies. This calls /common-issues/collect from #8762, which runs the collector script from #8742 (YARN-10422). The script calls the NodeManager endpoints ws/v1/node/apps/{appId}/jstack/{n} and ws/v1/node/jstack/{n}, and the panel text refers to yarn.nodemanager.jstack-endpoints.enabled. I couldn't find either the endpoints or the property in trunk, #8762 or #8742. Which PR/JIRA adds them? Could you list all the dependencies in the description, so it's clear this can only go in after them?
Scope vs. the JIRA. YARN-10423 asks for a UI2 page that lists the diagnostic cases defined by the collector script (i.e. /common-issues/list). This PR hard-codes the application_diagnostic case on the app info page. That's a reasonable first step, but please either update the JIRA description or move this to its own sub-task of YARN-10323, so the JIRA matches what gets merged.
Where the archive is built. As #8762 stands, the RM reads every output file into a String (DiagnosticsService.collectIssueFilesContent) and returns everything in one JSON response. The browser then parses it and builds the ZIP in memory. With large aggregated logs (yarn logs -applicationId), that means several full copies on both the RM and the client. Have you considered having the endpoint stream application/zip directly (StreamingOutput + ZipOutputStream)? The UI would then be a plain download, and the hand-written ZIP/CRC32 code here would no longer be needed.
HTTP method. Collecting is expensive and has side effects (it runs yarn logs and takes jstacks on the NodeManager), but it's exposed as GET. RestCsrfPreventionFilter skips GET,OPTIONS,HEAD,TRACE by default, so when yarn.resourcemanager.webapp.rest-csrf.enabled is turned on this endpoint still isn't protected. RMWebServices already uses @POST for a comparable admin action (/scheduler-logs). I'd suggest POST in #8762 and in this PR.
Tests. Maven/Yetus don't run the Ember unit tests, so the +1 doesn't cover them. I ran tests/unit/utils/diagnostic-archive-test.js against app/utils/diagnostic-archive.js, and resolveEntryFileName adds txt extension for non-xml payload fails: it returns application_logs.log, but the test expects application_logs.txt. Details inline. Could you run ember test locally before the next push?
Housekeeping.
- Hadoop's title convention is
YARN-10423. <summary>(period, not colon). - The ASF generative-tooling checkbox ("My use of AI contributions follows the ASF legal policy") in the description is unchecked.
| if (contentType.indexOf('json') >= 0 || content.startsWith('{') || content.startsWith('[')) { | ||
| return `${filename}.json`; | ||
| } | ||
| if (filename.indexOf('log') >= 0) { |
There was a problem hiding this comment.
This substring check matches "log" anywhere in the name, which is why the PR's own test resolveEntryFileName adds txt extension for non-xml payload fails: application_logs → application_logs.log, expected .txt. It would also match names like catalog or dialog. The script names its log files resourcemanager_log and nodemanager_log, so matching the suffix would be enough:
if (/(^|_)log$/.test(filename)) {There was a problem hiding this comment.
Nice catch, I will update this in my next commit. I thought the unit-test was failing because I have not updated the unit-test since I changed the file extension before from .log to .txt.
| fileEntry.contentType, | ||
| content, | ||
| ); | ||
| let zipPath = `${folderName}/${relativeName.replace(/^\/+/, '')}`; |
There was a problem hiding this comment.
Entry names aren't sanitised, but the folder name is (buildBundleFolderName, line 23). The collector script in #8742 writes the NodeManager jstack as nm_{amHostHttpAddress}_jstack, so the entry becomes e.g. …/nm_host1.example.com:8042_jstack.txt. Windows can't extract a file with : in its name. Could you apply the same /[^\w.-]+/g → '_' replacement to each path segment of relativeName? A unit test with a host:port filename would cover it.
There was a problem hiding this comment.
Nice catch, I will apply to /[^\w.-]+/g → '_' to each of the segment of relativeName.
When I was tested on Mac, it all works as expected. Thank you for your suggestions!
| Accept: 'application/json' | ||
| }, | ||
| success: (payload) => resolve(payload), | ||
| error: (jqXHR) => reject(this.parseAjaxError(jqXHR)) |
There was a problem hiding this comment.
When the 5-minute timeout fires, jQuery 3.6 calls jqXHR.abort("timeout"), which sets jqXHR.status to 0. parseAjaxError then reaches the status === 0 branch (line 82) and shows "Not able to connect to the Resource Manager.". That's misleading for an operation the panel says may take several minutes. The error callback also receives textStatus, so a timeout can be reported separately:
error: (jqXHR, textStatus) => reject(this.parseAjaxError(jqXHR, textStatus)) parseAjaxError(jqXHR, textStatus) {
if (textStatus === 'timeout') {
return new Error('Diagnostic collection did not finish within 5 minutes. ' +
'It may still be running on the Resource Manager; check the RM logs.');
}
...Please add a unit test for that case next to the existing parseAjaxError ones.
There was a problem hiding this comment.
You have a very good eye, @joseluisll!
I will update this in my next commit along with a unit-test to make sure the timeout shows different error.
| isCollectingApplicationDiagnostic: false, | ||
| diagnosticCollectError: null, |
There was a problem hiding this comment.
Controllers are singletons in Ember 2.8, and neither routes/yarn-app/info.js nor routes/abstract.js resets these properties. So if you start a collection for app A and open app B's info page, B shows A's spinner with the button disabled, and an error from A stays visible on B. Resetting them in the route's setupController (or storing the state per appId) would fix it:
setupController(controller, model) {
this._super(controller, model);
controller.setProperties({
isCollectingApplicationDiagnostic: false,
diagnosticCollectError: null
});
},(The in-flight request's finally still clears the flag later, which is harmless.)
There was a problem hiding this comment.
Thank you for the suggestion! It seems like the in-flight request's finally could cause a problem.
If a collection on app B has started while A is still going. Then A's finally will clear B's spinner and re-enabled the app's B button while it is still collecting. This is also appA error handler will show on appB page.
I will fix on of this in my next commit with by giving id to the app request and only update the page if the id is the latest.
| </div> | ||
| {{/if}} | ||
|
|
||
| <div class="col-md-12 margin-bottom-20"> |
There was a problem hiding this comment.
A few things about the panel:
- Permissions. YARN-10421: Adding controller for handling diagnostic python script #8762 calls
initForWritableEndpoints(callerUGI, true), so withyarn.acl.enable=trueonly YARN admins can run this. Everyone else gets a 403 after clicking. Could the panel say "Requires YARN admin privileges when ACLs are enabled", and could a 403 be shown with that message? - Placement. The panel goes above the app's own Diagnostics block, which pushes the failure diagnostics users usually look for further down. Putting it below that block, or making it collapsible, would avoid that.
- Hard-coded description. The bullet list and the
yarn.nodemanager.jstack-endpoints.enabledinstructions (lines 44-61) describe what the script happens to do today, and will drift from it (the property doesn't exist in trunk yet either). A short general sentence plus a link to the docs (YARN-10461) would be easier to maintain. - Styles.
margin-left-15(line 75) isn't defined inapp/styles(onlymargin-bottom-20is). The inlinestyle="margin-bottom: …"attributes could be classes too.
There was a problem hiding this comment.
Thank you for the suggestion, @joseluisll!
These are very good points. My bad that I miss the important permission check here.
I will make these necessary changes in my next commit.
| /** | ||
| * Build a ZIP archive (stored, no compression) containing one top-level folder. | ||
| */ | ||
| export function buildDiagnosticZipBlob(folderName, entries) { |
There was a problem hiding this comment.
I opened the output with unzip -t and Python's zipfile. The archive is valid, but a few header fields are worth fixing:
- The modification date and time are 0, so every entry is listed as
1980-00-00 00:00, which is not a valid DOS date. Please write the DOS date/time fromgeneratedAt: local header offsets 10 and 12, central directory offsets 12 and 14. - Names are encoded as UTF-8, but general-purpose bit 11 (
0x0800) isn't set (local header offset 6, central directory offset 8). Readers that follow the spec decode non-ASCII names as CP437 (Python showsünïcodeas├╝n├»code). The script's names are ASCII today, so this is minor, but it's a one-line change. - The
folderNameparameter isn't used, and the doc comment says "containing one top-level folder" although no directory entry is written.
The only test today is blob.size > 0. Could you add one that checks the bytes? For example: CRC-32 of "hello" is 0x3610a686, and the end-of-central-directory entry count and central directory offset are easy to assert. Please also include a non-ASCII case: PhantomJS (the testem.json launcher) has no TextEncoder, so the tests exercise utf8EncodeFallback.
There was a problem hiding this comment.
Thank you for your detailed feedback, I really appreciate it.
My bad that I missed these points. I will fix the code in my next commit along with the unit-tests.
| * limitations under the License. | ||
| */ | ||
|
|
||
| const ISSUE_ID = 'application_diagnostic'; |
There was a problem hiding this comment.
Nit: this duplicates ISSUE_ID in app/services/application-diagnostic.js (line 26). The service already passes it to buildBundleFolderName, so the util could drop its own copy (or export one that the service imports).
There was a problem hiding this comment.
Thanks, I will centralise this ISSUE_ID and pass it to application-diagnostic.js.
|
Thank you so much @joseluisll for your detailed feedback! I really appreciate it. Let me address your overall review below and then I will answer in each of the review thread:
Exactly, that is how this new feature will work. The new jstack endpoint is on this PR #8123. I had already added it in the PR description that it must go in before the UI changes.
Good catch! When the doc was prepared there were 4 endpoints for diagnostic collector. However, after several round of discussions I narrowed down into only application_diagnostic option that is needed to be called from UIv2. I will change the jira description and probably remove the
Yep, I have thought about that duplication but have not found anything useful yet. Thank you so much for suggestion, I will research more about this. Having the backend endpoint send as a zip would be significantly easier than the UI doing it.
Thank you so much for the suggestion! I will change the collect endpoint to POST method.
Nice catch, I will make sure to run Ember unit-tests locally next time before the push. Regarding this failure
And thank you so much for letting me know this housekeeping rules. I was not familiar about this before. Please let me know if something is not clear to you. I would be more than happy to address the issue further :) |
Hean-Chhinling
changed the title
|
🎊 +1 overall
This message was automatically generated. |
Contains content generated by Cursor.
Description of PR
This PR is part of the YARN-10323 [Umbrella] YARN Diagnostic collector.
Its aims is to add a button in the YARN UIv2 for calling the Application Diagnostic endpoint from #8762.
Below is the screenshot:
It takes a couple mins to run and after a ZIP folder will be downloaded. Each of the diagnostic artifacts will be in its own separate file.
There is a ZIP utility written from scratch at diagnostic-archive.js to avoid installing new dependency.
This PR depends on the following PRs:
How was this patch tested?
Tested manually and wrote unit-test for utility function. They all work as expected.
For code changes:
declared according to the connector-specific documentation? Note: Automated CI
testing doesn't cover all cases so manual testing with cloud storage is still
required.
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
If an AI tool was used:
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html