Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow renting dedicated nodes if they're on standby status #923

Closed
xmonader opened this issue Dec 28, 2023 · 11 comments · Fixed by #933
Closed

allow renting dedicated nodes if they're on standby status #923

xmonader opened this issue Dec 28, 2023 · 11 comments · Fixed by #933
Assignees
Labels
priority_major type_feature New feature or request
Milestone

Comments

@xmonader
Copy link
Contributor

allow renting dedicated nodes if they're on standby status if not done already

the goal for threefold, is to run a farm full of nodes power managed by farmerbot and be in power target off (managed by the farmerbot), and the farmerbot is supposed to bring these nodes online if they got rented (we need to make sure the are immediately usable for the user "before billing them" otherwise the user will be abused

@sameh-farouk
Copy link
Member

sameh-farouk commented Dec 31, 2023

allow renting dedicated nodes if they're on standby status if not done already

ATM contracts (rent/node) cannot be created on nodes that have power state Down

Power state Down: When a node actually is going to sleep because of the changed power target (requested by farmer bot), it will set it's power state to down on chain by calling change_power_state(PowerState(Up/Down)).

Node contract creation:

ensure!(!node_power.is_down(), Error::<T>::NodeNotAvailableToDeploy);

Rent contract creation:

ensure!(!node_power.is_down(), Error::<T>::NodeNotAvailableToDeploy);

we need to make sure the are immediately usable for the user "before billing them"

What is your suggestion here?

@renauter
Copy link
Collaborator

renauter commented Jan 4, 2024

From TFChain perspective and according to @LeeSmet inputs here #873 (comment) about how it was designed, it is all working as expected regarding rent contracts, assuming that this is client responsibility to cancel contract if node is down.

Now the question is do we want to change the design and remove this rule?

@xmonader
Copy link
Contributor Author

xmonader commented Jan 4, 2024

@renauter the goal is to have e.g 100 nodes in my farm available for renting, problem is the will cost too much power if they don't get rented right away. What I'm suggesting is allowing that to increase the ready/available capacity and save the power bill. I believe this should be relaxed (removing the condition) to allow the renting and maintain the power saving.

The farmerbot is responsible start waking it up within 5 minutes (checking interval)

We will also need in the tooling specially the UI some sort of notification of the user if the rented a standby node that didn't wake up (if we don't update the billing)

@LeeSmet
Copy link
Contributor

LeeSmet commented Jan 4, 2024

The original design of this was that only nodes which are online can be rented. The user needs to contact the farmer bot so that the node to be rented can be brought online, after which the corresponding chain calls are done by the user.

the farmerbot is supposed to bring these nodes online if they got rented (we need to make sure the are immediately usable for the user "before billing them" otherwise the user will be abused

This was a major reason to do things like this, since a "standby" node might not actually be standby. The user will have reserved a node, which is then paid for, even though said node might not come online at all. Additionally, even if it comes online, this might take a significant time which needs proper consideration in the UX

@renauter
Copy link
Collaborator

renauter commented Jan 8, 2024

OK
So what we suggest in terms of implementation to fulfill the issue is:

Are we OK with that ?

In addition we need to prevent node from having PowerState set to Down (= "sleeping") if there is some contract on it
#924

@renauter renauter self-assigned this Jan 9, 2024
@renauter renauter added this to the 2.7.0 milestone Jan 9, 2024
@renauter renauter added this to 3.13.x Jan 9, 2024
@renauter renauter moved this to In Progress in 3.13.x Jan 9, 2024
@renauter
Copy link
Collaborator

Update:

The elapsed time for billing a contract is calculated using the last contract lock timestamp as initial time (see here) and this timestamp is only updated thanks to a previous call to bill_contract(). Now, when the node is in standby mode bill_contract() is not called and by consequence the timestamp not updated so we are still billing for standby period (even if delayed).

To fix this situation, an extra contract lock timestamp update was added outside of bill_contract() when node is first detected to be in standby mode. See the following commit aaa7065 and read the related test to better understand the case.

Even if not the most elegant way I don't see better at the moment and any suggestion is welcome.

@renauter renauter moved this from In Progress to Pending review in 3.13.x Jan 11, 2024
@renauter
Copy link
Collaborator

Update:

Strategy was changed because the one above was not satisfying. A loose pallet coupling (see doc here ) was used between pallet-smart-contract and pallet-tfgrid to be able to use pallet-smart-contract logic inside pallet-tfgrid. See the following commit 86bd2b5 for better understanding.

@renauter renauter added the type_feature New feature or request label Jan 18, 2024
@sameh-farouk
Copy link
Member

sameh-farouk commented Jan 21, 2024

These PRs will be merged soon, thanks for @renauter , It is related to the power-saving feature and the farmer bot so better to have a look @xmonader @rawdaGastan and make sure that the new behavior will be in sync ..

To summarize the incoming changes in behavior:

  • The chain will verify if a node has active node/rent contracts before switching the target to down. If there are active contracts, a NodeHasActiveContracts error will be returned. Previously, the target was changed without chain-side verification. This verification will probably taken out from the farmer's bot code and just handle the chain error accordingly.

  • Creation of Rent contracts will be allowed for standby nodes (any client-side restrictions should be adjusted accordingly). However, Node contracts will retain their old behavior and return the same error as before.

  • Billing for rent contracts will be skipped when nodes are in standby mode and will be based on a timestamp that is updated only when the node status changes back to up. If a user cancels the rent contract before the node power status changes back up (for example, if the node doesn’t come up), he won’t be charged anything other than the extrinsic fees.

Some concerns:
This satisfies the requirements mentioned in the issue to protect the user. However, what if a user decides to spam a farmer by renting their nodes and canceling it immediately afterward? Will Farmer Bot detect the contract cancellation and put the nodes on standby again? Is that enough? Should we restrict the cancellation of rent contracts till a specific block count passes or it would complicate things unnecessarily?

Update:
These changes are live now on devnet (runtime 149)

@github-project-automation github-project-automation bot moved this from Pending review to Done in 3.13.x Jan 22, 2024
@renauter renauter moved this from Done to Pending Deployment in 3.13.x Jan 22, 2024
@renauter renauter reopened this Jan 22, 2024
@sameh-farouk sameh-farouk moved this from Pending Deployment to In Verification in 3.13.x Jan 28, 2024
@A-Harby
Copy link

A-Harby commented Feb 18, 2024

Devnet, a4a560c.

I don't think this is solved; I got a standby node with ID 155 on farm 4420.
It has no contracts, is on standby, and is rentable, as you can see below.
image
But I still can't rent it.
image

@A-Harby A-Harby moved this from In Verification to Accepted in 3.13.x Feb 18, 2024
@AhmedHanafy725
Copy link

@A-Harby you can use the clients to rent this node. For the dashboard, there is an issue to allow that threefoldtech/tfgrid-sdk-ts#2143

@A-Harby
Copy link

A-Harby commented Feb 18, 2024

Verified,
On grid client rent script.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority_major type_feature New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants