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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
change feature flag name
Signed-off-by: Flyingliuhub <[email protected]>
Flyingliuhub committed May 7, 2024
commit 893f6dbceec0d14ced22260397fa955eeb7c295f
2 changes: 1 addition & 1 deletion config/opensearch_dashboards.yml
Original file line number Diff line number Diff line change
@@ -138,7 +138,7 @@
# 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.streamErrorHandling: false
#logging.ignoreEnospcError: false

# Set the value of this setting to true to suppress all logging output.
#logging.silent: false
Original file line number Diff line number Diff line change
@@ -58,6 +58,7 @@ opensearch_dashboards_vars=(
opensearchDashboards.defaultAppId
opensearchDashboards.index
logging.dest
logging.ignoreEnospcError
logging.json
logging.quiet
logging.rotate.enabled
2 changes: 1 addition & 1 deletion src/legacy/server/config/schema.js
Original file line number Diff line number Diff line change
@@ -109,7 +109,7 @@ export default () =>
}),
events: Joi.any().default({}),
dest: Joi.string().default('stdout'),
streamErrorHandling: Joi.boolean().default(false),
ignoreEnospcError: Joi.boolean().default(false),
filter: Joi.any().default({}),
json: Joi.boolean().when('dest', {
is: 'stdout',
2 changes: 1 addition & 1 deletion src/legacy/server/logging/configuration.js
Original file line number Diff line number Diff line change
@@ -64,7 +64,7 @@ export default function loggingConfiguration(config) {
json: config.get('logging.json'),
dest: config.get('logging.dest'),
timezone: config.get('logging.timezone'),
streamErrorHandling: config.get('logging.streamErrorHandling'),
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
2 changes: 1 addition & 1 deletion src/legacy/server/logging/log_reporter.js
Original file line number Diff line number Diff line change
@@ -59,13 +59,13 @@
encoding: 'utf8',
});

if (config.streamErrorHandling) {
if (config.ignoreEnospcError) {
pipeline(logInterceptor, squeeze, format, dest, onFinished);

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

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

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

Codecov / codecov/patch

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

Added line #L68 was not covered by tests
}
}

@@ -73,12 +73,12 @@
}

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

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

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

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

Codecov / codecov/patch

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

Added line #L81 was not covered by tests
}
}
}
4 changes: 2 additions & 2 deletions src/legacy/server/logging/log_reporter.test.js
Original file line number Diff line number Diff line change
@@ -93,7 +93,7 @@ describe('getLoggerStream', () => {
expect(lines[0]).toMatch(/^log \[[^\]]*\] \[foo\] test data$/);
});

it('should log to custom file when the json config is set to true and streamErrorHandling', async () => {
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);
@@ -102,7 +102,7 @@ describe('getLoggerStream', () => {
config: {
json: true,
dest,
streamErrorHandling: true,
ignoreEnospcError: true,
filter: {},
},
events: { log: '*' },