Skip to content

[release/1.6] Require plugins to succeed after registering readiness #9166

Merged
fuweid merged 2 commits intocontainerd:release/1.6from
dmcgowan:backport-1.6-readiness-hang-fix
Oct 14, 2023
Merged

[release/1.6] Require plugins to succeed after registering readiness #9166
fuweid merged 2 commits intocontainerd:release/1.6from
dmcgowan:backport-1.6-readiness-hang-fix

Conversation

@dmcgowan
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan commented Sep 29, 2023

Fixes hang when plugin which registers readiness fail. Additional fix in CRI to register readiness later on in its initialization.

Backport #9153 #9164

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Sep 30, 2023

Could you swap the commit order to help bisecting?

@dmcgowan dmcgowan force-pushed the backport-1.6-readiness-hang-fix branch from a8fb8e7 to 219bbb7 Compare September 30, 2023 03:54
@dmcgowan
Copy link
Copy Markdown
Member Author

dmcgowan commented Sep 30, 2023

@AkihiroSuda done, still need the Go update for the WIndows build

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Sep 30, 2023 •
edited
Loading

Interesting; looks like it doesn't compile on go1.19 on Windows;

github.com/containerd/containerd/cmd/containerd
C:\hostedtoolcache\windows\go\1.19.12\x64\pkg\tool\windows_amd64\link.exe: running gcc failed: exit status 1
C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-2608677180\000007.o: in function `_cgo_preinit_init':
\\_\_\runtime\cgo/gcc_libinit_windows.c:40: undefined reference to `__imp___iob_func'
C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-2608677180\000007.o: in function `x_cgo_notify_runtime_init_done':
\\_\_\runtime\cgo/gcc_libinit_windows.c:105: undefined reference to `__imp___iob_func'
C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-2608677180\000007.o: in function `_cgo_beginthread':
\\_\_\runtime\cgo/gcc_libinit_windows.c:149: undefined reference to `__imp___iob_func'
C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-2608677180\000008.o: in function `x_cgo_thread_start':
\\_\_\runtime\cgo/gcc_util.c:18: undefined reference to `__imp___iob_func'
collect2.exe: error: ld returned 1 exit status

edit: 🤦‍♂️🙈 ignore me: missed the comment higher up;

still need the Go update for the WIndows build

AkihiroSuda and others added 2 commits October 9, 2023 15:59
@AkihiroSuda @dmcgowan
cri: call RegisterReadiness after NewCRIService
8f5eba3 
`NewCRIService()` may easily fail and its error has to be ignored
unless the CRI plugin is in the `required_plugins` list.

Now this has to be called before `RegisterReadiness()`, as
PR 9153 "Require plugins to succeed after registering readiness"
was merged on 2023-09-29.

Fix issue 9163: `[Regression in main (2023-09-29)]: containerd-rootless.sh doesn't start up`

Signed-off-by: Akihiro Suda <[email protected]>
(cherry picked from commit 5365f4b)
Signed-off-by: Derek McGowan <[email protected]>
@dmcgowan
Require plugins to succeed after registering readiness
5223bf3 
When readiness is registered on initialization, the plugin must not
fail. When such a plugin fails, containerd will hang on the readiness
condition.

Signed-off-by: Derek McGowan <[email protected]>
(cherry picked from commit e725440)
Signed-off-by: Derek McGowan <[email protected]>
@dmcgowan dmcgowan force-pushed the backport-1.6-readiness-hang-fix branch from 219bbb7 to 5223bf3 Compare October 9, 2023 23:07
fuweid
fuweid approved these changes Oct 14, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid merged commit 2d2661e into containerd:release/1.6 Oct 14, 2023
@akhilerm akhilerm mentioned this pull request Nov 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henry118 henry118 henry118 approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@fuweid fuweid fuweid approved these changes

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

@dmcgowan @AkihiroSuda @thaJeztah @henry118 @fuweid