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

[pkg] panic if no space is left #3636

Merged
merged 15 commits into from
Oct 13, 2022
Merged

Conversation

huof6829
Copy link
Contributor

Description

Fixes #3629

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • [] Code refactor or improvement
  • [] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
  • [] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • [] make test
  • [] fullsync
  • [] Other test (please specify)

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • [] My code follows the style guidelines of this project
  • [] I have performed a self-review of my code
  • [] I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • [] My changes generate no new warnings
  • [] I have added tests that prove my fix is effective or that my feature works
  • [] New and existing unit tests pass locally with my changes
  • [] Any dependent changes have been merged and published in downstream modules

@huof6829 huof6829 requested a review from a team as a code owner September 13, 2022 10:09
@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Merging #3636 (b6bbdd2) into master (e9732a1) will decrease coverage by 0.28%.
The diff coverage is 80.72%.

@@            Coverage Diff             @@
##           master    #3636      +/-   ##
==========================================
- Coverage   74.95%   74.67%   -0.29%     
==========================================
  Files         269      267       -2     
  Lines       23819    23784      -35     
==========================================
- Hits        17854    17761      -93     
- Misses       5039     5090      +51     
- Partials      926      933       +7     
Impacted Files Coverage Δ
db/config.go 100.00% <ø> (ø)
db/trie/mptrie/node.go 100.00% <ø> (ø)
ioctl/newcmd/action/action.go 79.62% <ø> (+0.09%) ⬆️
ioctl/newcmd/node/nodedelegate.go 70.19% <0.00%> (-0.95%) ⬇️
ioctl/newcmd/node/nodeprobationlist.go 90.90% <0.00%> (-4.33%) ⬇️
server/itx/server.go 60.00% <25.00%> (-2.21%) ⬇️
action/protocol/execution/evm/evm.go 44.57% <26.66%> (-0.86%) ⬇️
db/trie/mptrie/hashnode.go 36.36% <41.17%> (-54.55%) ⬇️
db/trie/mptrie/merklepatriciatrie.go 78.73% <46.80%> (-12.70%) ⬇️
db/db_bolt.go 72.13% <50.00%> (-0.39%) ⬇️
... and 23 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more


// checkDiskSpace returns the disk usage info
func checkDiskSpace(ctx context.Context) {
usage, err := disk.Usage("/")
Copy link
Member

Choose a reason for hiding this comment

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

if os disk partition like this

/          /dev/sda
/run       /dev/sdb

and iotex-core run within /run, the check may not work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"/" checks all disks

Copy link
Contributor

Choose a reason for hiding this comment

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

@huangzhiran +1

/    /dev/sda   500G
/run /dev/sdb 100MB

if run on /run, it will failed

continue
}
// check disk space per write operation
if ev.Has(fsnotify.Write) {
Copy link
Member

Choose a reason for hiding this comment

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

fsnotify.Create needed too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still left enough space to run program even if Avail is 0 after run df -h .

}
// check disk space per write operation
if ev.Has(fsnotify.Write) {
checkDiskSpace(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

all filesystem write ops trig a check, may inefficient

@millken
Copy link
Contributor

millken commented Sep 14, 2022

I think we don't need to monitor disk, just need to PANIC when write DB error is disk full

@huof6829
Copy link
Contributor Author

add log rotate and delete expired logs can be better.

select {
case <-ctx.Done():
return
case ev, ok := <-watcher.Events:
Copy link
Member

Choose a reason for hiding this comment

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

when does an event come

if err := task.Start(ctx); err != nil {
log.L().Panic("Failed to start watch disk space", zap.Error(err))
}
return func() {
Copy link
Member

Choose a reason for hiding this comment

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

why return a func() here?

server/main.go Outdated
@@ -103,6 +104,10 @@ func main() {
glog.Fatalln("Cannot config global logger, use default one: ", zap.Error(err))
}

// check device
stopWatch := watch.Start(ctx, 5*time.Minute)
defer stopWatch()
Copy link
Member

Choose a reason for hiding this comment

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

just call watch.Start() in server.Start(), and call watch.Stop() in server.Stop()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just check like heartbeat. server contains the major components like api blockchain etc...

// permitted by law, all liability for your use of the code is disclaimed. This source code is governed by Apache
// License 2.0 that can be found in the LICENSE file.

package watch
Copy link
Member

@Liuhaai Liuhaai Sep 27, 2022

Choose a reason for hiding this comment

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

package disk

)

// Start creates a timer task to check device per watchInternal
func Start(ctx context.Context, watchInternal time.Duration) func() {
Copy link
Member

Choose a reason for hiding this comment

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

func NewMonitor(time.Duration) *monitor
func (*monitor) Start()
func (*monitor) Stop()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

return
}
// panic if left less than 2%
if usage.UsedPercent > 98.0 || usage.InodesUsedPercent > 98.0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

if disk is 20TB, when there is %2=200GB of free disk, can't it be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

leave enough disk space is required when disk run out soon.


// checkDiskSpace returns the disk usage info
func checkDiskSpace(ctx context.Context) {
usage, err := disk.Usage("/")
Copy link
Contributor

Choose a reason for hiding this comment

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

@huangzhiran +1

/    /dev/sda   500G
/run /dev/sdb 100MB

if run on /run, it will failed

@CoderZhi
Copy link
Collaborator

@huof6829 @millken has the problem been resolved?

@huof6829
Copy link
Contributor Author

fixed. I have no problem.

@sonarcloud
Copy link

sonarcloud bot commented Oct 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@huof6829 huof6829 merged commit 62e8eb7 into iotexproject:master Oct 13, 2022
@huof6829 huof6829 deleted the panic_no_space branch October 13, 2022 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic if no space is left
6 participants