Use latest endpoint for /system_info #1421
Conversation
simao
force-pushed
the
use-system-info
branch
from
69b2321 to
735e6fb
Compare
|
I like the idea and requested a similar change a long time ago. However, this is going to break my deployment. I think this commit message should probably include what changes will be needed to the device-gateway and maybe some context about the bug you've referenced. |
|
rvi core is deprecated and not in use any more, so going forward all references to it should be removed. device-registry handles this api now, and the last versions of core forward the data to device-registry. Device gw should proxy The bugs I referenced report that |
| @@ -186,7 +186,7 @@ data::InstallationResult SotaUptaneClient::PackageInstallSetResult(const Uptane: | |||
| void SotaUptaneClient::reportHwInfo() { | |||
| Json::Value hw_info = Utils::getHardwareInfo(); | |||
| if (!hw_info.empty()) { | |||
| http->put(config.tls.server + "/core/system_info", hw_info); | |||
| http->put(config.tls.server + "/system_info", hw_info); | |||
There was a problem hiding this comment.
@patrickvacek Perhaps it makes sense to introduce another configuration variable to set a device registry URL explicitly as in case of Director, ImageRepo and Treehub, config.tls.server is kind of opaque/vague - not clear to which of the backend services it points to.
There was a problem hiding this comment.
config.tls.server is the URL for the device gateway. We can rename it, but then we have to maintain backwards compatibility with the old name, which sounds tedious.
There was a problem hiding this comment.
I am not sure if backwards compatibility is required for real customers as this variable is basically automatically defined by aktualiz.
Also, there is inconsistency in the way how URLs are defined in the config, there are service specific URLs for all backend services except DeviceRegistry.
There was a problem hiding this comment.
I am not sure if backwards compatibility is required for real customers as this variable is basically automatically defined by aktualiz.
We can't really guarantee that. If someone has overwritten tls.server in their config and then we change the name of the config variable in an update, their device will break and require manual intervention.
there is inconsistency in the way how URLs are defined in the config, there are service specific URLs for all backend services except DeviceRegistry.
That's more or less abstracted from us. It depends on how the endpoints are defined in the device gateway, since that's the only service (other than treehub) that we actually talk to directly. We can't really do anything about that unless there is a specific path only used for forwarding to device registry, which I'm not sure is true. @simao or another backend dev may know better.
There was a problem hiding this comment.
If someone has overwritten tls.server in their config and then we change the name of the config variable in an update, their device will break and require manual intervention.
I understand it and that's what I thought initially too, but on second thought I don't see an use case of tls.server server custom overriding by a customer that will work on practice unless they override it by the same value as it's set by default - DeviceGateway URL. Anyway, this is really not important at the moment.
@doanac Does this feature work in your deployment currently? It's been broken in ours for a while now. I'm not sure there's a good way to remain backwards-compatible on the aktualizr side; I don't want to have to have config variables for every last endpoint, and that might not even solve the problem. If you need to support the old version, the right solution is probably to support both endpoints on the backend. |
rvi core is deprecated and not in use any more, so going forward all references to it should be removed. device-registry handles this api now, and the last versions of core forward the data to device-registry. Device gw should proxy /core/system_info to api/v1/devices/:uuid/system_info, which is handled by device registry. Signed-off-by: Simão Mata <[email protected]>
Signed-off-by: Patrick Vacek <[email protected]>
pattivacek
force-pushed
the
use-system-info
branch
from
735e6fb to
806ca05
Compare
|
@simao I put some of the text you posted in the commit message as requested by Andy. I also pushed a fix for the test that was breaking. While doing so, I noticed that we're sending the list of installed packages to |
@patrickvacek - We created an out-of-tree patch to get it working: In the case of this change, I'll just add another nginx proxy section to handle things. |
|
@mike-sul FYI @doanac I'm interested in seeing this made more consistent going forward as well, and I think this is a step in the right direction, assuming |
Yeah, I see it, will try to figure it out, seems like some kind of race condition. |
|
@patrickvacek yes, /core/installed should be changed but not yet because there is no support for a new endpoint in device-gateway, /system_info is already supported in device gateway. |
Fixes https://saeljira.it.here.com/browse/OTA-3379
Only works in production, not sit, due to https://saeljira.it.here.com/browse/OTA-3936
Will change device-gateway so that legacy devices can still use
/core/system_info