Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Add an OCI annotation for sandbox log directory.#1056

Merged
Random-Liu merged 1 commit intocontainerd:masterfrom
Random-Liu:add-sandbox-log-dir-annotation
Mar 8, 2019
Merged

Add an OCI annotation for sandbox log directory.#1056
Random-Liu merged 1 commit intocontainerd:masterfrom
Random-Liu:add-sandbox-log-dir-annotation

Conversation

@Random-Liu
Copy link
Copy Markdown
Member

@Random-Liu Random-Liu commented Feb 23, 2019

Add an OCI annotation to pass down the pod log directory.

This is useful for sandbox runtime which has pod level logs, e.g. gvisor. Actually I chatted with @resouer about this, and he told me that kata used to have pod level logs as well.

Especially after kubernetes/kubernetes#74441 lands, Kubelet will be responsible for monitoring the disk usage and cleanup these logs.

@mikebrow Are you ok with cherrypicking this to release/1.2? It doesn't break anyone. :)

Signed-off-by: Lantao Liu [email protected]

Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM
see grammar nits
+1 on cherry pick(s)

SandboxID = "io.kubernetes.cri.sandbox-id"

// SandboxLogDir is the pod log directory annotation.
// If the sandbox needs to generate any log, it can put it into this directory.
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.

/s/can/will/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done


// SandboxLogDir is the pod log directory annotation.
// If the sandbox needs to generate any log, it can put it into this directory.
// Kubelet will be responsible to:
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.

/s/to/for/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

// SandboxLogDir is the pod log directory annotation.
// If the sandbox needs to generate any log, it can put it into this directory.
// Kubelet will be responsible to:
// 1) Monitoring the disk usage of the log, and include it as part of the pod
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.

/s/include/including/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

// Kubelet will be responsible to:
// 1) Monitoring the disk usage of the log, and include it as part of the pod
// ephemeral storage usage.
// 2) Cleanup the logs when the pod is deleted.
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.

/s/Cleanup/Cleaning up/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

// 1) Monitoring the disk usage of the log, and include it as part of the pod
// ephemeral storage usage.
// 2) Cleanup the logs when the pod is deleted.
// NOTE that kubelet is not responsible for rotating the logs.
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.

/s/Note that/Note: Kublet/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

@Random-Liu Random-Liu force-pushed the add-sandbox-log-dir-annotation branch from bc2559b to 9eabcf5 Compare March 7, 2019 00:43
@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 7, 2019
@Random-Liu
Copy link
Copy Markdown
Member Author

Addressed comments about the comment. Apply LGTM based on #1056 (review)

Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM

@Random-Liu
Copy link
Copy Markdown
Member Author

/test pull-cri-containerd-node-e2e

@Random-Liu Random-Liu merged commit 8a0bd84 into containerd:master Mar 8, 2019
@Random-Liu Random-Liu deleted the add-sandbox-log-dir-annotation branch March 8, 2019 09:32
Random-Liu added a commit that referenced this pull request Mar 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants