Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OSD Availability] Prevent OSD process crashes when disk is full #6733

Merged
merged 7 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/6733.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- [OSD Availability] Prevent OSD process crashes when disk is full ([#6733](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6733))
5 changes: 5 additions & 0 deletions config/opensearch_dashboards.yml
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@
# Enables you to specify a file where OpenSearch Dashboards stores log output.
#logging.dest: stdout

# This configuration option controls the handling of error messages in the logging stream. It is disabled by default.
# When set to true, the 'ENOSPC' error message will not cause the OpenSearch Dashboards process to crash. Otherwise,
# the original behavior will be maintained.
#logging.ignoreEnospcError: false

# Set the value of this setting to true to suppress all logging output.
#logging.silent: false

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ opensearch_dashboards_vars=(
opensearchDashboards.defaultAppId
opensearchDashboards.index
logging.dest
logging.ignoreEnospcError
logging.json
logging.quiet
logging.rotate.enabled
Expand Down
1 change: 1 addition & 0 deletions src/legacy/server/config/schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export default () =>
}),
events: Joi.any().default({}),
dest: Joi.string().default('stdout'),
ignoreEnospcError: Joi.boolean().default(false),
filter: Joi.any().default({}),
json: Joi.boolean().when('dest', {
is: 'stdout',
Expand Down
1 change: 1 addition & 0 deletions src/legacy/server/logging/configuration.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export default function loggingConfiguration(config) {
json: config.get('logging.json'),
dest: config.get('logging.dest'),
timezone: config.get('logging.timezone'),
ignoreEnospcError: config.get('logging.ignoreEnospcError'),

// I'm adding the default here because if you add another filter
// using the commandline it will remove authorization. I want users
Expand Down
26 changes: 21 additions & 5 deletions src/legacy/server/logging/log_reporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

import { Squeeze } from '@hapi/good-squeeze';
import { createWriteStream as writeStr } from 'fs';
import { pipeline } from 'stream';

import LogFormatJson from './log_format_json';
import LogFormatString from './log_format_string';
Expand All @@ -51,18 +52,33 @@
let dest;
if (config.dest === 'stdout') {
dest = process.stdout;
logInterceptor.pipe(squeeze).pipe(format).pipe(dest);
} else {
dest = writeStr(config.dest, {
flags: 'a',
encoding: 'utf8',
});

logInterceptor.on('end', () => {
dest.end();
});
if (config.ignoreEnospcError) {
pipeline(logInterceptor, squeeze, format, dest, onFinished);

Check warning on line 63 in src/legacy/server/logging/log_reporter.js

View check run for this annotation

Codecov / codecov/patch

src/legacy/server/logging/log_reporter.js#L62-L63

Added lines #L62 - L63 were not covered by tests
Copy link
Member

@kavilla kavilla May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will look into later but probably easier to ask:

does pipeline call dest end automatically? or does onfinished get invoked and then we should check if error.code === 'end' and then call dest.end()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, pipeline() automatically handles closing the streams once the pipeline is finished or if an error occurs during processing. This automatic cleanup ensures that resources associated with the streams are properly released when they are no longer needed. This behavior helps prevent memory leaks and ensures efficient resource usage in Node.js applications

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dang. could do you mind opening an issue? i feel like this legacy stuff needs to go and we updated to use the non legacy, or at least be updated to utilize the non-legacy to modern Node APIs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will open an new issue for it.

} else {
logInterceptor.on('end', () => {
dest.end();

Check warning on line 66 in src/legacy/server/logging/log_reporter.js

View check run for this annotation

Codecov / codecov/patch

src/legacy/server/logging/log_reporter.js#L65-L66

Added lines #L65 - L66 were not covered by tests
});
logInterceptor.pipe(squeeze).pipe(format).pipe(dest);

Check warning on line 68 in src/legacy/server/logging/log_reporter.js

View check run for this annotation

Codecov / codecov/patch

src/legacy/server/logging/log_reporter.js#L68

Added line #L68 was not covered by tests
}
}

logInterceptor.pipe(squeeze).pipe(format).pipe(dest);

return logInterceptor;
}

export function onFinished(error) {
if (error) {
if (error.code === 'ENOSPC') {

Check warning on line 77 in src/legacy/server/logging/log_reporter.js

View check run for this annotation

Codecov / codecov/patch

src/legacy/server/logging/log_reporter.js#L76-L77

Added lines #L76 - L77 were not covered by tests
// eslint-disable-next-line no-console
console.error('Error in logging pipeline:', error.stack);

Check warning on line 79 in src/legacy/server/logging/log_reporter.js

View check run for this annotation

Codecov / codecov/patch

src/legacy/server/logging/log_reporter.js#L79

Added line #L79 was not covered by tests
} else {
throw error;

Check warning on line 81 in src/legacy/server/logging/log_reporter.js

View check run for this annotation

Codecov / codecov/patch

src/legacy/server/logging/log_reporter.js#L81

Added line #L81 was not covered by tests
}
}
}
148 changes: 148 additions & 0 deletions src/legacy/server/logging/log_reporter.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import os from 'os';
import path from 'path';
import fs from 'fs';
import stripAnsi from 'strip-ansi';
import { getLoggerStream, onFinished } from './log_reporter';

const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms));

describe('getLoggerStream', () => {
it('should log to stdout when the json config is set to false', async () => {
const lines = [];
const origWrite = process.stdout.write;
process.stdout.write = (buffer) => {
lines.push(stripAnsi(buffer.toString()).trim());
return true;
};

const loggerStream = getLoggerStream({
config: {
json: false,
dest: 'stdout',
filter: {},
},
events: { log: '*' },
});

loggerStream.end({ event: 'log', tags: ['foo'], data: 'test data' });

await sleep(500);

process.stdout.write = origWrite;
expect(lines.length).toBe(1);
expect(lines[0]).toMatch(/^log \[[^\]]*\] \[foo\] test data$/);
});

it('should log to stdout when the json config is set to true', async () => {
const lines = [];
const origWrite = process.stdout.write;
process.stdout.write = (buffer) => {
lines.push(JSON.parse(buffer.toString().trim()));
return true;
};

const loggerStream = getLoggerStream({
config: {
json: true,
dest: 'stdout',
filter: {},
},
events: { log: '*' },
});

loggerStream.end({ event: 'log', tags: ['foo'], data: 'test data' });

await sleep(500);

process.stdout.write = origWrite;
expect(lines.length).toBe(1);
expect(lines[0]).toMatchObject({
type: 'log',
tags: ['foo'],
message: 'test data',
});
});

it('should log to custom file when the json config is set to false', async () => {
const dir = os.tmpdir();
const logfile = `dest-${Date.now()}.log`;
const dest = path.join(dir, logfile);

const loggerStream = getLoggerStream({
config: {
json: false,
dest,
filter: {},
},
events: { log: '*' },
});

loggerStream.end({ event: 'log', tags: ['foo'], data: 'test data' });

await sleep(500);

const lines = stripAnsi(fs.readFileSync(dest, { encoding: 'utf8' }))
.trim()
.split(os.EOL);
expect(lines.length).toBe(1);
expect(lines[0]).toMatch(/^log \[[^\]]*\] \[foo\] test data$/);
});

it('should log to custom file when the json config is set to true and ignoreEnospcError', async () => {
const dir = os.tmpdir();
const logfile = `dest-${Date.now()}.log`;
const dest = path.join(dir, logfile);

const loggerStream = getLoggerStream({
config: {
json: true,
dest,
ignoreEnospcError: true,
filter: {},
},
events: { log: '*' },
});

loggerStream.end({ event: 'log', tags: ['foo'], data: 'test data' });

await sleep(500);

const lines = fs
.readFileSync(dest, { encoding: 'utf8' })
.trim()
.split(os.EOL)
.map((data) => JSON.parse(data));
expect(lines.length).toBe(1);
expect(lines[0]).toMatchObject({
type: 'log',
tags: ['foo'],
message: 'test data',
});
});

it('should handle ENOSPC error when disk full', () => {
const error = { code: 'ENOSPC', stack: 'Error stack trace' };
const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();

expect(() => {
onFinished(error);
}).not.toThrow();

expect(consoleErrorSpy).toHaveBeenCalledWith('Error in logging pipeline:', 'Error stack trace');

consoleErrorSpy.mockRestore();
});

it('should throw error for non-ENOSPC error', () => {
const error = { message: 'non-ENOSPC error', code: 'OTHER', stack: 'Error stack trace' };

expect(() => {
onFinished(error);
}).toThrowError('non-ENOSPC error');
});
});
Loading