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.

Sign up for GitHub

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

Jump to bottom

fix(upgrade-job): wait for event worker to exit for errors #319

Merged
merged 1 commit into from
Aug 29, 2023
Merged

fix(upgrade-job): wait for event worker to exit for errors #319

merged 1 commit into from
Aug 29, 2023

Conversation

niladrih
Copy link
Member

@niladrih niladrih commented Aug 22, 2023 •
edited
Loading

The upgrade job main thread starts a worker thread which listens for events and publishes them (as kubernetes events). When the upgrade method exits, the 'EventRecorder' instance for it is dropped. This in turn drops the event channel (used to pass events to the event worker) and also the handle to event worker. The event worker does not have enough time after the channel is dropped, to publish its events and exit.

This PR defers all of the event publishes and actual upgrade steps to an inner function. The caller of this function makes sure the event loop exits before returning.

@niladrih niladrih changed the title fix(upgrade-job): wait for event worker to exit refact(upgrade-job): wait for event worker to exit Aug 22, 2023
@niladrih niladrih changed the title refact(upgrade-job): wait for event worker to exit refactor(upgrade-job): wait for event worker to exit Aug 22, 2023
@niladrih niladrih force-pushed the fix-event-loop branch 2 times, most recently from c8aa732 to f22401d Compare August 22, 2023 05:36
tiagolobocastro
tiagolobocastro reviewed Aug 22, 2023
View reviewed changes
Copy link
Contributor

@tiagolobocastro tiagolobocastro left a comment

Choose a reason for hiding this comment

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

Not sure if waiting on a drop is a good practice...
Could we achieve the same some other way?

@niladrih
Copy link
Member Author

niladrih commented Aug 22, 2023 •
edited
Loading

Not sure if waiting on a drop is a good practice... Could we achieve the same some other way?

Sure, could you suggest a suitable pattern for fleshing out this destructor-like tool.

@tiagolobocastro
Copy link
Contributor

Not sure if waiting on a drop is a good practice... Could we achieve the same some other way?

Sure, could you suggest a suitable pattern for fleshing out this destructor-like tool.

What about just waiting for the work to complete in shutdown_worker?

@niladrih
Copy link
Member Author

Not sure if waiting on a drop is a good practice... Could we achieve the same some other way?

Sure, could you suggest a suitable pattern for fleshing out this destructor-like tool.

What about just waiting for the work to complete in shutdown_worker?

So, no change?

@niladrih
Copy link
Member Author

Closing this as this refactor is not needed.

@niladrih niladrih closed this Aug 23, 2023
@niladrih
Copy link
Member Author

Just noticed that shutdown worker doesn't guarantee publish of error events. Only guarantees publish of successful upgrade. I think this is still required. What might be a good alternative to the drop thing, @tiagolobocastro ?

@niladrih niladrih reopened this Aug 29, 2023
@niladrih niladrih changed the title refactor(upgrade-job): wait for event worker to exit fix(upgrade-job): wait for event worker to exit for errors Aug 29, 2023
@niladrih
Copy link
Member Author

Updated code in this PR to not use the EventRecorder's Drop. Instead it moves the event recorder to one level up in the calling sequence. This let's the event recorder's shutdown_worker method wait for the loop to exit.

@niladrih niladrih force-pushed the fix-event-loop branch 2 times, most recently from 5090581 to 17a1023 Compare August 29, 2023 07:50
@niladrih
fix(upgrade-job): wait for event worker to exit for errors
1ebfa87 
Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>
sinhaashish
sinhaashish approved these changes Aug 29, 2023
View reviewed changes
Copy link
Member

@sinhaashish sinhaashish left a comment

Choose a reason for hiding this comment

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

Tested & LGTM

@niladrih
Copy link
Member Author

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 29, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 9af1f9d into openebs:develop Aug 29, 2023
@niladrih niladrih mentioned this pull request Aug 29, 2023
Merged
bors bot pushed a commit that referenced this pull request Aug 29, 2023
@niladrih
chore(bors): merge pull request #333
6288411 
333: Cherry-pick PR #319 into release/2.4 r=avishnu a=niladrih

Cherry-picks the commits from the following PR(s) into release/2.4:
- #319 

Co-authored-by: Niladri Halder <niladri.halder26@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhilashshetty04 abhilashshetty04 abhilashshetty04 approved these changes

@sinhaashish sinhaashish sinhaashish approved these changes

@dsharma-dc dsharma-dc Awaiting requested review from dsharma-dc

@Abhinandan-Purkait Abhinandan-Purkait Awaiting requested review from Abhinandan-Purkait

@tiagolobocastro tiagolobocastro Awaiting requested review from tiagolobocastro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@niladrih @tiagolobocastro @sinhaashish @Abhinandan-Purkait @abhilashshetty04