search

SiaFoundation / renterd

A renter for Sia
https://sia.tech/software/renterd
MIT License
68 stars 20 forks source link

Move `autopilots` into `autopilot_state` #1657

Open peterjan opened 1 week ago

peterjan commented 1 week ago

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 commented 6 days ago

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.

peterjan commented 5 days ago

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.
alexfreska commented 1 day ago

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.

@ChrisSchinnerl yes I can do that, although I think everything in the allowance-specific UI PR should be relevant, I'll do another check as it also it looks this PR has had a few more tweaks that I will need to sync up with.

Regarding waiting on the UI change, the workflow is quite awkward from my end as I have to create a temporary branch on the cluster repo that installs this renterd branch, and then temporarily install that into the internal/cluster e2e testing package in the web repo, update all tests to pass, and then to get CI to pass by either committing and reverting those afterwards or ignoring the CI failures because they run against dev. Could we merge this into dev and update the cluster repo? That way I can simply update internal/cluster in web and get all my tests to pass against these change, then I can release a new version of web/renterd. This workflow would just assumes the dev branch is not necessarily fully "stable" between daemon and UI for every commit, but that would probably be reasonable during these rare sweeping refactors, and is also part of the difference/expectation between dev vs master. Let me know if merging sounds ok or if I should go with the aforementioned workflow, or if there is another way to approach testing across this dependency.

ChrisSchinnerl commented 1 day ago

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.

@ChrisSchinnerl yes I can do that, although I think everything in the allowance-specific UI PR should be relevant, I'll do another check as it also it looks this PR has had a few more tweaks that I will need to sync up with.

Regarding waiting on the UI change, the workflow is quite awkward from my end as I have to create a temporary branch on the cluster repo that installs this renterd branch, and then temporarily install that into the internal/cluster e2e testing package in the web repo, update all tests to pass, and then to get CI to pass by either committing and reverting those afterwards or ignoring the CI failures because they run against dev. Could we merge this into dev and update the cluster repo? That way I can simply update internal/cluster in web and get all my tests to pass against these change, then I can release a new version of web/renterd. This workflow would just assumes the dev branch is not necessarily fully "stable" between daemon and UI for every commit, but that would probably be reasonable during these rare sweeping refactors, and is also part of the difference/expectation between dev vs master. Let me know if merging sounds ok or if I should go with the aforementioned workflow, or if there is another way to approach testing across this dependency.

@alexfreska we can do that while we still have dev but the plan is to get rid of it and use short-lived feature branches moving forward to avoid diverging that much from master and get into the habit of releasing more often. So longterm we need to figure out a way to more easily test UI PRs against specific renterd/hostd/etc. PRs.

alexfreska commented 1 day ago

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.

@ChrisSchinnerl yes I can do that, although I think everything in the allowance-specific UI PR should be relevant, I'll do another check as it also it looks this PR has had a few more tweaks that I will need to sync up with. Regarding waiting on the UI change, the workflow is quite awkward from my end as I have to create a temporary branch on the cluster repo that installs this renterd branch, and then temporarily install that into the internal/cluster e2e testing package in the web repo, update all tests to pass, and then to get CI to pass by either committing and reverting those afterwards or ignoring the CI failures because they run against dev. Could we merge this into dev and update the cluster repo? That way I can simply update internal/cluster in web and get all my tests to pass against these change, then I can release a new version of web/renterd. This workflow would just assumes the dev branch is not necessarily fully "stable" between daemon and UI for every commit, but that would probably be reasonable during these rare sweeping refactors, and is also part of the difference/expectation between dev vs master. Let me know if merging sounds ok or if I should go with the aforementioned workflow, or if there is another way to approach testing across this dependency.

@alexfreska we can do that while we still have dev but the plan is to get rid of it and use short-lived feature branches moving forward to avoid diverging that much from master and get into the habit of releasing more often. So longterm we need to figure out a way to more easily test UI PRs against specific renterd/hostd/etc. PRs.

@ChrisSchinnerl ah good to know re master branch. Yeah, then it sounds like we will need to think up an adjusted e2e testing workflow.

peterjan commented 9 hours ago

If we get rid of dev we definitely need feature branches and a way to easily test them in e2e test that include the UI. I personally like having dev if it means we can break it from time to time for refactors that touch upon many parts and are just annoying, like deduplicating autopilots or getting rid of contract sets. I think the key is short-lived, if those end up being actually short lived it'll work just fine but if they drag on it'll cause more merge conflicts than having a dev branch I think. Are we good to merge this then as long as we have dev. We could also have a feature branch for removing contract sets and make this a part of it.

ChrisSchinnerl commented 8 hours ago

@peterjan I don't think we need a new feature branch. We can rebase the contract set PR onto this one. Then merge it into this PR and finally merge them together.