Skip to content
This repository has been archived by the owner on Jan 20, 2023. It is now read-only.

[init] Cleanup in parallel. #63

Closed

Conversation

monaka
Copy link
Member

@monaka monaka commented May 24, 2019

The current init-plugin-broker container deletes /plugins/* in serial.
It is too slow to clean-up within the timeout when users installed more than 2 plugins.

I checked roughly by a simple script. The patched init-plugin-broker will be about 40% faster.

https://gist.github.com/monaka/5871f3c37296dfe46aed5a9999083f8a

What does this PR do?

Clean-ups /plugins directory in parallel.

What issues does this PR fix or reference?

No.

@monaka monaka added the enhancement New feature or request label May 24, 2019
@monaka monaka requested a review from amisevsk as a code owner May 24, 2019 07:34
@codecov
Copy link

codecov bot commented May 24, 2019

Codecov Report

Merging #63 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #63   +/-   ##
=======================================
  Coverage   66.37%   66.37%           
=======================================
  Files           6        6           
  Lines         565      565           
=======================================
  Hits          375      375           
  Misses        165      165           
  Partials       25       25

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 3ef07c7...f26efef. Read the comment docs.

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

I have a few issues here:

  1. I'm not clear on why this would work at all -- how much multiprocessing can the init broker even do, given its tiny quotas?
  2. Even the improved 25 second time is far too slow. If the init broker is regularly taking this long it suggests a far more fundamental problem somewhere along the line.
  3. How does the init broker compare against a bare rm -rf /plugins/*?
  4. Where was the rough speed test conducted? Minishift or on a actual cluster? What storage is backing the /plugins volume?

broker.PrintInfo("WARN: failed to remove '%s'. Error: %s", file, err)
}
}
recursive(nil, broker, "/plugins", 5)
Copy link
Contributor

Choose a reason for hiding this comment

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

A better name mind be cleanDirectory, recursive is too nondescript (says how it does something but not what it does).

@monaka
Copy link
Member Author

monaka commented May 25, 2019

I'm not clear on why this would work at all -- how much multiprocessing can the init broker even do, given its tiny quotas?

At the first, Goroutines will work as a lightweight thread. Less related on multiprocessing.

Almost all I/O systemcalls require long turn-around-time by physical device. So contemporary filesystems are run itself under multi-thread for their better throughput.
And the application can get a benefit by multi-threaded FS if it is run under multi-thread.

This patch is for getting the benefit.

Even the improved 25 second time is far too slow. If the init broker is regularly taking this long it suggests a far more fundamental problem somewhere along the line.

The benchmark test is run under Google Cloud Shell. Users share same CPUs and disks.
So it can be under high system load. In other words, it can be occurred in the real deployment of Che.

Where was the rough speed test conducted? Minishift or on a actual cluster? What storage is backing the /plugins volume?

The test bed is on Google Cloud Shell. Probably GKE disk.

I'll try to compare rm -fr * later.

@monaka
Copy link
Member Author

monaka commented May 25, 2019

@amisevsk it might be off-topic but. I found an issue about os:RemoveAll filed in the Golang official. golang/go#29921 It might be the room for consideration we rely on that method.

@monaka
Copy link
Member Author

monaka commented May 25, 2019

Err... I tried to test on the Azure VM instance. I got WARN log lines like this

 Cloning into 'che'...
remote: Enumerating objects: 953, done.
remote: Counting objects: 100% (953/953), done.
remote: Compressing objects: 100% (265/265), done.
remote: Total 483925 (delta 566), reused 771 (delta 509), pack-reused 482972
Receiving objects: 100% (483925/483925), 104.19 MiB | 23.46 MiB/s, done.
Resolving deltas: 100% (213519/213519), done.
Checking out files: 100% (11818/11818), done.
2019/05/25 07:28:38 Broker configuration
2019/05/25 07:28:38   Runtime ID:
2019/05/25 07:28:38     Workspace: dummy
2019/05/25 07:28:38     Environment: dummy
2019/05/25 07:28:38     OwnerId: dummy
2019/05/25 07:28:38 Starting Init Plugin Broker
2019/05/25 07:28:38 Cleaning /plugins dir
2019/05/25 07:28:39 WARN: failed to remove '/plugins/che'. Error: unlinkat /plugins/che: permission denied
2019/05/25 07:28:39 WARN: failed to remove '/plugins/che1'. Error: unlinkat /plugins/che1/.mvn/jvm.config: permission denied
2019/05/25 07:28:40 WARN: failed to remove '/plugins/che2'. Error: unlinkat /plugins/che2/.mvn/jvm.config: permission denied

when I call this script. the docker image is from the master branch. Not patched.

#!/bin/bash

(cd ~/plugins && rm -fr * && \
 git clone https://github.com/eclipse/che.git && chmod -R 777 che && \
 for i in {1..2}; do cp -R che che$i; done)
time docker run --rm -it -v ~/plugins:/plugins init:latest -disable-push -runtime-id dummy:dummy:dummy

Guess related on the issue I introduced above.

How can I send issues for che-plugin-broker? I cloudn't find the issue tab here.

Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>
@monaka
Copy link
Member Author

monaka commented May 26, 2019

@amisevsk I run my rough benchmark on Azure VM. The result is https://gist.github.com/monaka/469820873b00362e5065e5893b07bc43 . The master branch is not too slow than rm -r but 2 times or more slower than my patched version. This result may depend on the CPU resource. And this patch will be enough effective on CPU core rich environments.

BTW, I can't see why the original author didn't do the speed test rm -r vs init-plugin-borker. He must do it if I must report the performance test for requesting this PR.

@amisevsk
Copy link
Contributor

@monaka We're currently placing plugin broker issues in the upstream eclipse/che repository.

Thanks for your work, I'll take another look later today.

@amisevsk
Copy link
Contributor

Sorry I never got to this @monaka -- this PR is no longer applicable, since we've moved to caching extensions in #86.

If you run into the same issue as before, please let me know.

@amisevsk amisevsk closed this Jan 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants