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

Move autopilots into autopilot_state #1657

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from
Open

Move autopilots into autopilot_state #1657

wants to merge 25 commits into from

Conversation

peterjan
Copy link
Member

@peterjan peterjan commented Nov 6, 2024

This migrates the autopilots to a single autopilot_state table, effectively removing the concept of multiple autopilots. I think autopilot state can be defined as config + the current period, I opted to keep the AutopilotConfig which can be updated in the bus. The current_period can be updated separately, which solves a race in the autopilot but that won't be an issue anyway since we're merging autopilot and bus. Tested on SQLite, not (yet) on MySQL, want to get a round of reviews in first.

@peterjan peterjan self-assigned this Nov 6, 2024
@peterjan peterjan marked this pull request as ready for review November 7, 2024 08:32
api/host.go Outdated Show resolved Hide resolved
bus/bus.go Outdated Show resolved Hide resolved
internal/test/e2e/cluster_test.go Outdated Show resolved Hide resolved
stores/sql/main.go Outdated Show resolved Hide resolved
autopilot/autopilot.go Outdated Show resolved Hide resolved
stores/autopilot.go Outdated Show resolved Hide resolved
bus/routes.go Outdated Show resolved Hide resolved
RenewWindow: 144 * 7 * 2,
Download: 1e12, // 1 TB
Upload: 1e12, // 1 TB
Storage: 4e12, // 4 TB
Copy link
Member

@n8maninger n8maninger Nov 8, 2024

Choose a reason for hiding this comment

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

Similar conversation: since we now have initial contract funding instead of a global allowance and refreshing a contract in v2 will be less painful, are these even necessary? Attempting to calculate this ahead of time adds unnecessary setup complexity and overhead. It may end up in more contracts overall, but storage utilization could be more efficient if the scoring didn't need to check for storage space available based on expected storage * redundancy / target contracts.

If a host runs out of storage space, but is otherwise in working condition, form a contract with a new host instead of churning it out of the set.

Copy link
Member

Choose a reason for hiding this comment

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

We could get rid of the remaining storage score and simplify the collateral score and instead just check whether a host is already full / out of collateral before forming a contract without giving them a score.

@peterjan
Copy link
Member Author

peterjan commented Nov 12, 2024

Thanks @n8maninger for the review. I combined your suggestion into a separate issue and plan to F/U with that immediately after this one. Getting rid of the current period never crossed my mind, but since we got rid of allowance that's now a very logical next step indeed. I won't resolve the open comments to allow further discussion.

Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

This looks pretty good overall. Just need to make sure

  1. migrations were manually tested
  2. we make sure the changes here are consistent with refactor(renterd): remove contract sets web#810 and that we somewhat merge these together

autopilot/autopilot.go Show resolved Hide resolved
bus/client/autopilot.go Outdated Show resolved Hide resolved
@peterjan
Copy link
Member Author

This looks pretty good overall. Just need to make sure

  1. migrations were manually tested
    DONE
  2. we make sure the changes here are consistent with refactor(renterd): remove contract sets web#810 and that we somewhat merge these together
    Either we have to extend this PR with every F/U PR to dedupe autopilots and contract sets, that's going to be roughly 4k loc change though. Another option is to split the UI PR.

Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

Lgtm. @alexfreska any chance we can break out the changes of your big UI PR relevant to this PR and then tag a version? That way we could update the UI in this PR to make sure we don't break the UI when merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants