-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix: Remove resource offers in DealNegotiating[0] state on disconnect #475
Conversation
pkg/solver/controller.go
Outdated
// TODO Remove before merge | ||
fmt.Printf("+++ resource offers before removal: %+v\n", resourceOffers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Reminder to remove once we're ready to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you still planning to leave these print statements around @bgins ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nope, planning to remove those. Are you done with your review except that? Can remove if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, everything else looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright great! Removed them, waiting for the tests and will merge. ✨
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Reminder to remove once we're ready to merge
pkg/solver/controller.go
Outdated
if err != nil { | ||
return err | ||
} | ||
fmt.Printf("+++ resource offers after removal: %+v\n", offersAfter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Reminder to remove once we're ready to merge
994bef2
to
6a7cc7d
Compare
dc364b9
to
d563c7e
Compare
We remove resource offers in a unmatched DealNegotiating[0] state to avoid matching them when their resource provider is not connected.
d563c7e
to
2df9fad
Compare
Summary
This pull request makes the following changes:
removeResourceOfferByResourceProvider
toremoveUnmatchedResourceOffers
This 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