You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by syed <gi...@git.apache.org> on 2016/01/12 22:07:13 UTC
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
GitHub user syed opened a pull request:
https://github.com/apache/cloudstack/pull/1331
Fix Sync of template.properties in Swift
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/syed/cloudstack swift-restart-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1331.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1331
----
commit 1b9ff622f2b9a38e65e31c7f803fd8d283f5d36f
Author: Syed <sy...@gmail.com>
Date: 2015-12-14T22:37:28Z
Fix Sync of template.properties in Swift
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1331
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-212598453
@jburwell I agree with you. The key point here is
> instances that are rapidly allocated and deallocated
Singletons are not meant to be deallocated all the time.
I just created that commit to be used as an example for @syed; I have just done the same thing for the other method he created, giving that we are in very different time zones, and if we try to develop a more fine discussion and explanation here, I thought it could use too much of hist time. So, some examples for him would be easier to understand and grasp the idea of a test case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by kollyma <gi...@git.apache.org>.
Github user kollyma commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-202547429
@syed: thanks for the feedback. Here a more detailed description, based on your comments.
Setup: CS4.8, nfs secondary storage, kvm with local storage
```
# cat template.properties
uniquename=225-2-c7401a18-2163-3c22-9799-3cc9b79cb771
filename=eabd7703-4203-49c5-a3a3-f8234f2d1c23.qcow2
size=1753481216c22-9799-3cc9b79cb771
qcow2.filename=eabd7703-4203-49c5-a3a3-f8234f2d1c23.qcow2
qcow2.virtualsize=21474836480
public=true
id=1
```
Entry on the management server:
```
select * from vm_template where name = "myname2" \G;
*************************** 1. row ***************************
id: 225
unique_name: 225-2-c7401a18-2163-3c22-9799-3cc9b79cb771
name: myname2
uuid: cec0787b-5362-4e29-9ca6-63697ff8d872
public: 1
featured: 0
type: USER
hvm: 1
bits: 64
url: NULL
format: QCOW2
created: 2016-03-28 18:36:00
removed: NULL
account_id: 2
checksum: NULL
display_text: myname
enable_password: 0
enable_sshkey: 0
guest_os_id: 221
bootable: 1
prepopulate: 0
cross_zones: 0
extractable: 0
hypervisor_type: KVM
source_template_id: 215
template_tag: NULL
sort_key: 0
size: 21474836480
state: Active
update_count: 0
updated: NULL
dynamically_scalable: 0
```
Logs after management server restart:
```
2016-03-28 21:06:33,660 INFO [o.a.c.s.i.TemplateServiceImpl] (AgentConnectTaskPool-3:ctx-3eb6adca) (logid:9e020fbf) Template Sync did not find 225-2-c7401a18-2163-3c22-9799-3cc9b79cb771 on image store 1, may request download based on available hypervisor types
2016-03-28 21:06:33,661 INFO [o.a.c.s.i.TemplateServiceImpl] (AgentConnectTaskPool-3:ctx-3eb6adca) (logid:9e020fbf) Removing leftover template 225-2-c7401a18-2163-3c22-9799-3cc9b79cb771 entry from template store table
2016-03-28 21:06:33,668 INFO [o.a.c.s.i.TemplateServiceImpl] (AgentConnectTaskPool-3:ctx-3eb6adca) (logid:9e020fbf) Skip downloading template 225-2-c7401a18-2163-3c22-9799-3cc9b79cb771 since no url is specified.
```
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
eabd7703-4203-49c5-a3a3-f8234f2d1c23.qcow2 template.properties
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-211485538
I agree with you @syed, it is always good to discuss the patterns we use.
That example you brought up also works; it would do the same tricky to test, but using an appender stead of mocking. I am good with that too; I just coded differently, because I do not like much the idea of getting log entries by hand. But, I am willing to go either way. If you go the other way, please revert my commit, so the history does not get messed up.
For the new tests you have to write, you could mock the creation of the Object buffered writer, to one that you can control and check if the “write” method is been used as expected. In other words, writing the strings you expect to write.
Here there is an example of that for the File class (it is not the same, but it will give you an idea):
http://stackoverflow.com/questions/11849728/simulate-file-in-java
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-171276640
@syed thanks for picking this up. Can you
- link this to a ticket
- describe any testing you did (I can't test this)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-219163000
https://builds.apache.org/job/cloudstack-pr-analysis/1323/artifact/target/rat.txt here is the link to the file
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-219160595
@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 `/home/jenkins/jenkins-slave/workspace/cloudstack-pr-analysis/target/rat.txt` which has more info.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-219162095
@swill I got the file from jenkins and it points to `utils/testsmallfileinactive` ... I have no idea where this file came from
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-208034027
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by pdion891 <gi...@git.apache.org>.
Github user pdion891 commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-179568067
tested, I think this should be ported into master as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-208318739
@syed, you can change the method to be protected.
IMO every single method that has some logic in it should be tested without any discrimination among private and non-private ones.
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.
To check if the exception was treated properly you can use Mockito to mock the s_logger object and check if the “debug” method was called after the exception is thrown
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-173678575
Jira ticket : [CLOUDSTACK-9247](https://issues.apache.org/jira/browse/CLOUDSTACK-9247)
I've tested this on our local setup and it works as expected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r50446719
--- Diff: services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java ---
@@ -942,6 +931,83 @@ protected Answer copyFromNfsToS3(CopyCommand cmd) {
}
}
+ protected boolean swiftUploadMetadataFile(SwiftTO swift, File srcFile, String containerName) throws IOException {
+
+
+ //create a template.properties for Swift with its correct unique name
+ File uniqDir = _storage.createUniqDir();
+ String metaFileName = uniqDir.getAbsolutePath() + File.separator + "template.properties";
+ _storage.create(uniqDir.getAbsolutePath(), "template.properties");
+
+ String uniqueName = FilenameUtils.getBaseName(srcFile.getName());
+ File metaFile = new File(metaFileName);
+ FileWriter writer = new FileWriter(metaFile);
+ BufferedWriter bufferWriter = new BufferedWriter(writer);
+ bufferWriter.write("uniquename=" + uniqueName);
+ bufferWriter.write("\n");
+ bufferWriter.write("filename=" + srcFile.getName());
+ bufferWriter.write("\n");
+ bufferWriter.write("size=" + srcFile.length());
+ bufferWriter.write("\n");
+ bufferWriter.write("virtualsize=" + getVirtualSize(srcFile, getTemplateFormat(srcFile.getName())));
+ bufferWriter.close();
+ writer.close();
+
+ SwiftUtil.putObject(swift, metaFile, containerName, _tmpltpp);
+ metaFile.delete();
+ uniqDir.delete();
+
+ return true;
+ }
+
+
+ protected Answer copyFromNfsToSwift(CopyCommand cmd) {
+
+ final DataTO srcData = cmd.getSrcTO();
+ final DataTO destData = cmd.getDestTO();
+
+ DataStoreTO srcDataStore = srcData.getDataStore();
+ NfsTO srcStore = (NfsTO)srcDataStore;
+ DataStoreTO destDataStore = destData.getDataStore();
+ File srcFile = getFile(srcData.getPath(), srcStore.getUrl());
+
+ SwiftTO swift = (SwiftTO)destDataStore;
+
+ try {
+
+ String containerName = SwiftUtil.getContainerName(destData.getObjectType().toString(), destData.getId());
+ String swiftPath = SwiftUtil.putObject(swift, srcFile, containerName, srcFile.getName());
+
+
+ DataTO retObj = null;
+ if (destData.getObjectType() == DataObjectType.TEMPLATE) {
+ swiftUploadMetadataFile(swift, srcFile, containerName);
+ TemplateObjectTO newTemplate = new TemplateObjectTO();
+ newTemplate.setPath(swiftPath);
+ newTemplate.setSize(getVirtualSize(srcFile, getTemplateFormat(srcFile.getName())));
+ newTemplate.setPhysicalSize(srcFile.length());
+ newTemplate.setFormat(getTemplateFormat(srcFile.getName()));
+ retObj = newTemplate;
+ } else if (destData.getObjectType() == DataObjectType.VOLUME) {
+ VolumeObjectTO newVol = new VolumeObjectTO();
+ newVol.setPath(containerName);
+ newVol.setSize(srcFile.length());
--- End diff --
Good catch @pdube. That should be the virtual size (not the physical)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-201139141
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
### ACS CI BVT Run
**Sumarry:**
Build Number 126
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=2
Skipped=4
**Failed tests:**
* integration.smoke.test_guest_vlan_range.TestDedicateGuestVlanRange
* test_dedicateGuestVlanRange Failed
* integration.smoke.test_volumes.TestCreateVolume
* test_06_download_detached_volume Failed
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso
**Passed test suits:**
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData
integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
integration.smoke.test_snapshots.TestSnapshotRootDisk
integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by pdube <gi...@git.apache.org>.
Github user pdube commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-179502626
Tested on local setup, the template.properties is now uploaded correctly LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-214529726
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-212907833
@jburwell @rafaelweingartner I've added the test for checking the logged message using the gist provided by @jburwell. I've also added a `log4j.xml` because the test was not running without one. The test now fails to assert the message in `assertMessagesLogged`. @jburwell am I using this correctly?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-212596332
@rafaelweingartner the garbage collection issue is not the allocation/deallocation, but the reachability calculation that the garbage collector must perform for every instance member (i.e. determining whether or not that instance needs to be collected). Since it, ultimately, the logger is a singleton, this reachability check is completely unnecessary. In contrast, the garbage collector skips this check for ``static final`` members as it is guaranteed to be reachable for the lifetime of the JVM. While the overhead of this check is relatively small, making logger (or any other constant values) instance members on classes whose instances are rapidly allocated and deallocated accrues quickly.
@syed Done properly, the appender approach can be made reusable for other test cases to verify logging behavior. I built a [custom appender](https://gist.github.com/jburwell/98b6d94c57f409b5b778ffee6a8a12b1) that allows monitoring of a logger for one or more patterns and assert that logging occurred as expected. The following is an example of how to use it in this test case:
```
public class NfsSecondaryStorageResourceTest {
private TestAppender testAppender;
@Before
public void setup() {
testAppender = new TestAppenderBuilder().addExpectedPattern(Level.DEBUG, "Failed to clean up staging area:\*+").build();
Logger logger = Logger.getLogger(NfsSecondaryStorageResource.class);
TestAppender.safeAddAppender(logger, testAppender);
}
@Test
public void testSwiftWriteMetadataFile() throws Exception {
// test operations and asserts
testAppender.assertMessagesLogged();
}
```
The class likely needs a little work, and definitely, some testing. I put it together quickly to demonstrate a reusable approach to testing logging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-211995051
@rafaelweingartner I've added the test mocking the `BufferedWriter`. When I tried to run the tests I found that the test required `agent.properties` and `log4j.xml` which I am not sure where they are. Also something that I found was that tests for this module (`cloud-secondary-storage`) are disabled [see here](https://github.com/apache/cloudstack/blob/master/services/secondary-storage/server/pom.xml#L30) and from the history it appears that this `SkipTests` was present from the beigninging.
Any clues on fixing it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by kollyma <gi...@git.apache.org>.
Github user kollyma commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-200386681
@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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-180438974
@swill @syed please have a look at : services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java:941: Line has trailing spaces.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-207676112
@swill I've squashed the commits
@kollyma I'm sorry I missed your message. Do you still see a problem? Something strage that I see in your output is the `size=1753481216c22-9799-3cc9b79cb771` are you sure this is the case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-219073755
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 0
Errors: 0
Duration: 4h 18m 48s
```
**Associated Uploads**
**`/tmp/MarvinLogs/DeployDataCenter__May_12_2016_21_43_23_1TT3ZU:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/DeployDataCenter__May_12_2016_21_43_23_1TT3ZU/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/DeployDataCenter__May_12_2016_21_43_23_1TT3ZU/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/DeployDataCenter__May_12_2016_21_43_23_1TT3ZU/runinfo.txt)
**`/tmp/MarvinLogs/test_network_R3P7FB:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_network_R3P7FB/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_network_R3P7FB/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_network_R3P7FB/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_HKWN9Z:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_vpc_routers_HKWN9Z/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_vpc_routers_HKWN9Z/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1331/tmp/MarvinLogs/test_vpc_routers_HKWN9Z/runinfo.txt)
Uploads will be available until `2016-07-13 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by pdube <gi...@git.apache.org>.
Github user pdube commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-172715155
Code LGTM, as a general comment though, I think it is cleaner to be as precise as possible with exception handling.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r60742612
--- Diff: services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java ---
@@ -18,91 +18,68 @@
*/
package org.apache.cloudstack.storage.resource;
-import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileNotFoundException;
-import java.io.IOException;
-import java.net.URI;
-import java.util.Map;
-import java.util.Properties;
-
-import javax.naming.ConfigurationException;
-
-import junit.framework.Assert;
-import junit.framework.TestCase;
-
+import org.apache.cloudstack.storage.command.DeleteCommand;
+import org.apache.cloudstack.storage.to.TemplateObjectTO;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;
+import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mockito;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
-import com.cloud.utils.PropertiesUtil;
-import com.cloud.utils.exception.CloudRuntimeException;
+import java.io.BufferedWriter;
+import java.io.FileWriter;
+import java.io.StringWriter;
-public class NfsSecondaryStorageResourceTest extends TestCase {
- private static Map<String, Object> testParams;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
- private static final Logger s_logger = Logger.getLogger(NfsSecondaryStorageResourceTest.class.getName());
+@RunWith(PowerMockRunner.class)
+public class NfsSecondaryStorageResourceTest {
- NfsSecondaryStorageResource resource;
+ private NfsSecondaryStorageResource resource;
@Before
- @Override
- public void setUp() throws ConfigurationException {
- s_logger.setLevel(Level.ALL);
+ public void setUp() {
resource = new NfsSecondaryStorageResource();
- resource.setInSystemVM(true);
- testParams = PropertiesUtil.toMap(loadProperties());
- resource.configureStorageLayerClass(testParams);
- Object testLocalRoot = testParams.get("testLocalRoot");
- if (testLocalRoot != null) {
- resource.setParentPath((String)testLocalRoot);
- }
}
@Test
- public void testMount() throws Exception {
- String sampleUriStr = "cifs://192.168.1.128/CSHV3?user=administrator&password=1pass%40word1&foo=bar";
- URI sampleUri = new URI(sampleUriStr);
-
- s_logger.info("Check HostIp parsing");
- String hostIpStr = resource.getUriHostIp(sampleUri);
- Assert.assertEquals("Expected host IP " + sampleUri.getHost() + " and actual host IP " + hostIpStr + " differ.", sampleUri.getHost(), hostIpStr);
-
- s_logger.info("Check option parsing");
- String expected = "user=administrator,password=1pass@word1,foo=bar,";
- String actualOpts = resource.parseCifsMountOptions(sampleUri);
- Assert.assertEquals("Options should be " + expected + " and not " + actualOpts, expected, actualOpts);
-
- // attempt a configured mount
- final Map<String, Object> params = PropertiesUtil.toMap(loadProperties());
- String sampleMount = (String)params.get("testCifsMount");
- if (!sampleMount.isEmpty()) {
- s_logger.info("functional test, mount " + sampleMount);
- URI realMntUri = new URI(sampleMount);
- String mntSubDir = resource.mountUri(realMntUri);
- s_logger.info("functional test, umount " + mntSubDir);
- resource.umount(resource.getMountingRoot() + mntSubDir, realMntUri);
- } else {
- s_logger.info("no entry for testCifsMount in " + "./conf/agent.properties - skip functional test");
- }
- }
+ @PrepareForTest(NfsSecondaryStorageResource.class)
+ public void testSwiftWriteMetadataFile() throws Exception {
+ String expected = "uniquename=test\nfilename=testfile\nsize=100\nvirtualsize=1000";
- public static Properties loadProperties() throws ConfigurationException {
- Properties properties = new Properties();
- final File file = PropertiesUtil.findConfigFile("agent.properties");
- if (file == null) {
- throw new ConfigurationException("Unable to find agent.properties.");
- }
- s_logger.info("agent.properties found at " + file.getAbsolutePath());
- try(FileInputStream fs = new FileInputStream(file);) {
- properties.load(fs);
- } catch (final FileNotFoundException ex) {
- throw new CloudRuntimeException("Cannot find the file: " + file.getAbsolutePath(), ex);
- } catch (final IOException ex) {
- throw new CloudRuntimeException("IOException in reading " + file.getAbsolutePath(), ex);
- }
- return properties;
+ StringWriter stringWriter = new StringWriter();
+ BufferedWriter bufferWriter = new BufferedWriter(stringWriter);
+ PowerMockito.whenNew(BufferedWriter.class).withArguments(any(FileWriter.class)).thenReturn(bufferWriter);
+
+ resource.swiftWriteMetadataFile("testfile", "test", "testfile", 100, 1000);
+
+ Assert.assertEquals(expected, stringWriter.toString());
}
+ @Test
+ public void testCleanupStagingNfs() throws Exception{
+
+ NfsSecondaryStorageResource spyResource = spy(resource);
+ RuntimeException exception = new RuntimeException();
+ doThrow(exception).when(spyResource).execute(any(DeleteCommand.class));
+ TemplateObjectTO mockTemplate = Mockito.mock(TemplateObjectTO.class);
+
+ TestAppender.TestAppenderBuilder appenderBuilder = new TestAppender.TestAppenderBuilder();
+ appenderBuilder.addExpectedPattern(Level.DEBUG, "Failed to clean up staging area:");
+ TestAppender testLogAppender = appenderBuilder.build();
+ Logger testLogger = Logger.getLogger(NfsSecondaryStorageResource.class);
+ TestAppender.safeAddAppender(testLogger, testLogAppender);
--- End diff --
Minor nit (implement if you like): Lines 74-78 could be reduced to the following:
```
testAppender = new TestAppender.TestAppenderBuilder()
.addExpectedPattern(Level.DEBUG, "Failed to clean up staging area:")
.build();
TestAppender.safeAddAppender(Logger.getLogger(NfsSecondaryStorageResource.class), testAppender);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed closed the pull request at:
https://github.com/apache/cloudstack/pull/1331
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-219166142
@DaanHoogland sure ... doesn't hurt
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-210018309
@syed, I totally understand that you do not have much experience with Java development and that you were following the practices already put in place. The problem is that we have so many cases of what not to do. I find it very good that you are willing to learn and help us improve the ACS code base.
So let’s go to the work.
Sure I changed the variable “logger” to protected, I needed to access it on a test case, so I changed it to an access delimiter that I could use; you should always try to use the most restrictive one, but at the end in Java those access delimiters can always be bypassed using Reflections. The protected delimiter states that children’s of that class and classes at the same package will have access to that variable/method.
I also removed the “final” because I needed to change that variable to the test with a mocked one. The final delimiter would not allow that; therefore, I had to remove it. That is why I would normally avoid the use of final everywhere, it can complicate things. I would only use final where I really want to be strict and not allow a variable change on the fly.
I removed the static to transform that variable into an “instance” attribute; those objects are spring beans, and they are working as singletons, there will be no more than one instance of them. That is why I see no reason to use that attribute as a class field.
Now, you could go over all of the classes that extend “NfsSecondaryStorageResource”, they would have their own “logger” variable, then you could remove the “s_logger” they are using and use the one inherited. Additionally, you would be using a more common name for that variable that is “logger”.
For your second batch of questions, I prefer to use the most restrictive delimiter access possible. I always start with private and then I go opening them on a need basis. I have no idea why someone would do different, such as using “public static final” when it is not needed. ACS has a lot of code that should not be taken as reference. In doubt, you can always call for help; there are folks here that are great devs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-214518411
@rafaelweingartner yes, as I have said, ``final statics`` are not evaluated for GC. A single case is not a noticeable problem. However, if made all loggers were made instance variables, given the number we have, it would become a problem. You stated that your preference was to make loggers instance variables, and that they being ``final static`` was purely cosmetic. My point is that there are both performance and capability motivations to make them ``final static`` (i.e. the ability to log from static methods and accumulated GC overhead).
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. ``TestAppender`` approach is up to @syed and what he feels most comfortable implementing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-209435788
@syed I have seen the code, and I believe we could work a little bit here. Are you willing to?
For instance, class "NfsSecondaryStorageResource" lines 757-768 could be extracted to a method. Then, we can also write a test case to that method to check if the content that is written as expected. The code at lines 757-768 is the same as the one at lines 952-963.
After you extract that I have some other points for improvements, but let’s walk a step at time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-210066343
@rafaelweingartner all statics should ``final`` unless there is explicit handling for their value changing due to thread safety issues. For loggers, they should always be ``private`` and ``static``. Log4j provides the means to retrieve the same logger across threads and contexts by name. Therefore, if you want 5 classes to share the same logger, retrieve the logger for that class by name in each class to avoid the potential pitfalls of static shadowing (i.e. instead of declaring ``private static final Logger LOGGER = Logger.getLogger(MyClass.class)``, declare it as ``private static final Logger LOGGER = Logger.getLogger(CONSTANT_WITH_THE_NAME_OF_ COMMON_LOGGER)``).
As I mentioned in a previous comment, these type of compiler gymnastics are not necessary to test log output. Log4j provides the means to attach a custom appender to the logger so that you can monitor and test output.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r49888184
--- Diff: services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java ---
@@ -942,6 +931,83 @@ protected Answer copyFromNfsToS3(CopyCommand cmd) {
}
}
+ protected boolean swiftUploadMetadataFile(SwiftTO swift, File srcFile, String containerName) throws IOException {
+
+
+ //create a template.properties for Swift with its correct unique name
--- End diff --
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. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-218591057
@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:
```
INFO [c.c.r.ResourceManagerImpl] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) (logid:2044d020) Trying to add a new host at http://kvm1 in data center 1
INFO [o.a.c.f.j.i.AsyncJobManagerImpl] (AsyncJobMgr-Heartbeat-1:ctx-412302f9) (logid:5b139200) Begin cleanup expired async-jobs
INFO [o.a.c.f.j.i.AsyncJobManagerImpl] (AsyncJobMgr-Heartbeat-1:ctx-412302f9) (logid:5b139200) End cleanup expired async-jobs
INFO [o.a.c.e.o.NetworkOrchestrator] (Network-Scavenger-1:ctx-a52a3147) (logid:86cda124) NetworkGarbageCollector uses '10' seconds for GC interval.
INFO [c.c.h.k.d.LibvirtServerDiscoverer] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) (logid:2044d020) cloudstack agent setup command failed: cloudstack-setup-agent -m 192.168.22.61 -z 1 -p 1 -c 1 -g 57acd541-cd37-34ea-af6a-dc6ecd007325 -a --pubNic=cloudbr0 --prvNic=cloudbr1 --guestNic=cloudbr0 --hypervisor=kvm
WARN [c.c.r.ResourceManagerImpl] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) (logid:2044d020) Unable to find the server resources at http://kvm1
INFO [c.c.u.e.CSExceptionErrorCode] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) (logid:2044d020) Could not find exception: com.cloud.exception.DiscoveryException in error code list for exceptions
WARN [o.a.c.a.c.a.h.AddHostCmd] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) (logid:2044d020) Exception:
com.cloud.exception.DiscoveryException: Unable to add the host
at com.cloud.resource.ResourceManagerImpl.discoverHostsFull(ResourceManagerImpl.java:802)
at com.cloud.resource.ResourceManagerImpl.discoverHosts(ResourceManagerImpl.java:597)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:317)
at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:183)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:150)
at org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:91)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:204)
at com.sun.proxy.$Proxy159.discoverHosts(Unknown Source)
at org.apache.cloudstack.api.command.admin.host.AddHostCmd.execute(AddHostCmd.java:142)
at com.cloud.api.ApiDispatcher.dispatch(ApiDispatcher.java:150)
at com.cloud.api.ApiServer.queueCommand(ApiServer.java:698)
at com.cloud.api.ApiServer.handleRequest(ApiServer.java:529)
at com.cloud.api.ApiServer.handle(ApiServer.java:434)
at org.apache.http.protocol.HttpService.doService(HttpService.java:423)
at org.apache.http.protocol.HttpService.handleRequest(HttpService.java:341)
at com.cloud.api.ApiServer$WorkerTask.runInContext(ApiServer.java:1236)
at org.apache.cloudstack.managed.context.ManagedContextRunnable$1.run(ManagedContextRunnable.java:49)
at org.apache.cloudstack.managed.context.impl.DefaultManagedContext$1.call(DefaultManagedContext.java:56)
at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.callWithContext(DefaultManagedContext.java:103)
at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.runWithContext(DefaultManagedContext.java:53)
at org.apache.cloudstack.managed.context.ManagedContextRunnable.run(ManagedContextRunnable.java:46)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)
INFO [c.c.a.ApiServer] (ApiServer-1:ctx-7bf8070e ctx-cdcb3f39) (logid:2044d020) Unable to add the host
```
The fact that both Jenkins and Travis are failing may also be relevant.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-207555946
@syed can you squash the commits please and do a `push -f` to update the PR branch?
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-210081688
@rafaelweingartner The issue is not one of style but performance and availability. The garbage collector skips reachability evaluations of ``final static`` fields where it must perform those for instance variables. This cost adds up when a large number of object is allocated and deallocated. Second, you cannot log from a static method. Our standard in the codebase is to have ``final static`` loggers, and I don't see a compelling reason to step away from it.
In terms of the Mockito approach, I wouldn't have an opinion if the unit test where not requiring us to change the class. The test should always be the servant of the system not the other way around. Personally, I find mocks to be an obfuscation especially when the thing being mocked provides a standard mechanism to observe side-effects.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-219163616
@syed can you close and reopen to see if the error is persistent?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-210086309
I see your point in regards to the garbage collector; but, that class is a singleton, it is not going to be allocated and deallocated.
About the point about of using the log in static method; well, that class is a singleton and I believe that they should not have static method, we are using a dependency injection framework, we should wire things up using the lifecycle of the framework we choose and not try to work things out with static methods.
Actually I see that the need for unit tests reflects on the code. Code with 50, 100 and even more lines cannot be properly tested. The TDD approach affects a lot on the design and writing of code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-215851189
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-210078579
@jburwell I agree with you that all static can be final. I disagree that loggers should be static, but that is a matter of taste. I understand the cache methods of Log4J, but still I do not like much of using static variables. It feels more natural to use the instance attribute when logging.
I find it much more complications to add an appender and then use it to test if some method has been used or some message logged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-219074192
@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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-209759974
For sure @rafaelweingartner. As I have outlined in my mail to you, I've been basically following the practices that have been put in place by the previous developers. If they are not the most optimal/best ways to do things, I am willing to learn the right way.
I've seen your PR. You've made the logger a `protected` variable instead of a `static final` Can you tell me the difference? Is one preferred over the other? In all the code that I've seen on cloudstack, the `s_logger` is `public static final`. Do you have knowldege of why it was done so?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-209390353
@rafaelweingartner I've written a simple test which will fail if someone throws an exception and it is not caught. I've tried mocking the `s_logger` but for it to work is not easy. I've sent you a mail explaning the situation. In any case, even if someone decides to silence exceptions I don't really care as I am not doing anything with them anyways.
Let me know what you think.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
GitHub user syed reopened a pull request:
https://github.com/apache/cloudstack/pull/1331
Fix Sync of template.properties in Swift
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/syed/cloudstack swift-restart-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1331.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1331
----
commit f3b8c7e0f2720639f1db77b15e0791ec70537a29
Author: Syed <sy...@gmail.com>
Date: 2015-12-14T22:37:28Z
Fix Sync of template.properties in Swift
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-172214758
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-212599647
@rafaelweingartner hopefully, the appender I specified in the [gist](https://gist.github.com/jburwell/98b6d94c57f409b5b778ffee6a8a12b1) should be a good start (if not nearly complete) common appender that can be used across all test cases. Since I don't have a test case to exercise, handing it off to @syed to complete, if he desires, seems like the most pragmatic way to complete it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-175825330
Code looks good, but I have no way of testing this since I don't have Swift...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-207898130
@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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-209431789
@syed sorry the delay, I just read your email asking for some assistance; it is the time zone :(
I have opened a PR to your branch with the test case for the "cleanupStagingNfs" method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-201355723
@kollyma This was happening for swift because the `template.properties` file was not correctly populated. So, when a management restart happens, it syncs data from the secondary storage to see which files are present and which files need to be downloaded and it is at this point that the managment server sets the templates in the DB to `not ready` state. I'm not sure if this is the case with pure NFS but you might want to look at your `template.properties` files on the NFS to make sure they are correct.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r59756105
--- Diff: services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java ---
@@ -88,6 +93,19 @@ public void testMount() throws Exception {
}
}
+ @Test
+ public void testCleanupNfsStaging(){
+ TemplateObjectTO templateMock = Mockito.mock(TemplateObjectTO.class);
+ Exception exception = new Exception();
+ resource.logger = Mockito.mock(Logger.class);
+
+ Mockito.doNothing().when(resource.logger).debug("Failed to clean up staging area:", exception);
+ Mockito.when(resource.execute(any(DeleteCommand.class))).thenThrow(exception);
--- End diff --
Why are you using Mockito to mock the logger? Why not retrieve the logger used by the class and add test logger to collect the log message? You can then ask search the log messages in the appender to see if the log message you expect is present. This avoids the overhead of Mockito, and much more straightforward approach to observing that logging is behaving as expected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-179964783
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by pdube <gi...@git.apache.org>.
Github user pdube commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r50066929
--- Diff: services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java ---
@@ -942,6 +931,83 @@ protected Answer copyFromNfsToS3(CopyCommand cmd) {
}
}
+ protected boolean swiftUploadMetadataFile(SwiftTO swift, File srcFile, String containerName) throws IOException {
+
+
+ //create a template.properties for Swift with its correct unique name
+ File uniqDir = _storage.createUniqDir();
+ String metaFileName = uniqDir.getAbsolutePath() + File.separator + "template.properties";
+ _storage.create(uniqDir.getAbsolutePath(), "template.properties");
+
+ String uniqueName = FilenameUtils.getBaseName(srcFile.getName());
+ File metaFile = new File(metaFileName);
+ FileWriter writer = new FileWriter(metaFile);
+ BufferedWriter bufferWriter = new BufferedWriter(writer);
+ bufferWriter.write("uniquename=" + uniqueName);
+ bufferWriter.write("\n");
+ bufferWriter.write("filename=" + srcFile.getName());
+ bufferWriter.write("\n");
+ bufferWriter.write("size=" + srcFile.length());
+ bufferWriter.write("\n");
+ bufferWriter.write("virtualsize=" + getVirtualSize(srcFile, getTemplateFormat(srcFile.getName())));
+ bufferWriter.close();
+ writer.close();
+
+ SwiftUtil.putObject(swift, metaFile, containerName, _tmpltpp);
+ metaFile.delete();
+ uniqDir.delete();
+
+ return true;
+ }
+
+
+ protected Answer copyFromNfsToSwift(CopyCommand cmd) {
+
+ final DataTO srcData = cmd.getSrcTO();
+ final DataTO destData = cmd.getDestTO();
+
+ DataStoreTO srcDataStore = srcData.getDataStore();
+ NfsTO srcStore = (NfsTO)srcDataStore;
+ DataStoreTO destDataStore = destData.getDataStore();
+ File srcFile = getFile(srcData.getPath(), srcStore.getUrl());
+
+ SwiftTO swift = (SwiftTO)destDataStore;
+
+ try {
+
+ String containerName = SwiftUtil.getContainerName(destData.getObjectType().toString(), destData.getId());
+ String swiftPath = SwiftUtil.putObject(swift, srcFile, containerName, srcFile.getName());
+
+
+ DataTO retObj = null;
+ if (destData.getObjectType() == DataObjectType.TEMPLATE) {
+ swiftUploadMetadataFile(swift, srcFile, containerName);
+ TemplateObjectTO newTemplate = new TemplateObjectTO();
+ newTemplate.setPath(swiftPath);
+ newTemplate.setSize(getVirtualSize(srcFile, getTemplateFormat(srcFile.getName())));
+ newTemplate.setPhysicalSize(srcFile.length());
+ newTemplate.setFormat(getTemplateFormat(srcFile.getName()));
+ retObj = newTemplate;
+ } else if (destData.getObjectType() == DataObjectType.VOLUME) {
+ VolumeObjectTO newVol = new VolumeObjectTO();
+ newVol.setPath(containerName);
+ newVol.setSize(srcFile.length());
--- End diff --
Not 100% related to this PR, but are the sizes on volumes and snapshots correct?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-172240147
@DaanHoogland @syed I've opened a pull request containing some simple javadocs to @syed's swift-restart-fix branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-211455173
Loving the discussion here guys! Based on the comments by @jburwell I found http://stackoverflow.com/questions/1827677/how-to-do-a-junit-assert-on-a-message-in-a-logger @rafaelweingartner I would like your opinion on this.
I've also added a function which basically writes the `template.properties` file out to the disk. Now I was thinking of mocking the `BufferedWriter` in that function and somehow catch the output and assert it with an expected output but the writer is a variable which is local to that function. How do mock that @rafaelweingartner
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-208176655
@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. Would would be the best stragetgy for testing private methods. For me this method should be private and I don't know how the community feels like testing private methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r60742145
--- Diff: services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/TestAppender.java ---
@@ -0,0 +1,173 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied. See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+package org.apache.cloudstack.storage.resource;
+
+import com.google.common.base.Joiner;
+import com.google.common.base.Objects;
+import com.google.common.collect.ImmutableMap;
+import org.apache.log4j.AppenderSkeleton;
+import org.apache.log4j.Level;
+import org.apache.log4j.Logger;
+import org.apache.log4j.spi.LoggingEvent;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.regex.Pattern;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Strings.isNullOrEmpty;
+import static java.lang.String.format;
+import static org.apache.log4j.Level.ALL;
+import static org.apache.log4j.Level.DEBUG;
+import static org.apache.log4j.Level.ERROR;
+import static org.apache.log4j.Level.FATAL;
+import static org.apache.log4j.Level.INFO;
+import static org.apache.log4j.Level.OFF;
+import static org.junit.Assert.fail;
+
+/**
+*
+* Tracks one or more patterns to determine whether or not they have been
+* logged. It uses a streaming approach to determine whether or not a message
+* has a occurred to prevent unnecessary memory consumption. Instances of this
+* of this class are created using the {@link TestAppenderBuilder}.
+*
+* To use this class, register a one or more expected patterns by level as part
+* of the test setup and retain an reference to the appender instance. After the
+* expected logging events have occurred in the test case, call
+* {@link TestAppender#assertMessagesLogged()} which will fail the test if any of the
+* expected patterns were not logged.
+*
+*/
+public final class TestAppender extends AppenderSkeleton {
+ private final static String APPENDER_NAME = "test_appender";
+ private final ImmutableMap<Level, Set<PatternResult>> expectedPatternResults;
+ private TestAppender(final Map<Level, Set<PatternResult>> expectedPatterns) {
+ super();
+ expectedPatternResults = ImmutableMap.copyOf(expectedPatterns);
+ }
+ protected void append(LoggingEvent loggingEvent) {
+ checkArgument(loggingEvent != null, "append requires a non-null loggingEvent");
+ final Level level = loggingEvent.getLevel();
+ checkState(expectedPatternResults.containsKey(level), "level " + level + " not supported by append");
+ for (final PatternResult patternResult : expectedPatternResults.get(level)) {
+ if (patternResult.getPattern().matcher(loggingEvent.getRenderedMessage()).matches()) {
+ patternResult.markFound();
+ }
+ }
+ }
+ public void close() {
+// Do nothing ...
+ }
+ public boolean requiresLayout() {
+ return false;
+ }
+ public void assertMessagesLogged() {
+ final List<String> unloggedPatterns = new ArrayList<>();
+ for (final Map.Entry<Level, Set<PatternResult>> expectedPatternResult : expectedPatternResults.entrySet()) {
+ for (final PatternResult patternResults : expectedPatternResult.getValue()) {
+ if (!patternResults.isFound()) {
+ unloggedPatterns.add(format("%1$s was not logged for level %2$s",
+ patternResults.getPattern().toString(), expectedPatternResult.getKey()));
+ }
+ }
+ }
+ if (!unloggedPatterns.isEmpty()) {
+ fail(Joiner.on(",").join(unloggedPatterns));
+ }
+ }
+ private static final class PatternResult {
+ private final Pattern pattern;
+ private boolean foundFlag = false;
+ private PatternResult(Pattern pattern) {
+ super();
+ this.pattern = pattern;
+ }
+ public Pattern getPattern() {
+ return pattern;
+ }
+ public void markFound() {
+ // This operation is thread-safe because the value will only ever be switched from false to true. Therefore,
+ // multiple threads mutating the value for a pattern will not corrupt the value ...
+ foundFlag = true;
+ }
+ public boolean isFound() {
+ return foundFlag;
+ }
+ @Override
+ public boolean equals(Object thatObject) {
+ if (this == thatObject) {
+ return true;
+ }
+ if (thatObject == null || getClass() != thatObject.getClass()) {
+ return false;
+ }
+ PatternResult thatPatternResult = (PatternResult) thatObject;
+ return foundFlag == thatPatternResult.foundFlag &&
+ Objects.equal(pattern, thatPatternResult.pattern);
+ }
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(pattern, foundFlag);
+ }
+ @Override
+ public String toString() {
+ return format("Pattern Result [ pattern: %1$s, markFound: %2$s ]", pattern.toString(), foundFlag);
+ }
+ }
+
+ public static final class TestAppenderBuilder {
--- End diff --
Minor Nit: I should have named this class ``Builder`` since it is already namespaced ``TestAppender``. I would recommend that change since ``new TestAppender.Builder()`` is cleaner than ``newTestAppender.TestAppenderBuilder()``. (Cosmetic I know, but it hits my naming OCD)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-214509189
@jburwell, @syed, sorry the long post, I did a research on a few thing 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.
Please do not take me in a bad way; I find discussions like this very healthy for the future of the project. I also know that the class “TestAppender” is there to be reused in some other tests that want to check the use of log; but still, it feels pretty complicated to me. Giving that, do you see why when I created the test as an example for @Syed, I used the Mocking approach? It feels simpler and more natural to write tests using that approach, at least to me.
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” word per se, but rather with the use of the “static” word.
After having said that, perhaps we could benefit from both words? 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.
What do you think about that?
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:
Let’s take the example of “NfsSecondaryStorageResource” class. That class is an intermediate class to singletons (LocalNfsSecondaryStorageResource, MockLocalNfsSecondaryStorageResource, PremiumSecondaryStorageResource and SimulatorSecondaryStorageResource). All of them also have a Logger instance for their respective class. Also, the “NfsSecondaryStorageResource” extends the “ServerResourceBase” that has a Logger instance too. In total, we have 5 Logger instances. One for each class, giving that all of them are static attributes. If we used an approach similar to the one that is used by Spring-*, we would have one “Logger” instance for each singleton, which would represent 4 logger instances.
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
[2] http://www.ibm.com/developerworks/library/j-jtp01274/
[3] https://docs.oracle.com/cd/E26576_01/doc.312/e24936/tuning-apps.htm#GSPTG00161
[4] http://www.coderanch.com
[5] http://www.coderanch.com/t/381848/java/java/Garbage-Collector-Static-Variables
[6] http://stackoverflow.com/questions/453023/are-static-fields-open-for-garbage-collection
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-182023310
@DaanHoogland I'm not sure why the tests are failing. My changes shouldn't have affected that
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-215620806
@rafaelweingartner I apologize -- I did not understand that you were focused on ``final`` not ``final static``. You are correct, ``final`` has nothing to do with garbage collection -- it is about thread safety. There are very few (and rare) circumstances where a ``static`` reference should be mutable because it accessible across thread boundarys. When they need to be mutable, special care must be taken to ensure exclusive access (e.g. ``synchronized`` blocks, ``ReentrantLock``, ``AtomicReference``, etc). Therefore, I always put the two together. In this case, the logger is a constant value and should be declared ``final``. Sacrificing thread-safety for unit test is not an appropriate trade-off in my opinion.
Stepping back a bit, why are we investing so much time and effort to test a ``DEBUG`` log message? ``DEBUG`` level messages should only be for developers during development of the system. Not only is a low value item to test, it makes the test case brittle as there should never be any guarantees about log messages at ```DEBUG`` and ``TRACE`` log levels. In my opinion, @syed should remove this part of the test entirely.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-212529350
@syed, sorry the delay, I haven't had much time.
I noticed that this class is being ignored. The tests are not properly coded (test should not rely on external files). There is a PR (https://github.com/apache/cloudstack/pull/1499) from a colleague that is addressing exactly that.
On the meantime, this is a pretty tricky method to test; but, there is a nice way to do so.
I opened a PR for your branch with an example, so you can see how to address the test of that method you extracted.
Also, about the other test "testCleanupNfsStaging", I suggest you experiment the other approach pointed out by @jburwell, so you can use both and then it would be easier for you to create an opinion. The way I proposed in my PR, was to be used as an illustration for you.
Any other doubts/problems, you can just ping me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-218056377
@syed this is not compiling for me. Can you review?
```
[INFO] ------------------------------------------------------------------------
[INFO] Building Apache CloudStack Plugin - RabbitMQ Event Bus 4.7.2-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[WARNING] *****************************************************************
[WARNING] * Your build is requesting parallel execution, but project *
[WARNING] * contains the following plugin(s) that are not marked as *
[WARNING] * @threadSafe to support parallel building. *
[WARNING] * While this /may/ work fine, please look for plugin updates *
[WARNING] * and/or request plugins be made thread-safe. *
[WARNING] * If reporting an issue, report it against the plugin in *
[WARNING] * question, not against maven-core *
[WARNING] *****************************************************************
[WARNING] The following plugins are not marked @threadSafe in Apache CloudStack Plugin - RabbitMQ Event Bus:
[WARNING] org.apache.maven.plugins:maven-site-plugin:3.1
[WARNING] *****************************************************************
[INFO]
[INFO] --- maven-checkstyle-plugin:2.11:check (cloudstack-checkstyle) @ cloud-mom-rabbitmq ---
[INFO] Starting audit...
Audit done.
[INFO]
[INFO] --- maven-remote-resources-plugin:1.3:process (default) @ cloud-mom-rabbitmq ---
log4j:ERROR Could not parse url [file:/data/git/cs1/cloudstack/services/secondary-storage/server/test/../conf/log4j.xml].
java.io.FileNotFoundException: /data/git/cs1/cloudstack/services/secondary-storage/server/test/../conf/log4j.xml (No such file or directory)
at java.io.FileInputStream.open(Native Method)
at java.io.FileInputStream.<init>(FileInputStream.java:146)
at java.io.FileInputStream.<init>(FileInputStream.java:101)
at sun.net.www.protocol.file.FileURLConnection.connect(FileURLConnection.java:90)
at sun.net.www.protocol.file.FileURLConnection.getInputStream(FileURLConnection.java:188)
at org.apache.log4j.xml.DOMConfigurator$2.parse(DOMConfigurator.java:765)
at org.apache.log4j.xml.DOMConfigurator.doConfigure(DOMConfigurator.java:871)
at org.apache.log4j.xml.DOMConfigurator.doConfigure(DOMConfigurator.java:778)
at org.apache.log4j.helpers.OptionConverter.selectAndConfigure(OptionConverter.java:526)
at org.apache.log4j.LogManager.<clinit>(LogManager.java:127)
at org.apache.log4j.Logger.getLogger(Logger.java:117)
at com.cloud.resource.ServerResourceBase.<clinit>(ServerResourceBase.java:45)
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:195)
at javassist.runtime.Desc.getClassObject(Desc.java:43)
at javassist.runtime.Desc.getClassType(Desc.java:152)
at javassist.runtime.Desc.getType(Desc.java:122)
at javassist.runtime.Desc.getType(Desc.java:78)
at org.apache.cloudstack.storage.resource.NfsSecondaryStorageResourceTest.setUp(NfsSecondaryStorageResourceTest.java:49)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at org.junit.internal.runners.MethodRoadie.runBefores(MethodRoadie.java:132)
at org.junit.internal.runners.MethodRoadie.runBeforesThenTestThenAfters(MethodRoadie.java:95)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.executeTest(PowerMockJUnit44RunnerDelegateImpl.java:294)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTestInSuper(PowerMockJUnit47RunnerDelegateImpl.java:127)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTest(PowerMockJUnit47RunnerDelegateImpl.java:82)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.runBeforesThenTestThenAfters(PowerMockJUnit44RunnerDelegateImpl.java:282)
at org.junit.internal.runners.MethodRoadie.runTest(MethodRoadie.java:86)
at org.junit.internal.runners.MethodRoadie.run(MethodRoadie.java:49)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.invokeTestMethod(PowerMockJUnit44RunnerDelegateImpl.java:207)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.runMethods(PowerMockJUnit44RunnerDelegateImpl.java:146)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$1.run(PowerMockJUnit44RunnerDelegateImpl.java:120)
at org.junit.internal.runners.ClassRoadie.runUnprotected(ClassRoadie.java:33)
at org.junit.internal.runners.ClassRoadie.runProtected(ClassRoadie.java:45)
at org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.run(PowerMockJUnit44RunnerDelegateImpl.java:118)
at org.powermock.modules.junit4.common.internal.impl.JUnit4TestSuiteChunkerImpl.run(JUnit4TestSuiteChunkerImpl.java:104)
at org.powermock.modules.junit4.common.internal.impl.AbstractCommonPowerMockRunner.run(AbstractCommonPowerMockRunner.java:53)
at org.powermock.modules.junit4.PowerMockRunner.run(PowerMockRunner.java:53)
at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:283)
at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:173)
at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:153)
at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:128)
at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:203)
at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:155)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:103)
[INFO]
[INFO] --- maven-resources-plugin:2.5:resources (default-resources) @ cloud-mom-rabbitmq ---
[debug] execute contextualize
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] skip non existing resourceDirectory /data/git/cs1/cloudstack/plugins/event-bus/rabbitmq/resources
[INFO] Copying 3 resources
[INFO]
[INFO] --- maven-compiler-plugin:3.2:compile (default-compile) @ cloud-mom-rabbitmq ---
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.925 sec - in org.apache.cloudstack.storage.resource.NfsSecondaryStorageResourceTest
Running org.apache.cloudstack.storage.resource.LocalNfsSecondaryStorageResourceTest
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.001 sec <<< FAILURE! - in org.apache.cloudstack.storage.resource.LocalNfsSecondaryStorageResourceTest
testExecuteRequest(org.apache.cloudstack.storage.resource.LocalNfsSecondaryStorageResourceTest) Time elapsed: 0.001 sec <<< ERROR!
javax.naming.ConfigurationException: Unable to find agent.properties.
at org.apache.cloudstack.storage.resource.LocalNfsSecondaryStorageResourceTest.loadProperties(LocalNfsSecondaryStorageResourceTest.java:129)
at org.apache.cloudstack.storage.resource.LocalNfsSecondaryStorageResourceTest.setUp(LocalNfsSecondaryStorageResourceTest.java:68)
Results :
Tests in error:
LocalNfsSecondaryStorageResourceTest.setUp:68->loadProperties:129 Configuration
Tests run: 3, Failures: 0, Errors: 1, Skipped: 0
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-219150983
@syed failed again. :(
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r60740846
--- Diff: services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/TestAppender.java ---
@@ -0,0 +1,173 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied. See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+package org.apache.cloudstack.storage.resource;
--- End diff --
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``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-181507090
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-207676683
Hi, would you mind extracting to a method lines 606-612? They are duplicated at line 627.
Then, you could even create test cases for it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-217971133
@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 `final static` being immutable and skipped by GC makes me feel that using an appender makes more sense. Furthermore, the `TestAppender` class provided by @jburwell is generic so that other tests can use it too.
I will sqash the commits as it looking pretty good now unless you guys have any more comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/1331#issuecomment-218154430
@swill the problem is because of `LocalNfsSecondaryStorageResourceTest`. Before my commit, tests were ignored for this module probably because of the above file. Since I added a new file and enabled tests, the file started bothering again. For now I've silenced it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r59762357
--- Diff: services/secondary-storage/server/test/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResourceTest.java ---
@@ -88,6 +93,19 @@ public void testMount() throws Exception {
}
}
+ @Test
+ public void testCleanupNfsStaging(){
+ TemplateObjectTO templateMock = Mockito.mock(TemplateObjectTO.class);
+ Exception exception = new Exception();
+ resource.logger = Mockito.mock(Logger.class);
+
+ Mockito.doNothing().when(resource.logger).debug("Failed to clean up staging area:", exception);
+ Mockito.when(resource.execute(any(DeleteCommand.class))).thenThrow(exception);
--- End diff --
I didn't use that other approach because I think it adds more complications. I find it easier to use the Mockito and then just checking if the method was used. It feels more natural.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift
Posted by syed <gi...@git.apache.org>.
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1331#discussion_r50443906
--- Diff: services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java ---
@@ -942,6 +931,83 @@ protected Answer copyFromNfsToS3(CopyCommand cmd) {
}
}
+ protected boolean swiftUploadMetadataFile(SwiftTO swift, File srcFile, String containerName) throws IOException {
+
+
+ //create a template.properties for Swift with its correct unique name
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---