andrijapanicsb commented on PR #13985:
URL: https://github.com/apache/cloudstack/pull/13985#issuecomment-5531623556
Thanks @wido, I think this is going in a much better direction.
(btw, 53 lines only, I really gave my best)
I like the change to `telemetry.*`, having one fixed reporting endpoint,
moving the installation ID logic into a separate utility, and especially the
tests around the actual JSON payload. This makes it much easier to understand
exactly what is being sent and also makes future changes to the format more
obvious.
I don't want to repeat my previous very long review here :) but I also don't
want it to look like the remaining points are no longer important. Since this
is still WIP, I think there are a few important things we should keep in mind
before this gets anywhere close to merge-ready:
1. **Installation ID**
I am OK with keeping the current derived ID approach for now, but I don't
think we should say that it is guaranteed to be unique for every installation.
Two installations could in theory have the same initial version and timestamp,
and cloning a CloudStack DB would also produce the same ID.
I think the important thing here is to clearly define what we consider an
"installation" and what we expect to happen with cloned/restored/reinstalled
databases.
2. **Making sure reports are genuine**
The collector still has no way to know if a report really came from
CloudStack. Anybody can read the source code, generate a random ID and send
valid-looking JSON directly to the endpoint.
Apart from possible DoS/storage abuse, this also means somebody could
pollute the statistics with fake installations. If we later use these
statistics to make decisions about features, hypervisors, storage support,
deprecations, etc., we need to have reasonable confidence that the data is real.
I think this is one of the main architectural points still to solve.
3. **Actual report schema validation**
The new client-side payload tests are very useful.
However, as far as I can see, the collector itself still validates only
that the request is valid/sane JSON: depth, number/length of keys, value types,
etc. It does not actually validate that the JSON matches the expected
CloudStack telemetry structure.
For example, an unrelated JSON object with valid types would still pass
the current validation.
I think we should have an explicit schema version in the report and
validate the received payload against the expected schema.
4. **Current vs historical VM statistics**
One thing I think is especially important here is `vm_instance`.
CloudStack normally keeps VM rows in the database after the VM has been
removed, so simply reading all `VMInstanceVO` objects can give us statistics
about VMs which existed during the lifetime of the cloud, rather than the
current size/state of the cloud.
Both numbers can actually be useful, but they mean very different things.
I think we should explicitly separate current/non-removed VMs from
historical/lifetime numbers.
5. **Privacy on the receiver side**
I don't have a problem with the actual report payload from a privacy
point of view. The information being sent is aggregated and does not directly
identify the operator.
My remaining concern is the HTTP side. The receiving infrastructure will
see the source IP, and if normal HTTP/proxy access logging is enabled we can
end up with a persistent installation ID associated with the public IP of the
operator.
That could potentially identify the company/provider even though there is
nothing identifying them inside the JSON itself.
So I think we need to define how source IPs, proxy headers, HTTP logs and
retention are handled before the real collector is put into production. This
probably also needs to be checked with ASF privacy/infra rather than only
decided inside the CloudStack PMC.
There are still some other points from the previous review: only one MS
should probably send the report in an HA setup, doing the aggregation in SQL
instead of loading all objects into Java, active/stale installation tracking,
actual volume statistics vs configured disk offerings, collector storage, and
making the opt-in/report preview visible to operators.
I don't think all of those need to be solved in the same commit though. They
can be handled step by step while this is still WIP.
Overall, definitely +1 from me on continuing with this and thx again for
such a great initiative!
The latest changes already address some of the previous concerns nicely. I
would mainly make sure we agree on the points above before we consider the
design stable and start moving towards merge-ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
