Opened 5 years ago

Closed 4 years ago

#18865 closed enhancement (fixed)

actively monitor resources like available storage space

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: CollecTor 1.0.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords: ctip
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

As the two incidents of at least temporary losses/unavailability of descriptors were due to insufficient memory (cf. here, a timed tasked should check this (and possibly other parameters) at regular intervals (preferably in a timely manner before the next run) and raise the red flag when a problem is visible.

Might this be useful? Or is storage only a current problem?

Child Tickets

Change History (13)

comment:1 Changed 5 years ago by karsten

I think I'd rather want us to fix the underlying problem of using massive amounts of disk space. The best way to achieve that is to improve how we're producing (xz-compressed) tarballs. Right now, we're creating those tarballs by first writing the uncompressed tarball to disk and then compressing it. Oh, and of course, we have the directories on disk that we're creating tarballs from. I hope we can find a better way. (If there's already a ticket for that, please feel free to paste a link to this ticket.)

I also just asked the Tor admins to send me automatic email notifications in case the disk is about to run full. That should be a good enough band-aid.

Sound okay?

comment:2 in reply to:  1 Changed 5 years ago by iwakeh

Replying to karsten:

I think I'd rather want us to fix the underlying problem of using massive amounts of disk space. The best way to achieve that is to improve how we're producing (xz-compressed) tarballs. Right now, we're creating those tarballs by first writing the uncompressed tarball to disk and then compressing it. Oh, and of course, we have the directories on disk that we're creating tarballs from. I hope we can find a better way. (If there's already a ticket for that, please feel free to paste a link to this ticket.)

Yes, this is fine. Let's use the above list for future discussion here.

I also just asked the Tor admins to send me automatic email notifications in case the disk is about to run full. That should be a good enough band-aid.

Sound okay?

That's fine, too. (I actually thought that this was the case already ;-)

Would it make sense to have the "is there enough space check?" immediately at the beginning of each
run? That way in case of a shortage only things like votes and consensus etc. could be retrieved, and
more space consuming tasks could be postponed.
Thus, avoiding the overfull disk?

comment:3 Changed 5 years ago by karsten

I'd say let's add a warning if there's not much space left on the device at the beginning of a run (less than 200M?). But let's not make CollecTor behave differently when it finds that it's low on disk space. If we have such warnings in place (the Nagios notification and the new low-on-disk-space warning), we should assume that operators are smart enough to do the right thing. The recent problems only happened, because we don't have any notifications at all. But acting differently in case of low disk space would be like a car on reserve that doesn't let you drive faster than 100 km/h or not in opposite direction of a nearby gas station. Oh, I would hate a car that thinks it's smarter than I, even if it is.

comment:4 Changed 4 years ago by iwakeh

Status: newaccepted

Yes, agreed; humans decide not algorithms ;-)

todo:
log warnings in case of insufficient space (start with 200M limit)
It might be good to also check at the end of a run, as there would be more time for operators to react.

comment:5 Changed 4 years ago by iwakeh

Milestone: CollecTor 1.0.0

Added to first release milestone.

comment:6 Changed 4 years ago by iwakeh

Status: acceptedneeds_review

Please review this branch.

Currently, the check is only added to ArchiveWriter. Where else should it go?

I didn't make the limit configurable, as the properties file is really big already and the space needed depends on the download and will be the same for all instances. But, if there is a good reason for adding it to the configuration that's not a big deal.

comment:7 Changed 4 years ago by karsten

Status: needs_reviewneeds_revision

Change looks good, just minor nitpicks, and I'm happy to make all changes if you're okay with that:

  • Some lines exceed the implicit 74 character limit that I have been using in the past and that we might not have included in the styleguides yet. Let me explain: 74 characters is the maximum number of characters that look reasonable in vim with line numbers turned on in files up to 999 lines. Somewhat arbitrary, I know. Are there arguments in favor of other line lengths? If so, let's discuss those. Otherwise, let's just keep 74.
  • Can we remove those newlines after closing brackets?
  • I wonder how the Javadoc with three sentences without newline between first and second and without <p></p> around second and third sentence conforms with checkstyle. Can we rephrase those three sentences to a single sentence or separate them into one sentence and a paragraph?
  • Both 1024. and Math.floor seem unnecessary for long values. Can we change them to 1024 and omit the flooring?
  • I think the preferred Git message format is: "summary of no more than 50 chars, newline, one or more parapraphs with no more than 70 chars per line". (Note: I sometimes exceed the 50, and I'm open to using something else than 70, for example, 72 which I just read on a random blog post.) Do you mind if we change the message to something like the following?
Check available disk space in relaydescs module.

Check if there's enough space before and after running the relaydescs
module, and warn if less than 200 MiB are left.

Implements #18865.

So, mostly whitespace complaints. :) But I figured it's better to bring them up at some point to make future reviews faster. Again, happy to change things if you agree.

comment:8 in reply to:  7 ; Changed 4 years ago by iwakeh

Thanks for reviewing so quickly!

  • Some lines exceed the implicit 74 character limit that I have been using in the past and that we might not have included in the styleguides yet. Let me explain: 74 characters is the maximum number of characters that look reasonable in vim with line numbers turned on in files up to 999 lines. Somewhat arbitrary, I know. Are there arguments in favor of other line lengths? If so, let's discuss those. Otherwise, let's just keep 74.

Well, we actually have a 80-100 column limit, but I prefer to keep it between 70 and 80. That's why this shows up repeatedly.

Please, make the changes to the guide doc, but I prefer 80.

(I vaguely remember that we agreed on 76-80 in one meeting, but I can't find the minutes.)

74 is too short and the problem will become worse with java8 and using lambda expressions and more fluent style. The coding style changed and we all use screens way bigger than in the last century. A line of length 80 can easily be read with one glance still.

  • Can we remove those newlines after closing brackets?
  • I wonder how the Javadoc with three sentences without newline between first and second and without <p></p> around second and third sentence conforms with checkstyle. Can we rephrase those three sentences to a single sentence or separate them into one sentence and a paragraph?

I usually run ant checks before committing and it doesn't complain.
The three sentence javadoc will become one paragraph, but when editing it later it's easier to see, which sentence was changed.

  • Both 1024. and Math.floor seem unnecessary for long values. Can we change them to 1024 and omit the flooring?

Oops, please change.

  • I think the preferred Git message format is: "summary of no more than 50 chars, newline, one or more parapraphs with no more than 70 chars per line". (Note: I sometimes exceed the 50, and I'm open to using something else than 70, for example, 72 which I just read on a random blog post.) Do you mind if we change the message to something like the following?
Check available disk space in relaydescs module.

Check if there's enough space before and after running the relaydescs
module, and warn if less than 200 MiB are left.

Implements #18865.

That's fine!
And the git-msg format should have a place in the docs somewhere.

So, mostly whitespace complaints. :) But I figured it's better to bring them up at some point to make future reviews faster. Again, happy to change things if you agree.

Please do. Thanks a lot!

comment:9 in reply to:  8 Changed 4 years ago by karsten

Replying to iwakeh:

Thanks for reviewing so quickly!

  • Some lines exceed the implicit 74 character limit that I have been using in the past and that we might not have included in the styleguides yet. Let me explain: 74 characters is the maximum number of characters that look reasonable in vim with line numbers turned on in files up to 999 lines. Somewhat arbitrary, I know. Are there arguments in favor of other line lengths? If so, let's discuss those. Otherwise, let's just keep 74.

Well, we actually have a 80-100 column limit, but I prefer to keep it between 70 and 80. That's why this shows up repeatedly.

Please, make the changes to the guide doc, but I prefer 80.

(I vaguely remember that we agreed on 76-80 in one meeting, but I can't find the minutes.)

74 is too short and the problem will become worse with java8 and using lambda expressions and more fluent style. The coding style changed and we all use screens way bigger than in the last century. A line of length 80 can easily be read with one glance still.

Alright, those are good arguments for changing to 80. Changed in the docs.

  • Can we remove those newlines after closing brackets?
  • I wonder how the Javadoc with three sentences without newline between first and second and without <p></p> around second and third sentence conforms with checkstyle. Can we rephrase those three sentences to a single sentence or separate them into one sentence and a paragraph?

I usually run ant checks before committing and it doesn't complain.

Yep, I ran that, too, and it didn't complain. Okay, removed the newlines anyway.

The three sentence javadoc will become one paragraph, but when editing it later it's easier to see, which sentence was changed.

Right, I assumed that the first paragraph should be a single sentence, because that's what goes into the summary. I didn't change this yet, but what do you think about the suggestion above?

  • Both 1024. and Math.floor seem unnecessary for long values. Can we change them to 1024 and omit the flooring?

Oops, please change.

Changed.

  • I think the preferred Git message format is: "summary of no more than 50 chars, newline, one or more parapraphs with no more than 70 chars per line". (Note: I sometimes exceed the 50, and I'm open to using something else than 70, for example, 72 which I just read on a random blog post.) Do you mind if we change the message to something like the following?
Check available disk space in relaydescs module.

Check if there's enough space before and after running the relaydescs
module, and warn if less than 200 MiB are left.

Implements #18865.

That's fine!
And the git-msg format should have a place in the docs somewhere.

Sounds good. Amended and pushed to my task-18865 branch.

Can you add this format guideline to the docs?

So, mostly whitespace complaints. :) But I figured it's better to bring them up at some point to make future reviews faster. Again, happy to change things if you agree.

Please do. Thanks a lot!

Great! :) As soon as I know what to do with the Javadoc, I'll push and close. Thanks!

comment:10 Changed 4 years ago by iwakeh

Please just rephrase to the one sentence option, than all will be visible in the summary.
Thanks!

comment:11 Changed 4 years ago by karsten

Status: needs_revisionassigned

Rephrased and pushed to master. Leaving this ticket open for the Git format thing. Please either add some text for that or suggest a place where this could live and I'll add something there. Thanks!

comment:12 Changed 4 years ago by iwakeh

Thanks!

I think the patch format for contributers is a good place, i.e. here.

Last edited 4 years ago by iwakeh (previous) (diff)

comment:13 Changed 4 years ago by karsten

Resolution: fixed
Status: assignedclosed

Okay, added something. Closing.

Note: See TracTickets for help on using tickets.