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

chore: remove pendingmessage functionality #246

Merged
merged 5 commits into from
May 2, 2024

Conversation

shreddedbacon
Copy link
Member

@shreddedbacon shreddedbacon commented Jan 5, 2024

Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated
  • PR title is ready for changelog and subsystem label(s) applied

The pending messages functionality of the remote-controller never really worked well, and in clusters that typically do exhibit signs of queue instability the pending messages functionality didn't actually work in a lot of cases.

Rather than try and support a feature that adds additional complexity for not much gain, this PR is to remove it. It retains a command line flag but in a noop capacity, which should be removed in a future version.

The removal of this feature will mean that if there is a problem communicating back to rabbitmq during a build, that there is the possibility that the logs for the build may be lost, or the status of the build may be unknown. PR #224 also helped reduce the need for the pending messages check, along with older PRs that enhanced build cancellation functionality to attempt to retrieve the status of builds if a cancellation is received, #211 and associated PRS

Adjusting the flags num-builds-to-keep and num-build-pods-to-keep beyond the default of 1 can also help to retain logs longer in clusters that may exhibit communication issues with rabbitmq more frequently, but generally should not be required.

@shreddedbacon shreddedbacon marked this pull request as ready for review January 7, 2024 21:10
@shreddedbacon shreddedbacon requested a review from bomoko January 8, 2024 01:09
Copy link
Contributor

@bomoko bomoko left a comment

Choose a reason for hiding this comment

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

Looks good, makes sense.

Nice simplification - especially if the existing mechanism is flaky.

@shreddedbacon
Copy link
Member Author

Just looking back at this, there are considerable changes to the CRD here that "may (unlikely)" cause problems if we removed them from the CRD. I think I will leave this for now, but create a new PR that refactors the CRD to create a newer v1beta2 api version where this information doesn't exist, and we can start work on being able to deprecate APIs, as this would also make it easier in the future to do more drastic CRD changes if required.

@shreddedbacon shreddedbacon changed the base branch from main to main-v1beta2 May 2, 2024 01:15
@shreddedbacon shreddedbacon merged commit 710e627 into main-v1beta2 May 2, 2024
9 checks passed
@shreddedbacon shreddedbacon deleted the remove-pendingmessages-feature branch May 2, 2024 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants