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.

Sign up for GitHub

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

Jump to bottom

Fixes #18357: Add check for free space #3301

Merged
merged 1 commit into from
Oct 25, 2020
Merged

Fixes #18357: Add check for free space #3301

merged 1 commit into from
Oct 25, 2020

Conversation

ElaadF
Copy link
Member

@ElaadF ElaadF commented Oct 23, 2020

fanf
fanf reviewed Oct 23, 2020
View reviewed changes
...udder-core/src/main/scala/com/normation/rudder/services/healthcheck/HealthcheckService.scala Outdated
final case class SpaceInfo(val partition: String, val free: Long, val available: Long){
def pourcentage: Double =
if (available != 0) {
(free * 100 / BigDecimal(available)).setScale(2, BigDecimal.RoundingMode.HALF_UP).toDouble
Copy link
Member

Choose a reason for hiding this comment

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

not sur why the BigDecimal here? BigDecimal(free * 100) if you want to avoid an overflow on the mult, but why on the denominator?

...udder-core/src/main/scala/com/normation/rudder/services/healthcheck/HealthcheckService.scala Outdated
partitionsList <- IOResult.effect{
partc.map { x =>
val file = new io.File(x)
SpaceInfo(x, file.getFreeSpace, file.getTotalSpace)
Copy link
Member

Choose a reason for hiding this comment

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

you should do that only once you have filtered partition and found what you want.

...udder-core/src/main/scala/com/normation/rudder/services/healthcheck/HealthcheckService.scala Outdated
}
}
} yield {
val (varSpaceInfo, rootSpaceInfo) = partitionsList.partition(_.partition.regionMatches(true, 0, "/var", 0, 4))
Copy link
Member

Choose a reason for hiding this comment

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

I have an hard time followinf what is done, while it's (should be) simple.
Perhaps can you add comments for the four big steps:

  • find partition
  • get memory info
  • find problems (do math)
  • transform to result

@ElaadF ElaadF added the WIP Use that label for a Work In Progress PR that must not be merged yet label Oct 23, 2020
@ElaadF
Copy link
Member Author

ElaadF commented Oct 23, 2020

PR rebased

@ElaadF ElaadF force-pushed the ust_18357/add_check_for_free_space branch from 23e31fd to 465cae5 Compare October 23, 2020 17:44
VinceMacBuche
VinceMacBuche reviewed Oct 23, 2020
View reviewed changes
...udder-core/src/main/scala/com/normation/rudder/services/healthcheck/HealthcheckService.scala Outdated
def name: CheckName = CheckName("Disk free space available")

final case class SpaceInfo(val partition: String, val free: Long, val available: Long){
def pourcentage: Double =
Copy link
Member

Choose a reason for hiding this comment

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

percent

@ElaadF ElaadF force-pushed the ust_18357/add_check_for_free_space branch from fc88cd4 to afd003f Compare October 25, 2020 00:03
@ElaadF ElaadF force-pushed the ust_18357/add_check_for_free_space branch from afd003f to b2b1ada Compare October 25, 2020 00:04
@ElaadF ElaadF force-pushed the ust_18357/add_check_for_free_space branch 2 times, most recently from b2f9f68 to 957398d Compare October 25, 2020 00:10
@ElaadF ElaadF force-pushed the ust_18357/add_check_for_free_space branch from 957398d to 1d281b1 Compare October 25, 2020 04:00
@ElaadF
Copy link
Member Author

ElaadF commented Oct 25, 2020

Commit modified

fanf
fanf approved these changes Oct 25, 2020
View reviewed changes
Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

WOUAH! The changes are striking since last time. Now, the logic is super clear and the code feels much shorter/concise. Nice work!

@fanf fanf added Ready for merge and removed WIP Use that label for a Work In Progress PR that must not be merged yet labels Oct 25, 2020
@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/3301
-- Your faithful QA
Kant merge: "Morality is not the doctrine of how we may make ourselves happy, but how we may make ourselves worthy of happiness."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/31578/console)

@fanf
Copy link
Member

fanf commented Oct 25, 2020

OK, merging this PR

@fanf fanf merged commit df988b5 into Normation:branches/rudder/6.2 Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanf fanf fanf approved these changes

@VinceMacBuche VinceMacBuche Awaiting requested review from VinceMacBuche

Assignees
No one assigned
Labels
qa: Can't merge Ready for merge Trigger test
Projects
None yet
Milestone
No milestone
4 participants
@ElaadF @Normation-Quality-Assistant @fanf @VinceMacBuche