search

threefoldtech / tfchain

Threefold Chain.
Apache License 2.0
15 stars 11 forks source link

Support update node location #995

Closed 0oM4R closed 1 month ago

0oM4R commented 3 months ago

We need to support updating node locations. changes on MaxMind Database lead to inconstant location data, https://github.com/threefoldtech/tfgrid-sdk-ts/issues/3021#issuecomment-2282836629

https://github.com/threefoldtech/zos/issues/2401

sameh-farouk commented 2 months ago

It's already supported on the TFChain side. A node can update its location by calling the update_node.

Omarabdul3ziz commented 2 months ago

using update_node requires passing the full node object which can't be reconstructed each time we update the location. i tried to send the node object with the new location struct and the basic fields like nodeid/twinid/farmid but I got errors of missing fields like capacity image

i see we handle the uptime report with a separate endpoint https://github.com/threefoldtech/tfchain/blob/1c787e1918a2d75e0e0c3db74d82f661c63f06e9/clients/tfchain-client-go/node.go#L595

could we have a separate method that do the same for the location update?

sameh-farouk commented 2 months ago

using update_node requires passing the full node object which can't be reconstructed each time we update the location. i tried to send the node object with the new location struct and the basic fields like nodeid/twinid/farmid but I got errors of missing fields like capacity

I can't understand why the node object can't be reconstructed each time we update the location. I'm not familiar with ZOS implementation, but the required data is local to the node and should be cheap and straightforward to retrieve. I also assume you are comparing the node location to what is stored on the chain, so you are already getting the full node info from the chain. You could then adjust and update it, and send it back.

It's already used when the node boots up; it compares the real node info with the on-chain info. If they don't match, it calls update_node with the new info.

for example, see this PR for a hint about how you can construct a node object https://github.com/threefoldtech/zos/commit/c3e183cf13de49136b4fa13d8e8c8ee52b8008a3

Also this: https://github.com/threefoldtech/zos/blob/a19e11ae0ed76e3faabed863b686bf348ef14b4b/pkg/registrar/register.go#L168-L215

could we have a separate method that do the same for the location update?

It is not optimal to have separate calls for every node property. If there are multiple inconsistencies, you would make multiple transactions to update the node object instead of one call.

Since the requested functionality is already available and used by Zos for the same purpose, introducing an alternative way is not reasonable.

Omarabdul3ziz commented 1 month ago

my main concern was calculating the used resources while it maybe during the update get updated by another event deploying a workload but double checking we don't actually send the used resources while updating.

my suggestion was to tweak the update_node method or add a new one to not require all the fields of the node but only update the fields that has value changed on the node object.

a workaround instead of restructuring the node object locally, we could call the chain to get the node, compare with the new location if changed, overwrite with the new location and send back to the chain with the current update_node method.

this will work fine for my case, so no work needed from the chain side.