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

Add watchdog script written in Lua to the gluon-mesh-vpn-tunneldigger #1735

Closed
wants to merge 11 commits into from

Conversation

MPW1412
Copy link
Contributor

@MPW1412 MPW1412 commented Jun 2, 2019

package.

The logic is based on the former watchdog written in (B)ash.

The code was written by Robin Weiligmann admin@robwei.me and me.

package.

The logic is based on the former watchdog written in (B)ash.

The code was written bei Robin Weiligmann <admin@robwei.me> and me.
@mweinelt
Copy link
Contributor

mweinelt commented Jun 2, 2019

Please use tabs for indentation everywhere.

@MPW1412
Copy link
Contributor Author

MPW1412 commented Jun 2, 2019

Thanks for your feedback, @mweinelt. The latest commit does the requested changes.

@MPW1412
Copy link
Contributor Author

MPW1412 commented Jun 2, 2019

Please use tabs for indentation everywhere.

Done in last commit. Had overseen it before.

@mweinelt
Copy link
Contributor

mweinelt commented Jun 2, 2019

The Gluon codebase uses lowercase function names with underlines separating words, not camel casing as you do.

readPidFile vs read_pid_file

…espect of coding convetions. Changed logic in read_pid_file for readability.
@MPW1412
Copy link
Contributor Author

MPW1412 commented Jun 2, 2019

The Gluon codebase uses lowercase function names with underlines separating words, not camel casing as you do.

readPidFile vs read_pid_file

@mweinelt mweinelt added 0. type: enhancement The changeset is an enhancement 3. topic: tunneldigger This is about tunneldigger, a L2TP brokering solution labels Jun 2, 2019
Copy link
Member

@blocktrron blocktrron left a comment

Choose a reason for hiding this comment

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

You mixed single and double quotes a lot in your file. This is very inconsistent in the rest of Gluon. However, while you are at it, you might want to bring a bit of consistency in your own file.

 - Fix quotation style
 - refractoring / code optimization
@MPW1412
Copy link
Contributor Author

MPW1412 commented Jun 3, 2019

Thanks for your feedback, @blocktrron. I tried to address all points in the last commit.

@mweinelt mweinelt added the 5. needs: testing Testing of the changes is necessary label Jun 4, 2019
@mweinelt
Copy link
Contributor

mweinelt commented Jun 4, 2019

Great progress, please test the latest version and report back.

@MPW1412
Copy link
Contributor Author

MPW1412 commented Jun 4, 2019

I just tested it in x86-64. Works :)

@rotanid rotanid added this to the 2019.1 milestone Jun 6, 2019
@rotanid rotanid added the 1. severity: blocker This issue/pr is required for the next release label Jun 6, 2019
Copy link
Contributor

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Looks good to me, but since I cannot test this myself, will someone with a tunneldigger setup give this a testdrive and make sure this works as intended?

@MPW1412
Copy link
Contributor Author

MPW1412 commented Jun 9, 2019

I already tested it...works. Do you want a second opinion?

@mweinelt
Copy link
Contributor

mweinelt commented Jun 9, 2019

Yup, that would be nice. I'm not very comfortable yet reviewing Lua.

@CodeFetch
Copy link
Contributor

CodeFetch commented Jun 10, 2019

Looks good. Why did you use popen instead of execute for the logger? I'd use execute as it does not create a file handle in memory or at least directly closes it. Lua's garbage collection is not very clever and it might be possible that the file handle is only getting freed on garbage collection or program termination, but I'm not sure as you don't assign it to a variable at all... Just to be sure I'd use execute. This is still pedantic, as file handles don't really hurt. Your code is fine.

Edit: I'm curious... When does it happen that the PIDs don't match?
Edit2: And I just remember that file handle get closed when they are not being assigned to variables. So it makes no difference. Only for consistency with other code where execute is being used.

@MPW1412
Copy link
Contributor Author

MPW1412 commented Jun 10, 2019

Hello @CodeFetch,

thanks for your feedback.

About the logging: Robin and I are not very familiar with Lua. When we first started this little project, we expected Lua to have some command or package to write to the syslog. But we couldn't find anything in other gluon packages.

So we just used the first best command we knew.

About edit(1): We don't know what the idea behind this is. We just ported the old (b)ash script to Lua as we were told (b)ash scripts wouldn't be accepted in Gluon anymore.

About edit(2): Okay, we'll stay with popen for now.

Best,
Matthias

local uci = require('simple-uci').cursor()

function restart_tunneldigger()
io.popen('logger -t tunneldigger-watchdog "Restarting Tunneldigger."')
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we don't parse any return values we might use os.execute here as well, no?

@mweinelt mweinelt self-assigned this Jun 12, 2019
@Adorfer
Copy link
Contributor

Adorfer commented Jun 16, 2019

works as expected on 841: After killing the tunneldigger process, the checkscript detected and made a new tunnel.

@mweinelt
Copy link
Contributor

mweinelt commented Jun 16, 2019

Squashed and merged with minor changes in 8e17635.

Thanks for your work!

@mweinelt mweinelt closed this Jun 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. type: enhancement The changeset is an enhancement 1. severity: blocker This issue/pr is required for the next release 3. topic: tunneldigger This is about tunneldigger, a L2TP brokering solution 5. needs: testing Testing of the changes is necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants