Skip to content

make sure init exited event is last sent#10650

Closed
ningmingxiao wants to merge 1 commit intocontainerd:mainfrom
ningmingxiao:fix_event_lost_new2
Closed

make sure init exited event is last sent#10650
ningmingxiao wants to merge 1 commit intocontainerd:mainfrom
ningmingxiao:fix_event_lost_new2

Conversation

@ningmingxiao
Copy link
Contributor

@ningmingxiao ningmingxiao commented Aug 28, 2024 •
edited
Loading

delete s.pendingExecs and make code easier.

cc @corhere @laurazard @samuelkarp

@k8s-ci-robot
Copy link

k8s-ci-robot commented Aug 28, 2024

Hi @ningmingxiao. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@dosubot dosubot bot added the area/runtime Runtime label Aug 28, 2024
@ningmingxiao ningmingxiao force-pushed the fix_event_lost_new2 branch 9 times, most recently from da26564 to 42a7cc3 Compare August 28, 2024 10:44
laurazard
laurazard requested changes Aug 28, 2024
View reviewed changes
Copy link
Member

@laurazard laurazard left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR just reinvents s.pendingExecs (in the form of s.execRunned – but without accounting for the fact that multiple execs can be pending at once), but adds 3 (three) more state-structs where we store redundant information: s.lastExecEventSent, s.execRunned, and s.execable.

At the very least, a few of those are redundant, accesses to these new fields aren't synchronized, and you only wait for len(s.runningExecsList) == 0 when there are no pending execs which means if there are pending execs you skip over any running exec and immediately send the init exit event.

Access to the new fields aren't synchronized either.

@laurazard laurazard mentioned this pull request Aug 28, 2024
Merged
@ningmingxiao ningmingxiao force-pushed the fix_event_lost_new2 branch 15 times, most recently from 024e886 to f3e4bd1 Compare September 4, 2024 06:18
@ningmingxiao ningmingxiao force-pushed the fix_event_lost_new2 branch 2 times, most recently from 8201436 to ecb82bc Compare September 4, 2024 06:51
@ningmingxiao
Copy link
Contributor Author

ningmingxiao commented Sep 4, 2024

can you test this pr ? @samuelkarp

@samuelkarp
Copy link
Member

samuelkarp commented Sep 4, 2024

I'll look tomorrow but in the meantime you should be able to pull my PR locally and test it too. Steps would be:

  1. Check out your PR and make && sudo make install
  2. Check out my PR and run the test

@samuelkarp samuelkarp mentioned this pull request Sep 4, 2024
Closed
@samuelkarp
Copy link
Member

samuelkarp commented Sep 4, 2024

This does pass the test in #10649. I would lean toward the approach in #10651 however as it seems more straightforward to me to follow.

@samuelkarp
Copy link
Member

samuelkarp commented Sep 6, 2024

Closing in favor of #10651

@samuelkarp samuelkarp closed this Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurazard laurazard laurazard requested changes

Assignees

No one assigned

Labels

area/runtime Runtime needs-ok-to-test size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ningmingxiao @k8s-ci-robot @samuelkarp @laurazard