joseluisll commented on code in PR #8766: URL: https://github.com/apache/hadoop/pull/8766#discussion_r4234179906
########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-ui/src/main/webapp/app/utils/diagnostic-archive.js: ########## @@ -0,0 +1,238 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const ISSUE_ID = 'application_diagnostic'; + +export function buildBundleFolderName(generatedAt, appId, issueId) { + const safeTs = generatedAt.replace(/[:.]/g, '-'); + const safeAppId = String(appId).replace(/[^\w.-]+/g, '_'); + const safeIssueId = String(issueId || ISSUE_ID).replace(/[^\w.-]+/g, '_'); + return `${safeAppId}_${safeIssueId}_${safeTs}`; +} + +export function normalizeIssueFiles(issueData) { + if (!issueData || !issueData.file) { + return []; + } + + const files = issueData.file; + return Array.isArray(files) ? files : [files]; +} + +export function resolveEntryFileName(filename, contentType, content) { + + if (contentType.indexOf('xml') >= 0 || content.startsWith('<?xml')) { + return `${filename}.xml`; + } + if (contentType.indexOf('json') >= 0 || content.startsWith('{') || content.startsWith('[')) { + return `${filename}.json`; + } + if (filename.indexOf('log') >= 0) { Review 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: ```js if (/(^|_)log$/.test(filename)) { ``` ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-ui/src/main/webapp/app/controllers/yarn-app/info.js: ########## @@ -19,12 +19,40 @@ import Ember from 'ember'; export default Ember.Controller.extend({ + applicationDiagnostic: Ember.inject.service('application-diagnostic'), + queryParams: ["service"], service: undefined, isLoading: false, actionResponse: null, + isCollectingApplicationDiagnostic: false, + diagnosticCollectError: null, Review 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: ```js 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.) ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-ui/src/main/webapp/app/utils/diagnostic-archive.js: ########## @@ -0,0 +1,238 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const ISSUE_ID = 'application_diagnostic'; + +export function buildBundleFolderName(generatedAt, appId, issueId) { + const safeTs = generatedAt.replace(/[:.]/g, '-'); + const safeAppId = String(appId).replace(/[^\w.-]+/g, '_'); + const safeIssueId = String(issueId || ISSUE_ID).replace(/[^\w.-]+/g, '_'); + return `${safeAppId}_${safeIssueId}_${safeTs}`; +} + +export function normalizeIssueFiles(issueData) { + if (!issueData || !issueData.file) { + return []; + } + + const files = issueData.file; + return Array.isArray(files) ? files : [files]; +} + +export function resolveEntryFileName(filename, contentType, content) { + + if (contentType.indexOf('xml') >= 0 || content.startsWith('<?xml')) { + return `${filename}.xml`; + } + if (contentType.indexOf('json') >= 0 || content.startsWith('{') || content.startsWith('[')) { + return `${filename}.json`; + } + if (filename.indexOf('log') >= 0) { + return `${filename}.log`; + } + + return `${filename}.txt`; +} + +function stringToUtf8Bytes(str) { + if (typeof TextEncoder !== 'undefined') { + return new TextEncoder().encode(str); + } + + return utf8EncodeFallback(str); +} + +function utf8EncodeFallback(str) { + const bytes = []; + for (let i = 0; i < str.length; i++) { + let codePoint = str.charCodeAt(i); + if (codePoint >= 0xd800 && codePoint <= 0xdbff && i + 1 < str.length) { + const next = str.charCodeAt(i + 1); + if (next >= 0xdc00 && next <= 0xdfff) { + codePoint = ((codePoint - 0xd800) << 10) + (next - 0xdc00) + 0x10000; + i++; + } + } + + if (codePoint < 0x80) { + bytes.push(codePoint); + } else if (codePoint < 0x800) { + bytes.push(0xc0 | (codePoint >> 6), 0x80 | (codePoint & 0x3f)); + } else if (codePoint < 0x10000) { + bytes.push( + 0xe0 | (codePoint >> 12), + 0x80 | ((codePoint >> 6) & 0x3f), + 0x80 | (codePoint & 0x3f), + ); + } else { + bytes.push( + 0xf0 | (codePoint >> 18), + 0x80 | ((codePoint >> 12) & 0x3f), + 0x80 | ((codePoint >> 6) & 0x3f), + 0x80 | (codePoint & 0x3f), + ); + } + } + return new Uint8Array(bytes); +} + +const CRC32_TABLE = (function buildCrc32Table() { + const table = new Uint32Array(256); + for (let i = 0; i < 256; i++) { + let c = i; + for (let j = 0; j < 8; j++) { + c = (c & 1) ? (0xedb88320 ^ (c >>> 1)) : (c >>> 1); + } + table[i] = c >>> 0; + } + return table; +}()); + +function crc32(bytes) { + let crc = 0xffffffff; + for (let i = 0; i < bytes.length; i++) { + crc = CRC32_TABLE[(crc ^ bytes[i]) & 0xff] ^ (crc >>> 8); + } + return (crc ^ 0xffffffff) >>> 0; +} + +function writeUint32LE(view, offset, value) { + view.setUint32(offset, value, true); +} + +function writeUint16LE(view, offset, value) { + view.setUint16(offset, value, true); +} + +/** + * Build a ZIP archive (stored, no compression) containing one top-level folder. + */ +export function buildDiagnosticZipBlob(folderName, entries) { + const localParts = []; + const centralParts = []; + let offset = 0; + + entries.forEach((entry) => { + const nameBytes = stringToUtf8Bytes(entry.path); + const dataBytes = entry.data instanceof Uint8Array ? entry.data : stringToUtf8Bytes(entry.data); + const crc = crc32(dataBytes); + + const localHeader = new ArrayBuffer(30 + nameBytes.length); + const localView = new DataView(localHeader); + writeUint32LE(localView, 0, 0x04034b50); + writeUint16LE(localView, 4, 20); + writeUint16LE(localView, 6, 0); + writeUint16LE(localView, 8, 0); + writeUint16LE(localView, 10, 0); + writeUint16LE(localView, 12, 0); + writeUint32LE(localView, 14, crc); + writeUint32LE(localView, 18, dataBytes.length); + writeUint32LE(localView, 22, dataBytes.length); + writeUint16LE(localView, 26, nameBytes.length); + writeUint16LE(localView, 28, 0); + new Uint8Array(localHeader, 30).set(nameBytes); + + localParts.push(new Uint8Array(localHeader), dataBytes); + + const centralHeader = new ArrayBuffer(46 + nameBytes.length); + const centralView = new DataView(centralHeader); + writeUint32LE(centralView, 0, 0x02014b50); + writeUint16LE(centralView, 4, 20); + writeUint16LE(centralView, 6, 20); + writeUint16LE(centralView, 8, 0); + writeUint16LE(centralView, 10, 0); + writeUint16LE(centralView, 12, 0); + writeUint16LE(centralView, 14, 0); + writeUint32LE(centralView, 16, crc); + writeUint32LE(centralView, 20, dataBytes.length); + writeUint32LE(centralView, 24, dataBytes.length); + writeUint16LE(centralView, 28, nameBytes.length); + writeUint16LE(centralView, 30, 0); + writeUint16LE(centralView, 32, 0); + writeUint16LE(centralView, 34, 0); + writeUint16LE(centralView, 36, 0); + writeUint32LE(centralView, 38, 0); + writeUint32LE(centralView, 42, offset); + new Uint8Array(centralHeader, 46).set(nameBytes); + + centralParts.push(new Uint8Array(centralHeader)); + offset += localHeader.byteLength + dataBytes.length; + }); + + const centralSize = centralParts.reduce((sum, part) => sum + part.length, 0); + const endRecord = new ArrayBuffer(22); + const endView = new DataView(endRecord); + writeUint32LE(endView, 0, 0x06054b50); + writeUint16LE(endView, 4, 0); + writeUint16LE(endView, 6, 0); + writeUint16LE(endView, 8, entries.length); + writeUint16LE(endView, 10, entries.length); + writeUint32LE(endView, 12, centralSize); + writeUint32LE(endView, 16, offset); + writeUint16LE(endView, 20, 0); + + const totalLength = localParts.reduce((sum, part) => sum + part.length, 0) + + centralSize + + endRecord.byteLength; + const output = new Uint8Array(totalLength); + let writeOffset = 0; + + localParts.forEach((part) => { + output.set(part, writeOffset); + writeOffset += part.length; + }); + centralParts.forEach((part) => { + output.set(part, writeOffset); + writeOffset += part.length; + }); + output.set(new Uint8Array(endRecord), writeOffset); + + return new Blob([output], { type: 'application/zip' }); +} + +export function buildDiagnosticArchiveEntries(folderName, issueData) { + const entries = []; + const usedNames = {}; + + normalizeIssueFiles(issueData).forEach((fileEntry, index) => { + const content = fileEntry.content != null ? String(fileEntry.content) : ''; + const relativeName = resolveEntryFileName( + fileEntry.filename || `file_${index + 1}`, + fileEntry.contentType, + content, + ); + let zipPath = `${folderName}/${relativeName.replace(/^\/+/, '')}`; Review 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. ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-ui/src/main/webapp/app/templates/yarn-app/info.hbs: ########## @@ -35,6 +35,59 @@ </div> {{/if}} +<div class="col-md-12 margin-bottom-20"> Review Comment: A few things about the panel: - **Permissions.** #8762 calls `initForWritableEndpoints(callerUGI, true)`, so with `yarn.acl.enable=true` only 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.enabled` instructions (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 in `app/styles` (only `margin-bottom-20` is). The inline `style="margin-bottom: …"` attributes could be classes too. ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-ui/src/main/webapp/app/services/application-diagnostic.js: ########## @@ -0,0 +1,107 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import Ember from 'ember'; +import { + buildBundleFolderName, + buildDiagnosticArchiveEntries, + buildDiagnosticZipBlob, +} from '../utils/diagnostic-archive'; + +const ISSUE_ID = 'application_diagnostic'; +const COLLECT_TIMEOUT_MS = 300000; + +export default Ember.Service.extend({ + hosts: Ember.inject.service('hosts'), + env: Ember.inject.service('env'), + + buildCollectUrl(appId) { + const host = this.get('hosts.rmWebAddress'); + const namespace = this.get('env.app.namespaces.cluster'); + const base = `${host}/${namespace}/common-issues/collect`; + const query = `issueId=${encodeURIComponent(ISSUE_ID)}&args=${encodeURIComponent(appId)}`; + return `${base}?${query}`; + }, + + collectApplicationDiagnostic(appId) { + const url = this.buildCollectUrl(appId); + + return new Ember.RSVP.Promise((resolve, reject) => { + Ember.$.ajax({ + url, + method: 'GET', + dataType: 'json', + timeout: COLLECT_TIMEOUT_MS, + crossDomain: true, + xhrFields: { + withCredentials: true + }, + headers: { + Accept: 'application/json' + }, + success: (payload) => resolve(payload), + error: (jqXHR) => reject(this.parseAjaxError(jqXHR)) Review 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: ```js error: (jqXHR, textStatus) => reject(this.parseAjaxError(jqXHR, textStatus)) ``` ```js 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. ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-ui/src/main/webapp/app/utils/diagnostic-archive.js: ########## @@ -0,0 +1,238 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const ISSUE_ID = 'application_diagnostic'; Review 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). ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-ui/src/main/webapp/app/utils/diagnostic-archive.js: ########## @@ -0,0 +1,238 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const ISSUE_ID = 'application_diagnostic'; + +export function buildBundleFolderName(generatedAt, appId, issueId) { + const safeTs = generatedAt.replace(/[:.]/g, '-'); + const safeAppId = String(appId).replace(/[^\w.-]+/g, '_'); + const safeIssueId = String(issueId || ISSUE_ID).replace(/[^\w.-]+/g, '_'); + return `${safeAppId}_${safeIssueId}_${safeTs}`; +} + +export function normalizeIssueFiles(issueData) { + if (!issueData || !issueData.file) { + return []; + } + + const files = issueData.file; + return Array.isArray(files) ? files : [files]; +} + +export function resolveEntryFileName(filename, contentType, content) { + + if (contentType.indexOf('xml') >= 0 || content.startsWith('<?xml')) { + return `${filename}.xml`; + } + if (contentType.indexOf('json') >= 0 || content.startsWith('{') || content.startsWith('[')) { + return `${filename}.json`; + } + if (filename.indexOf('log') >= 0) { + return `${filename}.log`; + } + + return `${filename}.txt`; +} + +function stringToUtf8Bytes(str) { + if (typeof TextEncoder !== 'undefined') { + return new TextEncoder().encode(str); + } + + return utf8EncodeFallback(str); +} + +function utf8EncodeFallback(str) { + const bytes = []; + for (let i = 0; i < str.length; i++) { + let codePoint = str.charCodeAt(i); + if (codePoint >= 0xd800 && codePoint <= 0xdbff && i + 1 < str.length) { + const next = str.charCodeAt(i + 1); + if (next >= 0xdc00 && next <= 0xdfff) { + codePoint = ((codePoint - 0xd800) << 10) + (next - 0xdc00) + 0x10000; + i++; + } + } + + if (codePoint < 0x80) { + bytes.push(codePoint); + } else if (codePoint < 0x800) { + bytes.push(0xc0 | (codePoint >> 6), 0x80 | (codePoint & 0x3f)); + } else if (codePoint < 0x10000) { + bytes.push( + 0xe0 | (codePoint >> 12), + 0x80 | ((codePoint >> 6) & 0x3f), + 0x80 | (codePoint & 0x3f), + ); + } else { + bytes.push( + 0xf0 | (codePoint >> 18), + 0x80 | ((codePoint >> 12) & 0x3f), + 0x80 | ((codePoint >> 6) & 0x3f), + 0x80 | (codePoint & 0x3f), + ); + } + } + return new Uint8Array(bytes); +} + +const CRC32_TABLE = (function buildCrc32Table() { + const table = new Uint32Array(256); + for (let i = 0; i < 256; i++) { + let c = i; + for (let j = 0; j < 8; j++) { + c = (c & 1) ? (0xedb88320 ^ (c >>> 1)) : (c >>> 1); + } + table[i] = c >>> 0; + } + return table; +}()); + +function crc32(bytes) { + let crc = 0xffffffff; + for (let i = 0; i < bytes.length; i++) { + crc = CRC32_TABLE[(crc ^ bytes[i]) & 0xff] ^ (crc >>> 8); + } + return (crc ^ 0xffffffff) >>> 0; +} + +function writeUint32LE(view, offset, value) { + view.setUint32(offset, value, true); +} + +function writeUint16LE(view, offset, value) { + view.setUint16(offset, value, true); +} + +/** + * Build a ZIP archive (stored, no compression) containing one top-level folder. + */ +export function buildDiagnosticZipBlob(folderName, entries) { Review 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 from `generatedAt`: 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ïcode` as `├╝n├»code`). The script's names are ASCII today, so this is minor, but it's a one-line change. - The `folderName` parameter 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`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
