tahoe-lafs/tahoe-lafs
tahoe-lafs
/
tahoe-lafs
2
0
Fork 0
Code Issues Pull Requests 23 Packages Projects Releases 1 Wiki Activity

remove start stop restart daemonize #921

Merged
exarkun merged 35 commits from 3550.remove-start-stop-restart-daemonize into master 2020-12-14 19:58:36 +00:00
Conversation 3 Commits 35 Files Changed 25 +313 -1244
exarkun commented 2020-12-09 15:57:55 +00:00 (Migrated from github.com)
Copy Link

https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3550
https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3523
https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3524

https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3550 https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3523 https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3524
meejah (Migrated from github.com) reviewed 2020-12-09 15:57:55 +00:00
codecov[bot] commented 2020-12-09 17:10:37 +00:00 (Migrated from github.com)
Copy Link

Codecov Report

Merging #921 (bdb7c50) into master (e59a922) will decrease coverage by 0%.
The diff coverage is 62%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #921    +/-   ##
=======================================
- Coverage      92%     92%    -0%     
=======================================
  Files         156     151     -5     
  Lines       27291   27110   -181     
  Branches     4091    4069    -22     
=======================================
- Hits        25116   24939   -177     
- Misses       1516    1519     +3     
+ Partials      659     652     -7
Impacted Files Coverage Δ
src/allmydata/scripts/common.py 92% <ø> (ø)
src/allmydata/scripts/runner.py 74% <ø> (-1%) ⬇️
src/allmydata/scripts/tahoe_run.py 63% <62%> (-37%) ⬇️
src/allmydata/util/deferredutil.py 65% <0%> (-19%) ⬇️
src/allmydata/web/status.py 95% <0%> (-<1%) ⬇️
src/allmydata/immutable/downloader/node.py 96% <0%> (+<1%) ⬆️
src/allmydata/immutable/downloader/share.py 95% <0%> (+1%) ⬆️

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 e59a922...bdb7c50. Read the comment docs.

# [Codecov](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921?src=pr&el=h1) Report > Merging [#921](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921?src=pr&el=desc) (bdb7c50) into [master](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/commit/e59a922b27e9f177e1b51faf75c64bdd915865ef?el=desc) (e59a922) will **decrease** coverage by `0%`. > The diff coverage is `62%`. [![Impacted file tree graph](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921/graphs/tree.svg?width=650&height=150&src=pr&token=Ztmu9sr4vi)](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921?src=pr&el=tree) ```diff @@ Coverage Diff @@ ## master #921 +/- ## ======================================= - Coverage 92% 92% -0% ======================================= Files 156 151 -5 Lines 27291 27110 -181 Branches 4091 4069 -22 ======================================= - Hits 25116 24939 -177 - Misses 1516 1519 +3 + Partials 659 652 -7 ``` | [Impacted Files](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921?src=pr&el=tree) | Coverage Δ | | |---|---|---| | [src/allmydata/scripts/common.py](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921/diff?src=pr&el=tree#diff-c3JjL2FsbG15ZGF0YS9zY3JpcHRzL2NvbW1vbi5weQ==) | `92% <ø> (ø)` | | | [src/allmydata/scripts/runner.py](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921/diff?src=pr&el=tree#diff-c3JjL2FsbG15ZGF0YS9zY3JpcHRzL3J1bm5lci5weQ==) | `74% <ø> (-1%)` | :arrow_down: | | [src/allmydata/scripts/tahoe\_run.py](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921/diff?src=pr&el=tree#diff-c3JjL2FsbG15ZGF0YS9zY3JpcHRzL3RhaG9lX3J1bi5weQ==) | `63% <62%> (-37%)` | :arrow_down: | | [src/allmydata/util/deferredutil.py](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921/diff?src=pr&el=tree#diff-c3JjL2FsbG15ZGF0YS91dGlsL2RlZmVycmVkdXRpbC5weQ==) | `65% <0%> (-19%)` | :arrow_down: | | [src/allmydata/web/status.py](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921/diff?src=pr&el=tree#diff-c3JjL2FsbG15ZGF0YS93ZWIvc3RhdHVzLnB5) | `95% <0%> (-<1%)` | :arrow_down: | | [src/allmydata/immutable/downloader/node.py](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921/diff?src=pr&el=tree#diff-c3JjL2FsbG15ZGF0YS9pbW11dGFibGUvZG93bmxvYWRlci9ub2RlLnB5) | `96% <0%> (+<1%)` | :arrow_up: | | [src/allmydata/immutable/downloader/share.py](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921/diff?src=pr&el=tree#diff-c3JjL2FsbG15ZGF0YS9pbW11dGFibGUvZG93bmxvYWRlci9zaGFyZS5weQ==) | `95% <0%> (+1%)` | :arrow_up: | ------ [Continue to review full report at Codecov](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921?src=pr&el=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921?src=pr&el=footer). Last update [e59a922...bdb7c50](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/921?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
exarkun (Migrated from github.com) reviewed 2020-12-11 01:54:37 +00:00
src/allmydata/test/check_grid.py
@ -32,3 +23,1 @@
tahoe put -d DIR FILE testgrid:recentdir/recent.MD5SUM
echo "" | tahoe put -d DIR --mutable testgrid:log
echo "" | tahoe put -d DIR --mutable testgrid:recentlog
tahoe create-client --introducer=INTRODUCER_FURL DIR
exarkun (Migrated from github.com) commented 2020-12-11 01:54:37 +00:00
Copy Link

Still need to do something about this...

Still need to do something about this...
exarkun commented 2020-12-11 01:55:21 +00:00 (Migrated from github.com)
Copy Link

There's one outstanding issue (check_grid.py) but I think the solution will be small and localized. The rest of this should be ready for a review.

There's one outstanding issue (check_grid.py) but I think the solution will be small and localized. The rest of this should be ready for a review.
sajith (Migrated from github.com) approved these changes 2020-12-12 15:46:32 +00:00
sajith (Migrated from github.com) left a comment
Copy Link

Looks good to me!

Since this PR contains a fairly large set of changes, it might be good to have a second set of eyes to go over this?

Looks good to me! Since this PR contains a fairly large set of changes, it might be good to have a second set of eyes to go over this?
docs/logging.rst
@ -135,3 +133,4 @@
"``gatherer.tac``" file should be modified to add classifier functions.
The incident gatherer writes incident names (which are simply the relative
pathname of the ``incident-\*.flog.bz2`` file) into ``classified/CATEGORY``.
sajith (Migrated from github.com) commented 2020-12-12 13:56:49 +00:00
Copy Link

I am unsure about flogtool create-incident-gatherer WORKDIR because:

$ flogtool create-incident-gatherer `pwd`
flogtool:  --location= is mandatory

Perhaps working examples would be better?

I am unsure about ``flogtool create-incident-gatherer WORKDIR`` because: ``` $ flogtool create-incident-gatherer `pwd` flogtool: --location= is mandatory ``` Perhaps working examples would be better?
docs/logging.rst
sajith (Migrated from github.com) commented 2020-12-12 14:21:20 +00:00
Copy Link

In the case of flogtool create-gatherer too, I believe a working example would be better.

In the case of `flogtool create-gatherer` too, I believe a working example would be better.
src/allmydata/scripts/run_common.py
sajith (Migrated from github.com) commented 2020-12-12 14:49:16 +00:00
Copy Link

After start/stop/daemonize has been removed, will lines 219-261 still be up-to-date?

e59a922b27/src/allmydata/scripts/run_common.py (L219-L261)

After start/stop/daemonize has been removed, will lines 219-261 still be up-to-date? https://github.com/tahoe-lafs/tahoe-lafs/blob/e59a922b27e9f177e1b51faf75c64bdd915865ef/src/allmydata/scripts/run_common.py#L219-L261
src/allmydata/test/test_runner.py
sajith (Migrated from github.com) commented 2020-12-12 15:44:33 +00:00
Copy Link

Curious comment here. There's no special handling for cygwin in this module, so I guess this must have been stale since 629185d98.

We don't know anything about Cygwin these days, do we? I guess we will burn that bridge if/when we ever come to it again. :-)

Curious comment here. There's no special handling for cygwin in this module, so I guess this must have been stale since 629185d98. We don't know anything about Cygwin these days, do we? I guess we will burn that bridge if/when we ever come to it again. :-)
sajith (Migrated from github.com) reviewed 2020-12-12 21:11:17 +00:00
src/allmydata/scripts/run_common.py
sajith (Migrated from github.com) commented 2020-12-12 21:11:17 +00:00
Copy Link

There's also DaemonizeTahoeNodePlugin and DaemonizeTheRealService that can probably go.

Seems that --nodaemon argument to tahoe run command will not have any effect any more? There's handling for --nodaemon in a bunch of places:

51e50671e5/src/allmydata/test/cli/test_cli.py (L1285)

51e50671e5/src/allmydata/test/cli/test_cli.py (L1305-L1308)

51e50671e5/src/allmydata/scripts/tahoe_run.py (L14-L15)

Perhaps scrips.tahoe_run and scripts.run_common can merge at this point?

There's also `DaemonizeTahoeNodePlugin` and `DaemonizeTheRealService` that can probably go. Seems that `--nodaemon` argument to `tahoe run` command will not have any effect any more? There's handling for `--nodaemon` in a bunch of places: https://github.com/tahoe-lafs/tahoe-lafs/blob/51e50671e5a6c986b41d235b5d541337dcf2e40d/src/allmydata/test/cli/test_cli.py#L1285 https://github.com/tahoe-lafs/tahoe-lafs/blob/51e50671e5a6c986b41d235b5d541337dcf2e40d/src/allmydata/test/cli/test_cli.py#L1305-L1308 https://github.com/tahoe-lafs/tahoe-lafs/blob/51e50671e5a6c986b41d235b5d541337dcf2e40d/src/allmydata/scripts/tahoe_run.py#L14-L15 Perhaps `scrips.tahoe_run` and `scripts.run_common` can merge at this point?
exarkun (Migrated from github.com) reviewed 2020-12-12 21:55:46 +00:00
docs/logging.rst
@ -135,3 +133,4 @@
"``gatherer.tac``" file should be modified to add classifier functions.
The incident gatherer writes incident names (which are simply the relative
pathname of the ``incident-\*.flog.bz2`` file) into ``classified/CATEGORY``.
exarkun (Migrated from github.com) commented 2020-12-12 21:55:46 +00:00
Copy Link

Working examples are great. I guess this is a pre-existing and unrelated defect in the docs, though, so it shouldn't strictly be a blocker for merging this PR. I can file a ticket for this and the other error in this doc ... but my preferred outcome would probably be to escape Foolscap entirely and no longer have an "incident gatherer" or a "[log] gatherer" (and why is Tahoe-LAFS documenting the Foolscap flogtool anyway instead of referencing Foolscap-maintained docs anyway)?

Also there's the larger question of CI for examples which we should have sometime.

So, gonna leave this broken but I filed a ticket - https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3556

Working examples are great. I guess this is a pre-existing and unrelated defect in the docs, though, so it shouldn't strictly be a blocker for merging this PR. I can file a ticket for this and the other error in this doc ... but my preferred outcome would probably be to escape Foolscap entirely and no longer have an "incident gatherer" or a "[log] gatherer" (and why is Tahoe-LAFS documenting the Foolscap `flogtool` anyway instead of referencing Foolscap-maintained docs anyway)? Also there's the larger question of CI for examples which we should have sometime. So, gonna leave this broken but I filed a ticket - https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3556
exarkun (Migrated from github.com) reviewed 2020-12-12 21:59:08 +00:00
src/allmydata/scripts/run_common.py
exarkun (Migrated from github.com) commented 2020-12-12 21:59:08 +00:00
Copy Link

As far as I can tell, `DaemonizeTahoeNode", "DaemonizeTahoeNodePlugin", and "DaemonizeTheRealService" are somewhat misnamed as none of them actually do any daemonization - they're just the code that runs after daemonization is done.

But, yea, there is some more cleanup that could happen here, and maybe some more dead code that could be deleted. I'll see what else I can rip out.

As far as I can tell, `DaemonizeTahoeNode", "DaemonizeTahoeNodePlugin", and "DaemonizeTheRealService" are somewhat misnamed as none of them actually do any daemonization - they're just the code that runs *after* daemonization is done. But, yea, there is some more cleanup that could happen here, and maybe some more dead code that could be deleted. I'll see what else I can rip out.
exarkun (Migrated from github.com) reviewed 2020-12-12 22:01:36 +00:00
src/allmydata/test/test_runner.py
exarkun (Migrated from github.com) commented 2020-12-12 22:01:36 +00:00
Copy Link

Yea I haven't cared about Cygwin for a long time.

Yea I haven't cared about Cygwin for a long time.
exarkun (Migrated from github.com) reviewed 2020-12-12 23:35:08 +00:00
src/allmydata/scripts/run_common.py
exarkun (Migrated from github.com) commented 2020-12-12 23:35:07 +00:00
Copy Link

Turns out there wasn't really much.

Turns out there wasn't really much.
sajith commented 2020-12-14 01:42:49 +00:00 (Migrated from github.com)
Copy Link

I was looking at check_grid.py. This paragraph in the commentary is no longer correct, now that start/stop is gone:

34cd1efaa4/src/allmydata/test/check_grid.py (L19-L21)

Setup instructions could be a little more helpful here:

34cd1efaa4/src/allmydata/test/check_grid.py (L25-L27)

In order to save some time with fiddling, I think lines 25-26 could be re-written as:

$ tahoe create-client --nickname=NICK --introducer=pb://... DIR

I found the error message when running daemonize tahoe confusing:

$ daemonize tahoe run DIR
The 'path' parameter must be an absolute path name.

It is unclear what is printing that message -- daemoinize or tahoe? (It is daemonize.) It should probably be:

$DAEMONIZE /path/to/bin/tahoe run /path/to/DIR

Absolute paths to both tahoe and DIR are needed, or things will be confusing. My copy of daemonize was silent when tahoe failed to run because DIR was not an absolute path.

Once all the setup was done python check_grid.py DIR $(which tahoe) reported that it's checked things it was supposed to check, without spewing a bunch of errors. I guess there could be a version of check_grid.py that can be nifty, with some work...

I was looking at `check_grid.py`. This paragraph in the commentary is no longer correct, now that start/stop is gone: https://github.com/tahoe-lafs/tahoe-lafs/blob/34cd1efaa477ba28e0bd7ce15605db3c72631b7a/src/allmydata/test/check_grid.py#L19-L21 Setup instructions could be a little more helpful here: https://github.com/tahoe-lafs/tahoe-lafs/blob/34cd1efaa477ba28e0bd7ce15605db3c72631b7a/src/allmydata/test/check_grid.py#L25-L27 In order to save some time with fiddling, I think lines 25-26 could be re-written as: ``` $ tahoe create-client --nickname=NICK --introducer=pb://... DIR ``` I found the error message when running `daemonize tahoe` confusing: ``` console $ daemonize tahoe run DIR The 'path' parameter must be an absolute path name. ``` It is unclear what is printing that message -- daemoinize or tahoe? (It is daemonize.) It should probably be: ``` $DAEMONIZE /path/to/bin/tahoe run /path/to/DIR ``` Absolute paths to both `tahoe` and `DIR` are needed, or things will be confusing. My copy of daemonize was silent when tahoe failed to run because `DIR` was not an absolute path. Once all the setup was done `python check_grid.py DIR $(which tahoe)` reported that it's checked things it was supposed to check, without spewing a bunch of errors. I guess there could be a version of `check_grid.py` that can be nifty, with some work...
Sign in to join this conversation.
Reviewers
No reviewers
meejah
sajith
Labels
Clear labels   
Benchmarking and Performance

   
HTTP Storage Protocol

   
Nevow Removal

   
Python 3 Porting

   
not-for-merge
No Label
Benchmarking and Performance
HTTP Storage Protocol
Nevow Removal
Python 3 Porting
not-for-merge
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: tahoe-lafs/tahoe-lafs#921
No description provided.
Delete Branch "3550.remove-start-stop-restart-daemonize"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?