Skip to content

nil check to avoid panic on upgrade#7809

Merged
mxpv merged 1 commit intocontainerd:mainfrom
mikebrow:check-deep-copies-on-restart
Dec 14, 2022
Merged

nil check to avoid panic on upgrade#7809
mxpv merged 1 commit intocontainerd:mainfrom
mikebrow:check-deep-copies-on-restart

Conversation

@mikebrow
Copy link
Copy Markdown
Member

to fix #7806
Signed-off-by: Mike Brown [email protected]

@mikebrow
Copy link
Copy Markdown
Member Author

introduced here: #6517
@ruiwen-zhao FYI.. PTAL

@mikebrow mikebrow added area/cri Container Runtime Interface (CRI) cherry-pick/1.6.x impact/breaking labels Dec 13, 2022
@MikeZappa87
Copy link
Copy Markdown
Member

/lgtm

Copy link
Copy Markdown
Member

@kzys kzys 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 to me. We should have some tests here.

@dcantah
Copy link
Copy Markdown
Member

dcantah commented Dec 13, 2022

How are there even nil entries in the slice to begin with?

@ruiwen-zhao
Copy link
Copy Markdown
Member

Thanks for fixing it!

/lgtm

@mikebrow
Copy link
Copy Markdown
Member Author

mikebrow commented Dec 14, 2022

How are there even nil entries in the slice to begin with?

good question .. #7661 is related

@dcantah
Copy link
Copy Markdown
Member

dcantah commented Dec 14, 2022

Weird.. I can't see how that could cause nil entries, but couldn't that code also just be:

hugepageLimits := make([]*runtime.HugepageLimit, len(s.Resources.Linux.HugepageLimits))
for idx, l := range s.Resources.Linux.HugepageLimits {
        limit := &runtime.HugepageLimit{
 		PageSize: l.PageSize,
		Limit:    l.Limit,
	}
 	hugepageLimits[idx] = limit
}

@mikebrow
Copy link
Copy Markdown
Member Author

mikebrow commented Dec 14, 2022

Weird.. I can't see how that could cause nil entries, but couldn't that code also just be:

hugepageLimits := make([]*runtime.HugepageLimit, len(s.Resources.Linux.HugepageLimits))
for idx, l := range s.Resources.Linux.HugepageLimits {
        limit := &runtime.HugepageLimit{
 		PageSize: l.PageSize,
		Limit:    l.Limit,
	}
 	hugepageLimits[idx] = limit
}

Yeah we could use indexing won't fix the exception though..

I've not found proof of the root cause. I asked for a pod spec.. My repro was local via abusing the deep copy code..

@dcantah
Copy link
Copy Markdown
Member

dcantah commented Dec 14, 2022

On mobile or I'd try and look also for a little 😭. Let's get this in though and we can plug away

@mxpv mxpv merged commit 371e27f into containerd:main Dec 14, 2022
@dmcgowan dmcgowan added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch and removed cherry-pick/1.6.x labels Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cri Container Runtime Interface (CRI) cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch impact/breaking priority/P1

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Segmentation fault upon startup

7 participants