Fix Sync of template.properties in Swift #1331
Conversation
|
@syed thanks for picking this up. Can you
|
| protected boolean swiftUploadMetadataFile(SwiftTO swift, File srcFile, String containerName) throws IOException { | ||
|
|
||
|
|
||
| //create a template.properties for Swift with its correct unique name |
There was a problem hiding this comment.
Since you went through the trouble of cleaning up some of the code, how about you improve it some more by turning this comment into a javadoc comment for this method, also describing the parameters involved.
If you like this idea, I'd suggest going ahead and writing another javadoc comment for the copyFromNfsToSwift method.
Little things like these can go a long way to improve code readability. :)
There was a problem hiding this comment.
Thanks for the suggestion. I finally was able to get some time to work on this again. I'll send a new PR soon with your comments.
|
@cristofolini are you aware that you can write a PR for a PR? you can find the example of checking out a pr in tools/git and after that you can just PR to the branch from which the original PR was created. |
|
@DaanHoogland That's a really interesting feature which I wasn't aware of. I'm afraid I'll be out of commission for most of today but I'd love to try it out as soon as I can get my hands on a computer. |
|
@DaanHoogland @syed I've opened a pull request containing some simple javadocs to @syed's swift-restart-fix branch. |
| } else if (destData.getObjectType() == DataObjectType.VOLUME) { | ||
| VolumeObjectTO newVol = new VolumeObjectTO(); | ||
| newVol.setPath(containerName); | ||
| newVol.setSize(srcFile.length()); |
There was a problem hiding this comment.
Not 100% related to this PR, but are the sizes on volumes and snapshots correct?
There was a problem hiding this comment.
Good catch @pdube. That should be the virtual size (not the physical)
|
Code LGTM, as a general comment though, I think it is cleaner to be as precise as possible with exception handling. |
|
Jira ticket : CLOUDSTACK-9247 I've tested this on our local setup with swift as a secondary storage. I create a template from a snapshot and restart the management server. I don't see the problem where the template went back to not ready. |
|
Code looks good, but I have no way of testing this since I don't have Swift... |
|
Tested on local setup, the template.properties is now uploaded correctly LGTM |
|
tested, I think this should be ported into master as well. |
|
Should Jenkins and CI checks be succeeding or is it because the tests do not have access to a Swift instance to test and succeed? The code looks good to me. I have not tested because I do not have access to a different Swift installation than the others who have tested already. |
|
Fixed |
|
@DaanHoogland I'm not sure why the tests are failing. My changes shouldn't have affected that |
syed
force-pushed
the
swift-restart-fix
branch
from
145f441
to
ce1eaa1
Compare
|
@syed: Thanks a lot for your fix. We are facing the same issue with CS4.8 and NFS secondary storage. Templates go into "Not Ready" when restarting the management server. Did you had the chance to verify what causes the job to fail? |
|
Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 ACS CI BVT RunSumarry: Failed tests:
Skipped tests: Passed test suits: |
|
@kollyma This was happening for swift because the |
|
@syed: thanks for the feedback. Here a more detailed description, based on your comments. Entry on the management server: Logs after management server restart: You are referring to the path set incorrectly in template properties. The cloudstack logs says "Template Sync did not find..." Do we have the same issue ? the template file is present on the nfs: template/tmpl/2/225# ls |
|
@syed can you squash the commits please and do a Do you have any feedback for @kollyma if he is seeing a similar issue as what you fixed here for swift? it would be worth understanding if the problem exists in more than one place so we can better understand if we need another PR for NFS storage as well. |
|
Hi, would you mind extracting to a method lines 606-612? They are duplicated at line 627. |
|
@rafaelweingartner Done. Although I didn't create any test cases yet. There is nothing much to test. The function doesn't throw any execptions, doesn't expect any results so I think this should be good. @swill I've rebased again so it is still a single commit |
|
@syed, you could delete the line 631, or use it as a java doc that explains the method. I believe that there is, at least, one test you can do here. You could test if an exception happens while calling the “execute” method with the “deleteCommand” variable. If an exception happens it cannot be re-thrown by this method, and it has to be logged as a debug message. |
|
@rafaelweingartner Updated the comment as a javadoc. For the test. I am not sure what the best strategy is. Maybe you can help me understand better. For me this method should be private and I don't know how the community feels like testing private methods. |
|
@syed, you can change the method to be protected. To write a test for this method, you can use Mockito; you can use the spy method into the object that you want to test (NfsSecondaryStorageResource), then you can force an exception to be thrown when the "execute" method is called with “deleteCommand” object. This way, if someone changes the logic and let the exception be re-thrown, the test case will catch. Also, if someone silences the exception, the test case will catch. If you have doubts/problems when writing the test case, just call me. |
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.cloudstack.storage.resource; |
There was a problem hiding this comment.
Since there is nothing about that is specific to secondary storage, Why not place this class in a more general package such as com.cloud.test.utils?
|
@jburwell, @syed, sorry the long post, I did a research on a few things and I would like to share with you guys. I was just looking at the state of this PR, and I noticed that the number of lines added jumped to 300+; most of those lines are needed in order to write a test case using the “TestAppender” approach. Additionally, I was curious when you mentioned that the use of “static final” delimiters could optimize the GC, given that it (the GC) would not check if those attributes have or not to be garbage collected. I had never heard that before, so I tried to find something online. If you have some reliable reference about that, could you share? I tried to find some specs or guideline documents from either Oracle or OpenJDK without much success. However, I found something like you said to the android JVM [1], but that would not be our case. Then, I read some articles (not scientific ones) from IBM [2] and Oracle [3] about the tuning of java code. But still, they did not mention anything related to what you said. Then, I decided to revisit a forum that I did not visit for a long time, since the time of my first Java certification (I am definitely getting old :( ), the coderanch [4]. There I found something that may be related to what you said [5]. There was a discussion there about the GC and static variables; at some point, someone highlights that “Static variables are destroyed when the class is unloaded”. After that, I also found this [6] on Stack overflow, in which it is described that “Static variables cannot be elected for garbage collection while the class is loaded. They can be collected when the respective class loader (that was responsible for loading this class) is itself collected for garbage.”. I believe that was the point you wanted to make, right? Static variables are not GCed by the GC, they are not even checked. If that was the case, it would have nothing to do with the “final” keyword per se, but rather with the use of the “static” keyword. After having said that, perhaps we could benefit from both worlds? I mean we could still use the “Logger” variable as static, but not final; then, we would be able to write a test case using Mockito (as the first example I presented to @syed), which would add less code. Additionally, I had taken a look at some spring framework code. Their framework is not only the base of ACS but also many other huge projects; so, I thought it would be interesting to see how they use the logger variables. They use their “logger” attributes as Object variables and not Classes. With that approach when you extend their code, you “get for free” a logger instance to be used. I believe that is why I am so used to loggers being object attributes. I might have been working too much with their components and frameworks. When we do that, we can avoid the following example that happens in ACS: I did some tests, and the approximated size of a Logger instance is ~820 bytes. So, if we save the instantiation of a few of this we can reduce a little bit of the use of ACS memory, giving that due to the way we create “Logger” objects today, we have an instance even for classes that are not object per se, but intermediated classes in a hierarchy of singletons. [1] http://developer.android.com/training/articles/perf-tips.html |
|
@rafaelweingartner yes, as I have said, Personally, I use Mockito (and its ilk) sparingly. Namely, I only use it in circumstances where language constructs are unable to mock a mechanism. First, I find it more difficult to understand the intentions of a test using Mockito rather than pure Java as it layers another set of abstractions onto the test. Second, Mockito approaches discourage reuse. We could use Mockito to mock logging tests in this case. However, we have done nothing to make testing of expected logging easier for future test cases. It's most important to me that we do not start turning references to constant values into instance variables due to potential GC churn. It's less a memory size issue than it is about the length of GC operations and CPU overhead. The Mockito vs. |
|
I do not find them (final static) purely cosmetics (we are using too much this expression lately). I understand its use, as I presented the references about the GC and static variables. Maybe we could use only the static keyword? That is what I am saying. The final keyword would work more like an assurance, so no one can change that reference during runtime. But, I believe we do not need such guarantees, right? I could not find anything (specs or guides from Oracle or OpenJDK) saying that the "final" keyword would provide such improvements. Maybe you could help me find something to clarify my doubts? About logging from static methods, I do not find that argument appealing. Even though static methods may be appealing, they are easier to use and code, not needing to integrate into a framework life cycle to wire all of the dependencies. I believe that if we have an “util” class with static methods, then it (the util class) will have a static variable “Logger”; now, mixing static methods into a singleton; I see that as an anti-pattern. This is very personal, I like to delegate very specific tasks to each class. I believe that facilitates the design of the software and the writing of test cases (both unit and integration one) I understand your feelings about Mockc. I had the same some time ago when I started working with TDD. At that time I used Easymock. But still, the point is that, once we start writing test cases (the unit ones), we get into a moment in which we have to write integration tests. That means a method that uses few methods that are self-contained and unit tested. Therefore, we do not need to test the whole method (we already assured that the units are working), but only check if the method is calling the methods (units) it should (checking the order), with the parameters that are expected and at the end returning what we expect it to return. To facilitate that job we would need to use Mocks, then we would not have to prepare complicated set ups to write our tests. About the overhead problems to the GC, as I said, I am talking about singletons, they are created only once in the whole application life cycle. Therefore, they would not cause overhead problems with the GC. |
|
@rafaelweingartner I apologize -- I did not understand that you were focused on Stepping back a bit, why are we investing so much time and effort to test a |
|
@jburwell, that is great that we understood each other ;) Your questions is a great one, why are we discussion this, and why testing that. Actually, I do not care much about the writing of test cases to check if something is logged. The point is that there is that “cleanupStagingNfs” method. If we want to write a nice test to it, we would have to test the happy day flow; that means if the method is creating the “DeleteCommand” object and then if it call the “execute” method with that object. That is one test case. I see that kind of test as an integration test; meaning, a test if a method is using some other method. Having said that, I see another flow of execution; we might have an exception. So, if an exception occurs, what happens? By the code, today we log the exception and the execution continues. Then we would have to ask ourselves, do we want to enforce that? If someone silences that exception, would we like to catch that? If someone re-throws the exception, using a runtime one; would we like to detect that? At first sight, I think that we should guarantee both executions flows. But, if it becomes a burden, we can let it go. |
|
@rafaelweingartner @jburwell Sorry for the late reply to this. I was on vacation and returned today. I've decided to use John's approach for the logging test as I feel it is more natural. Also the dicussion about I will sqash the commits as it looking pretty good now unless you guys have any more comments. |
|
@syed this is not compiling for me. Can you review? |
|
@swill the problem is because of |
|
@syed can you have a look at this one. I am not sure why, but I am having a lot of trouble with this PR. I have not been able to get a test run to actually works. I have tried to run CI on this 3 times and I always get the following: The fact that both Jenkins and Travis are failing may also be relevant. |
CI RESULTSAssociated Uploads
Uploads will be available until Comment created by |
|
@syed can you force push or close and reopen the PR to kick off travis and jenkins? I think this one seems to be in a pretty good state now... |
|
@syed failed again. :( |
|
@swill looks like jenkins failed because of some licence issue in one file. I've checked all the files that I have changed and every one seems to have the correct licence. I don't have access but is it possible to check |
|
@swill I got the file from jenkins and it points to |
|
https://builds.apache.org/job/cloudstack-pr-analysis/1323/artifact/target/rat.txt here is the link to the file |
|
@syed can you close and reopen to see if the error is persistent? |
|
@DaanHoogland sure ... doesn't hurt |
Fix Sync of template.properties in SwiftWhen using Swift as a secondary storage, we create a template.properties file which stores the metadata about the template. Currently the data which is present in the file is incorrect which leads to templates becoming unavailable after they are downloaded. This fix makes sure that the template.properties has the correct "path" set so that templates are available. I've also done a bit of cleanup and made the code bit more clean. * pr/1331: Fix Sync of template.properties in Swift Signed-off-by: Will Stevens <williamstevens@gmail.com>
When using Swift as a secondary storage, we create a template.properties file which stores the metadata about the template. Currently the data which is present in the file is incorrect which leads to templates becoming unavailable after they are downloaded. This fix makes sure that the template.properties has the correct "path" set so that templates are available.
I've also done a bit of cleanup and made the code bit more clean.