-
Notifications
You must be signed in to change notification settings - Fork 761
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
p2pool: Restart monerod only when needed and with proper args #3936
p2pool: Restart monerod only when needed and with proper args #3936
Conversation
f1a52de
to
f9f58c3
Compare
One question. If someone has their monerod managed through something like systemd, does systemd even allow it that a different program restarts |
@selsta The current behavior is that monerod restarts every time p2pool mining is started. This PR will just reduce the amount of restarts. I think in the case of systemd, monerod will be restarted, but not under systemd anymore, it will be its own separate process. I can open a separate PR to detect if monerod is under systemd. |
Should be ready to go now, haven't tested yet though. |
597ccce
to
83c4506
Compare
@selsta Just tested it, it works. |
8e7b047
to
9169dbb
Compare
@devhyper will try to get it merged |
I can't compile this: (planning to confirm systemd monerod also)
I will also test linux but for now, moving onto windows: Win 10 using docker-windows-static (i need a 2nd opinion on these) Test: monerod args Videono_restart.webmTest: monerod args Videoprune_zmq_fine.webmTest: no args Videono_args_loop.webmTest: monerod with only arg Videoprune_only_loop.webmTest: monerod with only arg Videofine_with_tcp_only.webm |
@plowsof Deprecation and compile errors fixed, most of the cases should be fixed as well. I'll test some more when I can. |
Thanks! linux compiles now. some issues:
daemon args in gui: just
Win 10 docker-static: |
@plowsof This should fix it, I'm currently not able to test right now, will be able to test in a few days. |
Thanks! Only seeing the
|
@devhyper i have made the adjustments so all test cases pass. I will test some time tomorrow to confirm, then make a pull request to your branch with the commit. We can get final touches done / merged asap after |
Win 10 using docker-windows-static 5a4554f To test the js parser: play with it in e.g. jfiddle / w3schools editor<!DOCTYPE html>
<html>
<body>
<p id="demo"></p>
<script>
var customDaemonArgs = "--log-level 0 --check-updates disabled --non-interactive --max-concurrency 4"
//var customDaemonArgs = "--check-updates disabled --non-interactive --max-concurrency 4 --some-random variables"
//var customDaemonArgs = "--check-updates disabled --non-interactive --max-concurrency 4"
//var customDaemonArgs = "--detach --data-dir --bootstrap-daemon-address --prune-blockchain --no-sync --check-updates --non-interactive --max-concurrency --no-zmq --zmq-pub=tcp://127.0.0.1:18083"
//these args will be deleted because DaemonManager::start will re-add them later.
//--no-zmq must be deleted. removing '--zmq-pub=tcp...' lets us blindly add '--zmq-pub tcp...' later without risking duplication.
var defaultArgs = ["--detach","--data-dir","--bootstrap-daemon-address","--prune-blockchain","--no-sync","--check-updates","--non-interactive","--max-concurrency","--no-zmq","--zmq-pub=tcp://127.0.0.1:18083"]
var customDaemonArgsArray = customDaemonArgs.split(' ');
var flag = "";
var allArgs = [];
var p2poolArgs = ["--zmq-pub tcp://127.0.0.1:18083","--disable-dns-checkpoints"];
var daemonArgs = "";
//create an array (allArgs) of ['--arg value','--arg2','--arg3']
for (let i = 0; i < customDaemonArgsArray.length; i++) {
if(!customDaemonArgsArray[i].startsWith("--")) {
flag += " " + customDaemonArgsArray[i]
} else {
if(flag){
allArgs.push(flag)
}
flag = customDaemonArgsArray[i]
}
}
allArgs.push(flag)
//pop from allArgs if value is inside the deleteme array (defaultArgs)
for (let i = (allArgs.length - 1); i >= 0; i--) {
for (let n = (defaultArgs.length - 1); n >= 0; n--) {
if(allArgs[i].includes(defaultArgs[n])) {
allArgs.splice(i,1)
defaultArgs.splice(n,1)
if(i==0) {
break
}
i-=1
}
}
}
//append required p2pool flags
for (let i = 0; i < p2poolArgs.length; i++) {
if(!allArgs.includes(p2poolArgs[i])) {
allArgs.push(p2poolArgs[i])
continue
}
}
document.getElementById("demo").innerHTML = "Input:<br>" + customDaemonArgs + "<br><br>Monerod flags: <br>" + allArgs.join(" ");
</script>
</body>
</html> Test: monerod args
Videono_restart_1.webmTest: monerod args
Videono_restart_2.webmTest: monerod args
Videonozmq_3.webmTest: monerod args
Videolog_level_4.webmTest: no args
Videono_args.webmTest: monerod with only arg
Videoprune_removed_5.webmTest: monerod with only arg
Videozmq_eq_6.webmTest: monerod with only arg
Videozmq_7.webm |
Thanks for the help, I'll test again on Windows and Linux later today. |
Just tested all the cases on Windows (with custom data dir), all passed. Testing Linux next. |
I've also made another pull request to your branch with some extra tweaks that came up during review/testing. I am pleased with this PR so far! |
Linux tested, all cases passed. Testing omitting --disable-dns-checkpoints worked as well. |
@selsta Is --disable-dns-checkpoints needed for p2pool anymore? |
@devhyper It shouldn't be necessary anymore. |
After squashing, i approve, all tests passing* sanity checks performed on binaries from the latest workflow run Test: monerod args
Test: monerod args
Test: monerod args
Test: monerod args
Test: no args (so with defaults)
Test: monerod with only arg
Test: monerod running as a service but needs to restart
Test: monerod running as a service with --zmq-pub in the unit file
|
@devhyper please squash |
1ca8df6
to
757bc7d
Compare
Resolves #3911 and may resolve #3935