Skip to content

pleg: ensure the cache is updated whenever container are removed#22117

Closed
yujuhong wants to merge 1 commit into
kubernetes:masterfrom
yujuhong:pleg_states
Closed

pleg: ensure the cache is updated whenever container are removed#22117
yujuhong wants to merge 1 commit into
kubernetes:masterfrom
yujuhong:pleg_states

Conversation

@yujuhong

Copy link
Copy Markdown
Contributor

Even though we don't rely on the cache for garbage collection yet, we should
keep it up-to-date.

Even though we don't rely on the cache for garbage collection yet, we should
keep it up-to-date.
@yujuhong yujuhong added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Feb 27, 2016
@yujuhong

Copy link
Copy Markdown
Contributor Author

This doesn't affect correctness, but pleg may accumulate more removed containers internally over time.

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 27, 2016
@k8s-github-robot

Copy link
Copy Markdown

Labelling this PR as size/S

@k8s-bot

k8s-bot commented Feb 27, 2016

Copy link
Copy Markdown

GCE e2e build/test passed for commit 3923695.

@Random-Liu

Copy link
Copy Markdown
Member

@k8s-bot test this issue #22111

@k8s-bot

k8s-bot commented Feb 27, 2016

Copy link
Copy Markdown

GCE e2e build/test passed for commit 3923695.

// need to do it again.
return nil
// We already reported that the container died before.
return &PodLifecycleEvent{ID: podID, Type: ContainerRemoved, Data: cid}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yujuhong There is a bug in the old implementation.
If generateEvent returns nil here, the corresponding podRecord will never be removed in the following loop. This seems to be a memory leak.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yujuhong Just saw your comment above. I think we are talking about the same thing, right? :)

This doesn't affect correctness, but pleg may accumulate more removed containers internally over time.

@Random-Liu

Copy link
Copy Markdown
Member

LGTM

@Random-Liu Random-Liu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2016
@k8s-github-robot

Copy link
Copy Markdown

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot

k8s-bot commented Feb 29, 2016

Copy link
Copy Markdown

GCE e2e build/test passed for commit 3923695.

@yujuhong

yujuhong commented Mar 1, 2016

Copy link
Copy Markdown
Contributor Author

Closing since the commit has been merged as part of #22155

@Random-Liu

Copy link
Copy Markdown
Member

SGTM

@yujuhong yujuhong closed this Mar 1, 2016
@yujuhong yujuhong deleted the pleg_states branch April 22, 2016 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants