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 non rails worker starting on podified #23116

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

agrare
Copy link
Member

@agrare agrare commented Jul 30, 2024

The current_pods Concurrent::Hash doesn't have the full pod object so we have to move the logic for checking if a pod is running down into save_pod

WIP pending tests covering more of these methods specifically sync_starting_workers

Dependents:

@agrare agrare added the bug label Jul 30, 2024
@agrare agrare changed the title Fix non rails worker starting on podified [WIP] Fix non rails worker starting on podified Jul 30, 2024
@miq-bot miq-bot added the wip label Jul 30, 2024
@agrare agrare changed the title [WIP] Fix non rails worker starting on podified Fix non rails worker starting on podified Jul 30, 2024
@miq-bot miq-bot removed the wip label Jul 30, 2024
@Fryguy
Copy link
Member

Fryguy commented Jul 30, 2024

@agrare Is this ready? (I still see WIP comment in the OP, so I wasn't sure)

@Fryguy
Copy link
Member

Fryguy commented Jul 30, 2024

@jrafanie Please also review.

@agrare
Copy link
Member Author

agrare commented Jul 30, 2024

Yeah I added the specs mentioned in the description

@Fryguy
Copy link
Member

Fryguy commented Jul 30, 2024

@agrare When we discussed, we talked about maybe treating all workers like non-rails workers, and removing the logic from run_single_worker. Is that a future thing and this is tactical, or did you go in a different direction?

@agrare
Copy link
Member Author

agrare commented Jul 30, 2024

When we discussed, we talked about maybe treating all workers like non-rails workers, and removing the logic from run_single_worker. Is that a future thing and this is tactical, or did you go in a different direction?

I found this issue while working on that, I have another PR that is built on this.

@agrare
Copy link
Member Author

agrare commented Jul 30, 2024

Hm the spec failures are green locally, wonder if something in another plugin broke master

@agrare
Copy link
Member Author

agrare commented Jul 30, 2024

That's very weird, I just restarted this without changing any commits and it is green now 🤷

@agrare agrare force-pushed the fix_non_rails_worker_starting branch from f730cac to ed8d21a Compare July 30, 2024 15:39
@miq-bot
Copy link
Member

miq-bot commented Jul 30, 2024

Checked commits agrare/manageiq@281736e~...ed8d21a with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
4 files checked, 0 offenses detected
Everything looks fine. 🏆

@Fryguy
Copy link
Member

Fryguy commented Jul 31, 2024

LGTM - would like another set of eyes - @jrafanie ?

@jrafanie
Copy link
Member

jrafanie commented Aug 1, 2024

LGTM, just a minor question above.

@jrafanie jrafanie merged commit 89cb15b into ManageIQ:master Aug 1, 2024
8 checks passed
@agrare agrare deleted the fix_non_rails_worker_starting branch August 1, 2024 14:52
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.

4 participants