fix: Remove resource offers in DealNegotiating[0] state on disconnect #475
Conversation
bgins
changed the title
| // TODO Remove before merge | ||
| fmt.Printf("+++ resource offers before removal: %+v\n", resourceOffers) |
There was a problem hiding this comment.
[nit] Reminder to remove once we're ready to merge
pkg/solver/controller.go
Outdated
| if offer.State == 0 { | ||
| err = controller.store.RemoveResourceOffer(offer.ID) | ||
| if err != nil { | ||
| controller.log.Error("remove resource offer failed: %s", err) |
There was a problem hiding this comment.
Perhaps we should log some context around the resource offer to help debug issues
There was a problem hiding this comment.
Yeah great idea! Added the resource provider ID and the ID of the offer that could not be removed.
pkg/solver/controller.go
Outdated
| @@ -404,23 +404,35 @@ func (controller *SolverController) addResourceOffer(resourceOffer data.Resource | |||
| return ret, nil | |||
| } | |||
|
|
|||
| func (controller *SolverController) removeResourceOfferByResourceProvider(ID string) error { | |||
| // Remove resource offers in an unmatched DealNegotiating[0] state | |||
| func (controller *SolverController) removeUnmatchedResourceOffers(ID string) error { | |||
There was a problem hiding this comment.
[nit] lets change ID to resourceProviderID for more clarity cause I got confused at first and though this was supposed to be the resource offer id 😅
There was a problem hiding this comment.
Yeah totally. We lost some semantic meaning renaming the function. Pushed an update change ID to resourceProviderID.
pkg/solver/controller.go
Outdated
| // TODO Remove before merge | ||
| offersAfter, err := controller.store.GetResourceOffers(store.GetResourceOffersQuery{ | ||
| ResourceProvider: ID, | ||
| }) |
There was a problem hiding this comment.
[nit] Reminder to remove once we're ready to merge
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Printf("+++ resource offers after removal: %+v\n", offersAfter) |
There was a problem hiding this comment.
[nit] Reminder to remove once we're ready to merge
We remove resource offers in a unmatched DealNegotiating[0] state to avoid matching them when their resource provider is not connected.
bgins
force-pushed
the
bgins/fix-resource-offer-cleanup
branch
from
994bef2 to
6a7cc7d
Compare
Summary
This pull request makes the following changes:
removeResourceOfferByResourceProvidertoremoveUnmatchedResourceOffersThis pull request fixes a bug where we sometimes remove the wrong resource offer on disconnect. See #467 for details.
The updated code removes resource offers in a unmatched
DealNegotiating[0]state to avoid matching them when their resource provider is not connected. This approach has the additional benefit of removing multiple resource offers if the resource provider has offered them.Task/Issue reference
Closes: #467
Test plan
We have included a temporary commit (
994bef2) to demonstrate resource offer removal. The following steps are recommended for testing:DealNegotiating[0]state.DealNegotiating[0]state that gets removed, but the resource offer from running the job (in aResultsAccepted[3]) state should be preserved.Related issues or PRs (optional)
Epic: https://github.com/Lilypad-Tech/internal/issues/367