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

fix: Remove resource offers in DealNegotiating[0] state on disconnect #475

Merged
merged 4 commits into from
Jan 6, 2025

Conversation

bgins
Copy link
Contributor

@bgins bgins commented Dec 18, 2024

Summary

This pull request makes the following changes:

  • Fix remove resource offers on resource provider disconnect
  • Rename removeResourceOfferByResourceProvider to removeUnmatchedResourceOffers

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:

  • Start the stack
  • Stop the resource provider and observe the resource offers before and after. All offers should be removed because the one posted offer was in a DealNegotiating[0] state.
  • Restart the resource provider and run a job
  • Wait a moment, and stop the resource provider. The resource provider should have added a new resource offer in a DealNegotiating[0] state that gets removed, but the resource offer from running the job (in a ResultsAccepted[3]) state should be preserved.

Related issues or PRs (optional)

Epic: https://github.com/Lilypad-Tech/internal/issues/367

Sorry, something went wrong.

@bgins bgins requested a review from a team as a code owner December 18, 2024 16:34
@cla-bot cla-bot bot added the cla-signed label Dec 18, 2024
@bgins bgins changed the title Bgins/fix resource offer cleanup fix: Remove resource offers in DealNegotiating[0] state on disconnect Dec 18, 2024
@github-actions github-actions bot added the fix label Dec 18, 2024
Comment on lines 416 to 417
// TODO Remove before merge
fmt.Printf("+++ resource offers before removal: %+v\n", resourceOffers)
Copy link
Collaborator

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

Copy link
Collaborator

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 ?

Copy link
Contributor Author

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.

Copy link
Collaborator

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!

Copy link
Contributor Author

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. ✨

Comment on lines 428 to 432
// TODO Remove before merge
offersAfter, err := controller.store.GetResourceOffers(store.GetResourceOffersQuery{
ResourceProvider: ID,
})
Copy link
Collaborator

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

if err != nil {
return err
}
fmt.Printf("+++ resource offers after removal: %+v\n", offersAfter)
Copy link
Collaborator

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

@bgins bgins force-pushed the bgins/fix-resource-offer-cleanup branch from 994bef2 to 6a7cc7d Compare December 19, 2024 21:55
@bgins bgins requested a review from narbs91 January 6, 2025 22:23
@bgins bgins force-pushed the bgins/fix-resource-offer-cleanup branch 2 times, most recently from dc364b9 to d563c7e Compare January 6, 2025 23:12
bgins added 4 commits January 6, 2025 15:32
We remove resource offers in a unmatched DealNegotiating[0] state to
avoid matching them when their resource provider is not connected.
@bgins bgins force-pushed the bgins/fix-resource-offer-cleanup branch from d563c7e to 2df9fad Compare January 6, 2025 23:32
@bgins bgins merged commit 77b86af into main Jan 6, 2025
4 checks passed
@bgins bgins deleted the bgins/fix-resource-offer-cleanup branch January 6, 2025 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource offer cleanup sometimes removes the wrong offers
2 participants