-
Notifications
You must be signed in to change notification settings - Fork 33
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
V0.6.0 #165
V0.6.0 #165
Conversation
Hey @BenFradet , just added as reviewer, I'll ping you once I'm about to finish implementation phase. |
9bd8639
to
405eb43
Compare
5d43590
to
7d75caf
Compare
Hey @BenFradet , I'm done with the implementation phase and would like you to review my code changes. I merged 4 tickets into #23 , see #23's page for included issues. Also, only blocker for QA is that there is an issue with Caddy configuration (every endpoint except /kibana works as expected, probably due to kibana version bump, looking into that). I hope this is not a blocker for code review. In the meantime, I'll solve the issue with /kibana endpoint, write a QA journal, prepare blog post first draft and a separate wiki branch to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did a first pass 👍
@@ -5,7 +5,6 @@ | |||
/kibana | |||
/elasticsearch | |||
/control-plane | |||
/_plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did the es plugin path change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elasticsearch has removed site plugins as of 5.x, but head plugin can be used as a browser extension which works without a problem.
return | ||
} | ||
|
||
file, handler, err := req.FormFile("igluserverjson") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this filename, isn't it supposed to be hocon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'll correct.
fileContentBytes, err := ioutil.ReadAll(file) | ||
fileContent := string(fileContentBytes) | ||
|
||
f, err := os.OpenFile(config.Dirs.Config+"/"+handler.Filename, os.O_WRONLY|os.O_CREATE, 0666) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if there is a /
at the end of config.Dirs.Config
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues, I tested and works without a problem.
defer f.Close() | ||
|
||
f.Truncate(0) | ||
f.Seek(0, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a comment here on what this is doing
appender.rolling.strategy.action.type = Delete | ||
appender.rolling.strategy.action.basepath = ${sys:es.logs.base_path} | ||
appender.rolling.strategy.action.condition.type = IfLastModified | ||
appender.rolling.strategy.action.condition.age = 7D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that too long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wasn't acted on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many days of log should be kept @BenFradet ? Is a week unnecessarily long here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 days seems long enough to me, what do you reckon @jbeemster ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
container_name: elasticsearch | ||
restart: always | ||
environment: | ||
- "bootstrap.memory_lock=true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do? can we add a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is explained here. I'll add a short comment.
restart: always | ||
environment: | ||
- "bootstrap.memory_lock=true" | ||
- "ES_JAVA_OPTS=-Xms4g -Xmx4g" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be starting with 4g? cc @jbeemster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reasoning behind those settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to take the size of the machine into account now c.f. #173, Xms and Xmx should indeed be equal, c.f. https://www.elastic.co/guide/en/elasticsearch/reference/6.2/heap-size.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's decide and I'll act accordingly regarding heap size. From the page @BenFradet referenced, it is indicated as following;
Set Xmx to no more than 50% of your physical RAM, to ensure that there is enough
physical RAM left for kernel file system caches
My only reasoning is to make ES able to stay up under high load, as much as possible. I remember us talking about publishing to t2.large
as of this release and it has 8GB ram, hence 4GB allocation to ES.
Should it be less or is it fine, what do you think? @BenFradet @jbeemster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 50% is good so that would be:
type | ES | loader | enrich | collector |
---|---|---|---|---|
t2.large | 4G | 0.5G | 0.5G | 0.5G |
t2.xlarge | 8G | 1.5G | 1.5G | 1.5G |
t2.2xlarge | 16G | 3G | 3G | 3G |
max-size: "1M" | ||
max-file: "10" | ||
environment: | ||
- "SP_JAVA_OPTS=-Xms512m -Xmx512m" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't specify Xms
for any of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this removed altogether, I would still specify Xmx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specified only Xmx now.
|
||
- name: Ensure file permissions | ||
become: yes | ||
shell: chown ubuntu:ubuntu -R /home/ubuntu/snowplow && chmod 755 -R /home/ubuntu/snowplow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this necessary, can we add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to delete it, I used that for test purposes. 4th role already does that.
|
||
- name: Increase mmap count to recommended 262144 for Elasticsearch | ||
become: yes | ||
shell: echo "vm.max_map_count=262144" >> /etc/sysctl.conf && service procps start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't in the container why would it affect es?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is because containers share the same kernel as host OS.
8a9f14d
to
7d6fba6
Compare
# rebooting kibana and retrying works out | ||
- name: Likely to fail attempt to create ES indexes & Kibana index patterns & NSQ topics | ||
become: yes | ||
shell: sh {{init_dir}}/create.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reasoning for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kibana 5.x had problems with creating index patterns (they still don't have a stable API documentation, unfortunately) and I guaranteed index pattern creation using a dirty way. Anyway, 6.x doesn't have aforementioned issue and I removed this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍
special_time: weekly | ||
job: /usr/bin/curl -s -X POST http://localhost:9200/good/_delete_by_query -d '{ "query" :{ "range" :{ "collector_tstamp" :{ "lt" :"now-1w/d" } } } }' > /dev/null 2>&1 | ||
|
||
- cron: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have something more specific as far as names are concerned instead of 3 times cron
?
Am I right that we did not bump Iglu Server to 0.3.0 in this (or previous) release? If we didn't - why? It looked like a quite serious upgrade. If we did - it probably does not work as expected due snowplow/iglu#378 Edit: seems we did. This must be QA'ed with public schemas. |
6d47589
to
759799e
Compare
5931a8c
to
0f2f3a5
Compare
352183f
to
247992e
Compare
Hey @chuwy , @BenFradet has been through his QA and he thinks it is good to go. Will you also QA or should we release tomorrow? |
Good to go from me. |
release process