Consume raw/generator unmerged dump data in MSUnmerged #12059
Conversation
|
Jenkins results:
|
amaltaro
force-pushed
the
msunmer-verbose
branch
from
00ad580
to
e2d5bb8
Compare
|
I have this patch applied to the production pod, named At least CNAF and JINR are right now failing with permission issues. |
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
The auth/authz issues that we had before were actually coming from a missing environment variable in the manage script (X509_USER_PROXY). Fix has been provided here: dmwm/CMSKubernetes#1532 On what concerns FNAL, I have been testing both the Lastly, in terms of network traffic pulling the unmerged dump for FNAL, we can see an outstanding improvement both in time and data. Here are the results: |
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
amaltaro
force-pushed
the
msunmer-verbose
branch
from
0e3e4ee
to
a62c363
Compare
|
Jenkins results:
|
amaltaro
force-pushed
the
msunmer-verbose
branch
from
a62c363
to
f9aa0b8
Compare
|
Jenkins results:
|
amaltaro
force-pushed
the
msunmer-verbose
branch
from
f9aa0b8
to
e843c02
Compare
|
Jenkins results:
|
amaltaro
force-pushed
the
msunmer-verbose
branch
from
e843c02
to
a3fc592
Compare
|
Jenkins results:
|
|
Jenkins results:
|
amaltaro
force-pushed
the
msunmer-verbose
branch
from
a8f91ee
to
1493d18
Compare
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
amaltaro
changed the title
filterUnmergedFiles method no longer exists Fix check for isDeletable Fix key name for dirsDeletedFail check if ctx object exist before freeing it
temporarily remove integration tag for unit tests fix RucioConMon unit test fix MSUnmerged unit tests resolve MSUnmerged unit tests
amaltaro
force-pushed
the
msunmer-verbose
branch
from
104f09b
to
f01c556
Compare
|
Jenkins results:
|
|
I might have to do some polishing, but I think the bulk logic is in place now and I welcome any feedback. |
| @@ -0,0 +1,42 @@ | |||
| #!/usr/bin/env python | |||
| import logging | |||
There was a problem hiding this comment.
SELF-REMINDER: This script will be removed before merging this PR.
There was a problem hiding this comment.
The code seems very complex to me and very hard to follow. As such to properly define its logic someone needs to dig very deep into code flow. I suggest to further refactor the code into smaller functions with clear scope, e.g. if function is given an fobj which can be either a file or dir, the code can be re-organized to use concurrent patter on nested dir structure. The function behavior can be well defined with respect to its action, e.g.
- for directory get list of its content and call itself
- for a file, unlink the file and return the status
Without knowing exact logic how clean-up procedure should be done, which exceptions should be made, the review of the logic is very cumbersome.
| if dirLfn in rse['dirs']['deletedSuccess']: | ||
| self.logger.info("RSE: %s, dir: %s already successfully deleted.", rse['name'], dirLfn) | ||
| continue | ||
| for idx, dirLfn in enumerate(rse['dirs']['toDelete']): |
There was a problem hiding this comment.
As far as I can tell the toDelete parts of the rse['dirs] object is never cleaned up even though the logic below deletes dirs/files.
There was a problem hiding this comment.
Instead of loop I suggest to clearly define function(s) to delete an object. The function can take fobj (either dir or file) and perform the operation and return the status. Then code can be parallelized (to speed up nested operations) and deletion can be done concurrently.
| @@ -306,9 +304,24 @@ def _execute(self, rseList): | |||
| def cleanRSE(self, rse): | |||
| """ | |||
| The method to implement the actual deletion of files for an RSE. | |||
| Order of deletion attempts is: | |||
There was a problem hiding this comment.
It would be beneficial to clearly define logic of deletion, here you outline the steps instead of logic. But the underlying code seems very complex with different rules of what and how it should delete. I suggest to provide description of such logic in the doc string.
|
Minor comment: the code relies on class methods/functions with underscores. I expect that those should be protected methods or not directly accessible, but python does not provide such isolation and notations neither enforce it. I don't know the rationale behind this notations. |
|
I appreciate your code review, thanks Valentin! Making this code parallel is out of scope, the main issue to be addressed is memory consumption and removal optimizations where possible. As you stated, it is already very complex and I would rather not make further changes to increase it even more. I will see how to break some implementations into smaller functions/methods and will also provide a diagram for data removal. |
|
hi @amaltaro, in regards to:
May I ask two questions.
Let me put it that way, I was surprised to see you refactoring the logic of the service starting from an issue related to creating a streamer to feed the component. This messes things a lot and we could never see the benefits or even the needs of this. Such efforts should be separated. As we know in the team we always requested high issue granularity and to split effort by subject. I've been constantly flagged to put code changes in a PR with this very same argument. So in order to continue with this my suggestion would be to split the changes thematically in two issues:
Then we measure CPU && Memory consumption (measuring only one of the parameters gives no clear picture) before and after applying the patches to a standalone instance of the service/component (definitely not in Kubernetes), this is the only way to see the actual effect. And I stand behind my words on this - we cannot measure component resource consumption only by monitoring Kubernetes cluster behavior. We should profile the code in much better details - which was exactly what I was doing constantly on every change before we merge for at least a month back then when I was including these optimizations in the code. |
|
one more thing @amaltaro
This is already in place. If you are simply changing the way how to do it with this PR, that's another story, but this is definitely nothing new for MSUnmerged |
|
@todor-ivanov thank you for looking into this.
I need to update that comment, because what it is actually doing right now is:
I will update the PR description with this. I also need to revisit what was already in place and see the differences.
I don't think it makes sense to separate those developments. Having a generator object is of no help if we actually load the data into memory. So implementing a generator and consuming the generator properly has to be bound. Otherwise we have the same faith of the compressed RucioConMon feature, which was implemented but never adopted in MSUnmerged. Said that, my goal was indeed just to consume a generator, but memory footprint was still extremely high because of the parsing and assignment of directories and files to data structures. For the CPU and memory footprint, I actually performed an isolated study with RucioConMon only, which was actually discussed in this transient PR: I think it should be simple enough to run the current MSUnmerged only with those changes, versus those changes + modified MSUnmerged. I will try to get back to this next week. |
All of this is already in place .... I do not see a point of rewriting this logic.... Well if you like it your way .. of course there is a point. |
|
What needs to be done is:
Let me rephrase with some more details:
|
|
But,... if we strictly insist on keeping the dependency on RucioConMon.... ok, then we should download the file or stream it (our choice) , parse it in a separate thread to the level of granularity our service speaks and record it in a shorter file wich would reduce the size from Gbs to Mbs. And this logic is already in the current implementation of MSUnmerged, What I suggest here is simply to exchange one resource with the other - swap memory to i/o expenses. What should be the action item to achieve this with minimal effort would be to transform the MSUnmerge service in Producer consumer mode which we already very well know. And the current code should not be difficult to be split in two pieces without heavy logic rewriting - one thread to parse and preserve the file with reduced contents, one thread to consume it benefiting from all the possible resource optimizations that come out of this. |
|
If we don't rely on RucioConMon, how do we know which directories have been created in which storage? Are you saying that the scanning logic implemented in Rucio should become part of WMCore? I hope not! Second, we have no choice but go down to the file level once a directory fails to be deleted (which many times it could be because it has too many content in it, from what I have seen in the logs). About your last bullet, I totally agree that we have no way to communicate issues with the sites. However, we are responsible for the MSUnmerged functionality. Everyone has to take up responsibility in this process. |
We are already doing worse .. Once we fail a deletion we start all of this process recursively on a depth first logic && remotely..... which is a disaster. So What I suggest is to do this scan ourselves, but to stop at level -3. The level at which our services talk and never dive deeper. This is a fairly fast process and easy, because at that level of the tree the amount of information is still in the lower (almost linear) part of the exponent.... this is something we can confidently manage ourselves. And as I said we are already doing it .... much much much deeper..... so this needs no proof of concept. |
|
We only scan deeper, not to the top level directories. In addition, we only scan in case of failures, because otherwise there is no way we can delete files. Just to make sure we are speaking the same language, an example of file to be deleted would be: Are you saying that we need to list content up to I feel like we are deviating from the original goal of this issue, which is:
We can have the redesign of MSUnmerged discussion in a different moment. For now, let us try to resolve this one issue please. |
Yes we have. It was announced that all sites are now supposed to be using WEBDav, which btw in the past has hit us heavily, and exactly in that context I did some research and the outcome was that we could be able to confidently and rightfully ask the recursive deletions to be supported from the sites (similarly to that we ask the write permissions at the unmerged area by certificate role) and we should not own all the protocol level complexities. All of which were supposed to be encapsulated and isolated from us by using |
Your code change does not resolve an issue. With your code change you are already redesigning the service, but without having this discussion which I intentionally triggered here. |
|
I guess you didn't see the memory plots I provided in here then: I will try to get the performance measurements you requested at some point next week. Thanks |
|
about this:
I may be citing the wrong tree level... (could be -4 or close), because I do not remember it by heart. The point is, we should simply check the depth of the |
Alan, I saw them.... I have been running similar tests hundreds of times.... I said it few comments above. If we insist to keep depending on RucioConMon, be it, but lets separate it in a different thread. And running any test on the current code status would be speculative.... because you did not isolate the changes in different steps so it would be difficult to attribute the results either to some code optimizations or because you changed the way how you feed the service. So they would not be a justification of you rewriting the rest of the logic of the service. |
| results = json.loads(results) | ||
| return results | ||
| with self.refreshCache(cachedApi, apiUrl, decoder=True, binary=False) as istream: | ||
| results = istream.read() |
There was a problem hiding this comment.
NOTE: I don't know how much data istream reads, but here you have two potential memory spikes, one is reading data from istream and another to parse the json. Instead, I suggest to use
return json.load(istream)
which performs both steps as one, i.e. JSON can read from file object directly.
Fixes #12061
Status
In development
Description
The scope of this PR has inflated quite a lot, as I started just investigating the adoption of compressed RucioConMon data into MSUnmerged. Summary of changes are:
MSUnmerged.filterUnmergedFilesmethod - logic now embedded ingetUnmergedFilesand_isDeletablecleanRSEmethod by first trying to delete the whole directory. If it fails, then list all the content in that directory and delete its content by slices. Finally, try to remove the (now) empty directory._listDirto list root files (only) inside a directorytest_gfal.pyto test directory removal with gfal (simulating same similar behavior as MSUnmerged)Is it backward compatible (if not, which system it affects?)
YES
Related PRs
Gzipped support was added with this PR:
#11142
but never adopted by MSUnmerged. With this PR, we actually adopt it.
External dependencies / deployment changes
Gzipped data is not currently functional: