Add Settings#validate_all. #6008

jordansissel · 2016-10-07T00:34:48Z

All setting validations are now deferred until after setting sources have been processed (flags, logstash.yml, etc)

Deferred validation fixes #6004.

jordansissel · 2016-10-07T00:36:20Z

Testing that #6004 is fixed:

master, fails:

% chmod 0 ./data
% bin/logstash --debug -e '' --path.data /tmp/example
ArgumentError: Path "/home/jls/projects/logstash/data" is not a directory or not writable.

this PR, succeeds:

% chmod 0 ./data
% bin/logstash --debug -e '' --path.data /tmp/example
...
[2016-10-06T15:57:13,378][DEBUG][logstash.runner          ] *path.data: "/tmp/example" (default: "/home/jls/projects/logstash/data")
...

jordansissel · 2016-10-07T00:40:10Z

I wanted to do a larger refactor, but my goal was to do a minimal change in time for 5.0.0.

This PR brings in some changes already in the feature/java_persistence branch (adds Setting::Bytes, improves WritableDirectory). Bytes wasn't really needed, but was already in the patch I imported, and isn't used yet, so I felt was safe to include in this PR.

untergeek

LGTM™

jsvd

example of failing spec due to nil -> "" change:

  65) LogStash::Agent register_pipeline should delegate settings to new pipeline
     Failure/Error: expect(arg1).to eq(config_string)

       expected: "input { } filter { } output { }"
            got: "input { } filter { } output { }## Note: this changelog has been moved to markdown format and is available at https://github.com/elastic/logstash/blob/master/CHANGELOG.md\n## 1.5.5 (Oct 29, 2015)\n### general\n - Update to JRuby 1.7.22\n - Improved default security configuration for SSL/TLS. Default is now TLS1.2 (#3955)\n - Fixed bug in JrJackson v0.3.5 when handing shared strings. This manifested into issues when \n   JrJackson was used in json codec and ES output. (#4048, #4055\n - Added beats input in the default plugins list\n\n ## output\n - HTTP: Fixed memory leak in http output with usage of manticore library (#24) \n\n## 2.0.0 (Oct 28, 2015)\nNo additional changes from RC1 release. Please see below for changes in individual\npre-releases.\n\n## 2.0.0-rc1 (October 22, 2015)\n### filter\n

There are many test failures, coming from the changes in this PR, though I'm not sure why.

Also, a minor note, I see some whitespace ended at the end of some of the lines. Before merging it would be nice to remove it :)

jsvd · 2016-10-07T10:53:55Z

logstash-core/lib/logstash/environment.rb

@@ -17,9 +17,9 @@ module Environment

  [
            Setting::String.new("node.name", Socket.gethostname),
-            Setting::String.new("path.config", nil, false),
+            Setting::String.new("path.config", "", false),


there is a somewhat big consequence in changing this from nil to "", as now logstash will try to read all files in the local directory, see here

Is this because validate_all will fail if path.config, when disabled, is set to nil?

jsvd · 2016-10-07T11:02:18Z

logstash-core/lib/logstash/settings.rb

+      def value
+        super.tap do |path|
+          if !::File.directory?(path)
+            # Create the directory if it doesn't exist.


what is the use case for this implicit directory creation?
Not sure how strict we should be about this, IMO we should just check if it's there and it's writable, if not, throw an error, and leave the dir creation to the provisioning tools, thoughts?

also, this could prevent "silent" directory creating (I know dir creating action is logged but stillll.....).

Two use cases. First, for the PQ that's coming -- the default path si probably going to be {path.data}/queue/... and it'd be nice not to have to have all these empty directories. Second, for users who just want to specify {path.data} and that would imply the queue path (or similar) would be relative to {path.data} and would be nice if they were automatically created if possible. My hope is to avoid having our docs say Run these 7 mkdir commands before running logstash.

I've ejected the WritableDirectory changes into a new PR, #6023

jsvd · 2016-10-07T11:04:24Z

logstash-core/lib/logstash/settings.rb

+        when ::String
+          LogStash::Util::ByteValue.parse(value)
+        else
+          raise ArgumentError.new("Could not coerce '#{value}' into a bytes value")


I'd suggest also logging the value's class

jordansissel · 2016-10-07T15:09:55Z

Hmm yeah I saw lots of Travis failures last night. I will attend to that
when I am back at work. ❤️

On Friday, October 7, 2016, João Duarte notifications@github.com wrote:

@jsvd requested changes on this pull request.

example of failing spec due to nil -> "" change:

LogStash::Agent register_pipeline should delegate settings to new pipeline
Failure/Error: expect(arg1).to eq(config_string)
   expected: "input { } filter { } output { }"
        got: "input { } filter { } output { }## Note: this changelog has been moved to markdown format and is available at https://github.com/elastic/logstash/blob/master/CHANGELOG.md\n## 1.5.5 (Oct 29, 2015)\n### general\n - Update to JRuby 1.7.22\n - Improved default security configuration for SSL/TLS. Default is now TLS1.2 (#3955)\n - Fixed bug in JrJackson v0.3.5 when handing shared strings. This manifested into issues when \n   JrJackson was used in json codec and ES output. (#4048, #4055\n - Added beats input in the default plugins list\n\n ## output\n - HTTP: Fixed memory leak in http output with usage of manticore library (#24) \n\n## 2.0.0 (Oct 28, 2015)\nNo additional changes from RC1 release. Please see below for changes in individual\npre-releases.\n\n## 2.0.0-rc1 (October 22, 2015)\n### filter\n
There are many test failures, coming from the changes in this PR, though
I'm not sure why.

Also, a minor note, I see some whitespace ended at the end of some of the

lines. Before merging it would be nice to remove it :)

In logstash-core/lib/logstash/environment.rb
#6008 (review):

@@ -17,9 +17,9 @@ module Environment

[
Setting::String.new("node.name", Socket.gethostname),
       Setting::String.new("path.config", nil, false),
       Setting::String.new("path.config", "", false),
there is a somewhat big consequence in changing this from nil to "", as
now logstash will try to read all files in the local directory, see here
https://github.com/elastic/logstash/blob/master/logstash-core/lib/logstash/config/loader.rb#L31-L53

Is this because validate_all will fail if path.config, when disabled, is

set to nil?

In logstash-core/lib/logstash/settings.rb
#6008 (review):
       end
     end
   # If we get here, the directory exists and is writable.
   true
 end
 def value
   super.tap do |path|
     if !::File.directory?(path)
       # Create the directory if it doesn't exist.
what is the use case for this implicit directory creation?
Not sure how strict we should be about this, IMO we should just check if
it's there and it's writable, if not, throw an error, and leave the dir
creation to the provisioning tools, thoughts?

also, this could prevent "silent" directory creating (I know dir creating

action is logged but stillll.....).

In logstash-core/lib/logstash/settings.rb
#6008 (review):
 def initialize(name, default=nil, strict=true)
   super(name, ::Fixnum, default, strict=true) { |value| valid?(value) }
 end
 def valid?(value)
   value.is_a?(Fixnum) && value >= 0
 end
 def coerce(value)
   case value
   when ::Numeric
     value
   when ::String
     LogStash::Util::ByteValue.parse(value)
   else
     raise ArgumentError.new("Could not coerce '#{value}' into a bytes value")
I'd suggest also logging the value's class

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#6008 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIC6oh6YXmpkeNQ_Z6GnbShPKsVBLqTks5qxiqXgaJpZM4KQjrG
.

jordansissel · 2016-10-10T06:56:52Z

Switched the empty-default string settings with a newer NullableString. I'm not happy with this as a solution, but I think as a short term minimal fix it is ok.

There's still two more failing tests. I will get to these shortly.

jsvd · 2016-10-10T09:10:22Z

what if we added a set? predicate to know if a setting was ever .set(..) ? this way we wouldn't rely on the value to distinguish between an nil/""/etc?

This allows us to validate all settings after all the settings sources have been processed (logstash.yml, flags, environment variables, etc) NullableString is required for validation to pass on what were previously String settings with nil defaults. WritableDirectory's strict now defaults false to help with a problem where the default path.data might not be writable *and* the user could be specifying --path.data on the command line to compensate. Prior to this, the default value would be validated and cause Logstash to terminate on startup because of the default data directory was validated before the flag override was applied. To make this validate_all feature more useful, Setting#set will only call validate if `strict` is true. Fixes #6004

suyograo · 2016-10-11T00:34:33Z

logstash-core/lib/logstash/settings.rb

+            # Create the directory if it doesn't exist.
+            begin
+              # TODO(sissel): Log the fact that we are creating this directory.
+              ::FileUtils.mkdir_p(path)


@jordansissel can we log (debug?) that we created this dir?

ooh just saw your todo. my bad :)

This may be a larger change since Settings doesn't have access to a logger right now.

but yes, I agree.

jordansissel · 2016-10-11T00:57:18Z

I did some surgery to this PR. Changes:

This change only introduces validate_all and NullableString for Logstash checks default directory for write permissions always, even when overriden #6004
Moved Setting::Bytes addition to Add Bytes setting type #6022
Moved WritableDirectory improvements to WritableDirectory#value now creates the directory if needed. #6023

jordansissel · 2016-10-11T01:08:02Z

Manual test % bin/logstash --debug -e '' --path.data /tmp/example is now passing on this PR once again. Woo.

jordansissel · 2016-10-11T01:08:08Z

@jsvd can you re-review?

jsvd · 2016-10-11T09:12:37Z

any thoughts on my suggestion in #6008 (comment)?

Otherwise LGTM (test failure is unrelated)

jordansissel · 2016-10-11T20:21:14Z

@jsvd Yeah I think adding #set? could be useful, though I wonder about the blurred line between a 'set' and 'default' value when it comes to validation. We can work on that later. <3

elasticsearch-bot · 2016-10-11T20:21:36Z

Jordan Sissel merged this into the following branches!

Branch	Commits
master	`e98b1c6`
5.x	`6dd2aa6`
5.0	`f281538`

This allows us to validate all settings after all the settings sources have been processed (logstash.yml, flags, environment variables, etc) NullableString is required for validation to pass on what were previously String settings with nil defaults. WritableDirectory's strict now defaults false to help with a problem where the default path.data might not be writable *and* the user could be specifying --path.data on the command line to compensate. Prior to this, the default value would be validated and cause Logstash to terminate on startup because of the default data directory was validated before the flag override was applied. To make this validate_all feature more useful, Setting#set will only call validate if `strict` is true. Fixes #6004 Fixes #6008

jordansissel assigned jordansissel and jsvd Oct 7, 2016

untergeek approved these changes Oct 7, 2016

View reviewed changes

jsvd requested changes Oct 7, 2016

View reviewed changes

suyograo added the v5.0.0 label Oct 10, 2016

jordansissel added the v5.1.0 label Oct 10, 2016

jordansissel force-pushed the usability/lazy-settings-validation branch 2 times, most recently from dcd5486 to fd780b8 Compare October 11, 2016 00:26

suyograo reviewed Oct 11, 2016

View reviewed changes

jordansissel force-pushed the usability/lazy-settings-validation branch 3 times, most recently from 824d376 to 255c108 Compare October 11, 2016 00:54

jordansissel changed the title ~~Several improvements to Settings~~ Add Settings#validate_all. Oct 11, 2016

jordansissel force-pushed the usability/lazy-settings-validation branch from 255c108 to 07018fb Compare October 11, 2016 01:07

jsvd approved these changes Oct 11, 2016

View reviewed changes

elasticsearch-bot closed this in e98b1c6 Oct 11, 2016

jordansissel removed the v5.1.0 label Oct 11, 2016

suyograo deleted the usability/lazy-settings-validation branch October 27, 2016 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Settings#validate_all. #6008

Add Settings#validate_all. #6008

jordansissel commented Oct 7, 2016 •

edited

Loading

jordansissel commented Oct 7, 2016

jordansissel commented Oct 7, 2016

untergeek left a comment

jsvd left a comment

jsvd Oct 7, 2016

jsvd Oct 7, 2016 •

edited

Loading

jordansissel Oct 10, 2016

jordansissel Oct 11, 2016

jsvd Oct 7, 2016

jordansissel commented Oct 7, 2016

lines. Before merging it would be nice to remove it :)

set to nil?

action is logged but stillll.....).

jordansissel commented Oct 10, 2016

jsvd commented Oct 10, 2016

suyograo Oct 11, 2016

suyograo Oct 11, 2016

jordansissel Oct 11, 2016

jordansissel Oct 11, 2016

jordansissel Oct 11, 2016

jordansissel commented Oct 11, 2016

jordansissel commented Oct 11, 2016

jordansissel commented Oct 11, 2016

jsvd commented Oct 11, 2016

jordansissel commented Oct 11, 2016

elasticsearch-bot commented Oct 11, 2016

Add Settings#validate_all. #6008

Add Settings#validate_all. #6008

Conversation

jordansissel commented Oct 7, 2016 • edited Loading

jordansissel commented Oct 7, 2016

jordansissel commented Oct 7, 2016

untergeek left a comment

Choose a reason for hiding this comment

jsvd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsvd Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordansissel commented Oct 7, 2016

lines. Before merging it would be nice to remove it :)

set to nil?

action is logged but stillll.....).

jordansissel commented Oct 10, 2016

jsvd commented Oct 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordansissel commented Oct 11, 2016

jordansissel commented Oct 11, 2016

jordansissel commented Oct 11, 2016

jsvd commented Oct 11, 2016

jordansissel commented Oct 11, 2016

elasticsearch-bot commented Oct 11, 2016

jordansissel commented Oct 7, 2016 •

edited

Loading

jsvd Oct 7, 2016 •

edited

Loading