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

Warning if remote watcher configuration is too low #660

Merged
merged 2 commits into from
Jan 31, 2020
Merged

Conversation

pchico83
Copy link
Contributor

Fixes #610

@codecov
Copy link

codecov bot commented Jan 29, 2020

Codecov Report

Merging #660 into master will increase coverage by 0.13%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #660      +/-   ##
==========================================
+ Coverage   38.15%   38.29%   +0.13%     
==========================================
  Files          42       42              
  Lines        3575     3583       +8     
==========================================
+ Hits         1364     1372       +8     
  Misses       2131     2131              
  Partials       80       80
Impacted Files Coverage Δ
cmd/up.go 6.34% <0%> (-0.09%) ⬇️
cmd/utils.go 17.77% <66.66%> (+17.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a1a5ab...e484377. Read the comment docs.

cmd/up.go Outdated
os.Stderr,
[]string{"sh", "-c", "((cp /var/okteto/bin/* /usr/local/bin); (ps -ef | grep -v -E '/var/okteto/bin/syncthing|/var/okteto/bin/remote|PPID' | awk '{print $2}' | xargs -r kill -9)) >/dev/null 2>&1"},
[]string{"sh", "-c", "(((cp /var/okteto/bin/* /usr/local/bin); (ps -ef | grep -v -E '/var/okteto/bin/syncthing|/var/okteto/bin/remote|PPID' | awk '{print $2}' | xargs -r kill -9)) >/dev/null 2>&1) || cat /proc/sys/fs/inotify/max_user_watches"},
Copy link
Member

Choose a reason for hiding this comment

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

do we need all the parenthesis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is for readability. We should move this to a proper script/program in the okteto/bin image

cmd/up.go Outdated
os.Stderr,
[]string{"sh", "-c", "((cp /var/okteto/bin/* /usr/local/bin); (ps -ef | grep -v -E '/var/okteto/bin/syncthing|/var/okteto/bin/remote|PPID' | awk '{print $2}' | xargs -r kill -9)) >/dev/null 2>&1"},
[]string{"sh", "-c", "(((cp /var/okteto/bin/* /usr/local/bin); (ps -ef | grep -v -E '/var/okteto/bin/syncthing|/var/okteto/bin/remote|PPID' | awk '{print $2}' | xargs -r kill -9)) >/dev/null 2>&1) || cat /proc/sys/fs/inotify/max_user_watches"},
Copy link
Member

Choose a reason for hiding this comment

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

with the || the final script will only execute if the xargs exits in 1 no?

cmd/utils.go Outdated
log.Yellow("We recommend you to raise it to at least 524288 to ensure proper performance.")
fmt.Println()
}
}

func isWatchesTooLow(value string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func isWatchesTooLow(value string) bool {
func areWatchesTooLow(value string) bool {

log.Yellow("The value of /proc/sys/fs/inotify/max_user_watches is too low. This can affect Okteto's file synchronization performance.")
if isWatchesTooLow(string(f)) {
log.Yellow("The value of /proc/sys/fs/inotify/max_user_watches is too low.")
log.Yellow("This can affect Okteto's file synchronization performance.")
log.Yellow("We recommend you to raise it to at least 524288 to ensure proper performance.")
Copy link
Member

Choose a reason for hiding this comment

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

we are checking for 8192 in the function but on the message we recommend 524288. Is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is the default value. we do the same for local watches

@pchico83 pchico83 merged commit 35b7698 into master Jan 31, 2020
@pchico83 pchico83 deleted the remote-watcher branch January 31, 2020 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show a warning if the watchers are not properly configured in the node
2 participants