CLOUDSTACK-8677: Call-home functionality for CloudStack by wido · Pull Request #625 · apache/cloudstack

wido · 2015-07-27T12:44:04Z

With this commit the Management Server will be default generate a anonymous Usage
report every 7 (seven) days and submit this information back to the Apache CloudStack project.

These anonymous reports do NOT contain any information about Instance names, subnets, etc. It only
contains numbers about how CloudStack is being used.

This information is vital for the project to gain more insight in how CloudStack is being used.

Users can turn the reporting off by setting usage.report.interval to 0 (zero)

asfbot · 2015-07-27T12:45:48Z

cloudstack-pull-rats #130 SUCCESS
This pull request looks good

asfbot · 2015-07-27T12:58:06Z

cloudstack-pull-requests #828 UNSTABLE
Looks like there's a problem with this pull request

DaanHoogland · 2015-07-27T13:07:14Z

with the new style config (ConfigKey), this isn't needed is it? It should insert the default if it doesn't find the value.

Hmm, I don't know. I wrote this code in 2014 and was that in there by now? I'm just doing this because I assumed it was still required.

Maybe @bhaisaab knows?

@wido
@DaanHoogland is correct. DB query to insert new config items is not required now.

But only if you use new style ConfigKey!

DaanHoogland · 2015-07-27T13:08:09Z

any way to verify this?

sedukull · 2015-07-27T13:22:45Z

Is this resource as well closed? Do we want to close resources in exception path as well.

asfbot · 2015-07-27T13:41:46Z

cloudstack-pull-rats #131 SUCCESS
This pull request looks good

asfbot · 2015-07-27T13:58:31Z

cloudstack-pull-requests #829 UNSTABLE
Looks like there's a problem with this pull request

asfbot · 2015-07-27T14:04:19Z

cloudstack-pull-analysis #63 UNSTABLE
Looks like there's a problem with this pull request

asfbot · 2015-07-27T15:10:02Z

cloudstack-pull-analysis #64 UNSTABLE
Looks like there's a problem with this pull request

sedukull · 2015-07-28T01:00:49Z

It seems we are using a singleton pattern here, but it seems not thread safe?

Can you explain that a bit more? I think I'm missing some knowledge here

@wido look at my comment at line 123. you are writing a field that can be written from several locations at once. It needs guarding against programmers not communicating while coding.

It seems the purpose is to make only one instance of usagereporter available per process and so we added getInstance() method, where user calls the getInstance() to get the object reference, in that scenario,

Make the constructor private. create the object only when s_instance is null. Otherwise, s_instance will be new reference with each new object creation.

It seems we have two overloaded variants of getInstance, not sure which one is called and when to use?

Then, if getInstance is called to get an instance of UsageReporter and is static, do we want to use lock getinstance code to make it critical section, synchronize and avoid race conditions, if multiple threads are calling getInstance at the same time, its like two threads using getInstance at the sametime first time to create an object.

DaanHoogland · 2015-07-28T08:38:47Z

@wido you are introducing two findbugs findings, can you look at those?

wido · 2015-07-28T08:41:26Z

@DaanHoogland I will. Although I don't understand why these are happening

DaanHoogland · 2015-07-28T08:48:39Z

A public (instance) constructor writes to a static variable. I think you can make the constructor private and assign the static from getInstance(). First check for null and then assign if needed or return the existing object.

public static UsageReporter getInstance(Map<String, String> configs) {
if (s_instance == null) {
s_instance = new UsageReporter(); //make sure it is private so it can only be called from here
s_instance.init(configs);
}
return s_instance;
}

asfbot · 2015-07-28T12:37:38Z

cloudstack-pull-rats #135 SUCCESS
This pull request looks good

asfbot · 2015-07-28T12:50:44Z

cloudstack-pull-requests #833 UNSTABLE
Looks like there's a problem with this pull request

asfbot · 2015-07-28T13:37:53Z

cloudstack-pull-analysis #68 UNSTABLE
Looks like there's a problem with this pull request

DaanHoogland · 2015-07-28T13:53:37Z

so findbugs doesn't like my solution either and it is right ;)
new_instance = new UsageReporter();
new_instance.init(configs);
s_instance = new_instance;
would be better. maybe even in a synchronised block to prevent to synchronous call to getInstance resulting in two instances of a singleton present.

Please add synchronized block or make that method synchronized for safety. That should address the warning reported by findbug.

asfbot · 2015-07-29T15:25:25Z

cloudstack-pull-rats #150 SUCCESS
This pull request looks good

asfbot · 2015-07-29T15:39:45Z

cloudstack-pull-requests #848 UNSTABLE
Looks like there's a problem with this pull request

asfbot · 2015-07-29T16:37:43Z

cloudstack-pull-analysis #83 UNSTABLE
Looks like there's a problem with this pull request

wido · 2015-07-31T12:21:58Z

I still don't know why travis fails. It fails on a storage part, but this commit doesn't touch anything from the storage. So I can't see why.

asfbot · 2015-07-31T12:23:44Z

cloudstack-pull-rats #173 SUCCESS
This pull request looks good

DaanHoogland · 2015-07-31T12:37:51Z

@wido not sure if these are related but please have a look at https://builds.apache.org/job/cloudstack-pull-analysis/83/org.apache.cloudstack$cloud-core/testReport/
it contains some unit test failure reports. I don't think you touched this directly either.

wido · 2015-07-31T13:00:37Z

@DaanHoogland I see, but I'm not even getting near that code. So it's unclear to me why this PR seems unstable.

DaanHoogland · 2015-07-31T13:02:27Z

let's find out. it might reveal some totally unrelated problem due to .... I never saw the serialization tests before AFAIR but will look at them now ;)

DaanHoogland · 2015-07-31T13:17:32Z

cs.gson.version is defined a second time here.

DaanHoogland · 2015-07-31T13:28:12Z

I confirmed the problem in the failing test is in the newer gson version. Hope this works with the older gson. testing ... 1-2

to bad, it doesn't

asfbot · 2015-07-31T13:35:23Z

cloudstack-pull-analysis #106 UNSTABLE
Looks like there's a problem with this pull request

wido · 2015-07-31T13:42:47Z

@DaanHoogland I need 2.3.1 for this patch to work. It builds with 2.3.1, but not with 1.7.2

asfbot · 2015-07-31T13:47:20Z

cloudstack-pull-rats #174 SUCCESS
This pull request looks good

asfbot · 2015-07-31T14:08:37Z

cloudstack-pull-requests #872 UNSTABLE
Looks like there's a problem with this pull request

asfbot · 2015-07-31T15:14:44Z

cloudstack-pull-analysis #107 UNSTABLE
Looks like there's a problem with this pull request

wido · 2015-08-03T19:20:11Z

I just don't know why the tests keep failing on this one. They seem to fail on code not touched by this PR at all. Really want to get this in to 4.6.

DaanHoogland · 2015-08-03T19:45:25Z

@wido I am pretty sure acs code has a problem with the new gson version (see https://builds.apache.org/job/cloudstack-pull-requests/872/org.apache.cloudstack$cloud-core/testReport/)

wido · 2015-08-05T09:19:20Z

@DaanHoogland You are right. There has been a change between GSON 1.7 and 2.0 which causes a failure of the tests.

Looking into this, but my Java Reflecting knowledge is low and that makes it hard to figure this out.

asfbot · 2015-08-05T11:32:20Z

cloudstack-pull-rats #211 SUCCESS
This pull request looks good

asfbot · 2015-08-05T11:44:23Z

cloudstack-pull-requests #908 UNSTABLE
Looks like there's a problem with this pull request

asfbot · 2015-08-05T12:57:38Z

cloudstack-pull-analysis #144 UNSTABLE
Looks like there's a problem with this pull request

wido · 2015-08-14T08:55:31Z

I have create a issue regarding the new Gson version: https://issues.apache.org/jira/browse/CLOUDSTACK-8708

sebgoa · 2015-08-18T15:45:15Z

This will be turned ON by default ?
I think you should bring this up on the list, at least to give a heads up and an official public record.

wido · 2015-08-19T07:20:43Z

@Runseb Yes, indeed. But the PR is not ready to merge yet, it's blocked by a GSON issue which I don't know how to resolve.

This is what @DaanHoogland found out:

"the gson issue is with primitive types. support was explicitely dropped and these are used in the tests. We should make sure they are not used in the real code and adjust the test. (ref PR625 and CLOUDSTACK-8708)"

But yes, this should go on the ML and very, very clearly in the Release Notes.

yadvr · 2015-08-26T09:38:30Z

@wido can you implement this as a plugin instead of core feature? Also do we want cloudstack to report statistics on each run; I suggest to have it as a plugin with a small db entry (global setting or a separate db) to track the last time+version we sent the payload/report and we can report that on each upgrade and perform checks every time cloudstack runs. This also needs to have a global lock in case of multiple mgmt servers.

wido · 2015-08-27T10:04:38Z

@bhaisaab Isn't it already a plug-in?

Don't know if the PR is the right way to discuss it, but I also want to know how many Instances and cloud is running. So not only on upgrades. So it can send diagnostics every 7 days (default) and that way we can see how clouds grow and such.

We know the install data and after that we see the growth.

With this commit the Management Server will be default generate a anonymous Usage report every 7 (seven) days and submit this information back to the Apache CloudStack project. These anonymous reports do NOT contain any information about Instance names, subnets, etc. It only contains numbers about how CloudStack is being used. This information is vital for the project to gain more insight in how CloudStack is being used. Users can turn the reporting off by setting usage.report.interval to 0 (zero) More information: http://cloudstack.apache.org/call-home.html

rodrigo93 · 2016-02-16T16:50:02Z

+
+public class AtomicGsonAdapter extends TypeAdapter<AtomicLongMap> {
+
+    public AtomicLongMap<Object> read(JsonReader reader) throws IOException {


Hi @wido,
Why don't you turn this method "void", so you could remove the "return null;" line.

wido · 2016-04-21T07:42:22Z

This will probably never merge. Closing for now.

Co-authored-by: Pearl Dsilva <pearl.dsilva@shapeblue.com> Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

DaanHoogland reviewed Jul 27, 2015
View reviewed changes

sedukull reviewed Jul 27, 2015
View reviewed changes

sedukull reviewed Jul 28, 2015
View reviewed changes

DaanHoogland reviewed Jul 28, 2015
View reviewed changes

DaanHoogland reviewed Jul 31, 2015
View reviewed changes

Comment thread pom.xml Outdated

Copy link
Copy Markdown

Contributor

DaanHoogland Jul 31, 2015

Choose a reason for hiding this comment

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

cs.gson.version is defined a second time here.

wido force-pushed the CLOUDSTACK-8677 branch 3 times, most recently from c2eb3ec to 2a9117d Compare October 14, 2015 15:42

wido force-pushed the CLOUDSTACK-8677 branch from 2a9117d to b0eec2a Compare October 15, 2015 15:20

wido mentioned this pull request Oct 20, 2015

[wip] CLOUDSTACK-8708: gson update and ArrayTypeAdaptor rewrite #932

Closed

rodrigo93 reviewed Feb 16, 2016
View reviewed changes

wido closed this Apr 21, 2016

yadvr pushed a commit that referenced this pull request Jan 20, 2021

storage: Allow migration of detached volumes (#625)

8b85d0f

Co-authored-by: Pearl Dsilva <pearl.dsilva@shapeblue.com> Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>


		public class AtomicGsonAdapter extends TypeAdapter<AtomicLongMap> {

		public AtomicLongMap<Object> read(JsonReader reader) throws IOException {

Conversation

wido commented Jul 27, 2015

Uh oh!

asfbot commented Jul 27, 2015

Uh oh!

asfbot commented Jul 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jul 27, 2015

Uh oh!

asfbot commented Jul 27, 2015

Uh oh!

asfbot commented Jul 27, 2015

Uh oh!

asfbot commented Jul 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 28, 2015

Uh oh!

wido commented Jul 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jul 28, 2015

Uh oh!

asfbot commented Jul 28, 2015

Uh oh!

asfbot commented Jul 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jul 29, 2015

Uh oh!

asfbot commented Jul 29, 2015

Uh oh!

asfbot commented Jul 29, 2015

Uh oh!

wido commented Jul 31, 2015

Uh oh!

asfbot commented Jul 31, 2015

Uh oh!

DaanHoogland commented Jul 31, 2015

Uh oh!

wido commented Jul 31, 2015

Uh oh!

DaanHoogland commented Jul 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 31, 2015

Uh oh!

asfbot commented Jul 31, 2015

Uh oh!

wido commented Jul 31, 2015

Uh oh!

asfbot commented Jul 31, 2015

Uh oh!

asfbot commented Jul 31, 2015

Uh oh!

asfbot commented Jul 31, 2015