Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tutorial about running Pods with sidecar containers #46825

Merged

Conversation

SergeyKanzhelev
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev commented Jun 14, 2024

/sig node

more questions about sidecars warranted me to write this down.

It was discussed at Sidecar WG meeting: https://docs.google.com/document/d/1E1guvFJ5KBQIGcjCrQqFywU9_cBQHRtHvjuqcVbCXvU/edit#bookmark=id.u4zmu7j9vn31

This is the first step in this tutorial. It may require more work in the following areas:

  1. I am out of my depth on what would be the best practices writing tools and mutating web hooks that will preserve unknown fields: Add tutorial about running Pods with sidecar containers #46825 (comment) Extended documentation or even a link to other page that may explain this would go a long way. (Document the best practices when implementing mutating webhook #47465)
  2. Explain how to implement sidecar containers without the built-in feature. I am not sure how deep we want to go there to cover edge cases. Perhaps just cover the very basic scenario.
  3. Fully working example of a sidecar container that handles termination properly would go a long way explaining concepts.

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 14, 2024
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Jun 14, 2024
Copy link

netlify bot commented Jun 14, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 4bc6afb
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/66b52ce69800d70008c5fb34
😎 Deploy Preview https://deploy-preview-46825--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

It's great that we want to help people learn about running sidecars.

How about adding a tutorial page, eg https://k8s.io/docs/tutorials/pod-sidecar-containers/?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 17, 2024
@SergeyKanzhelev
Copy link
Member Author

It's great that we want to help people learn about running sidecars.

How about adding a tutorial page, eg https://k8s.io/docs/tutorials/pod-sidecar-containers/?

Updated.

@SergeyKanzhelev SergeyKanzhelev force-pushed the sidecarsTrobleshooting branch 3 times, most recently from 71124fd to e48c63d Compare June 21, 2024 16:17
@SergeyKanzhelev
Copy link
Member Author

Any advice what does this may mean:

Error: error building site: assemble: "/opt/build/repo/content/en/docs/tutorials/configuration/pod-sidecar-containers.md:1:1": got positional parameter 'or'. Cannot mix named and positional parameters

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Any advice what does this may mean:

Error: error building site: assemble: "/opt/build/repo/content/en/docs/tutorials/configuration/pod-sidecar-containers.md:1:1": got positional parameter 'or'. Cannot mix named and positional parameters

See inline feedback (the feature-state suggestion)

sftim
sftim previously requested changes Jun 23, 2024
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Questions in my mind:

  • After following all the steps in the tutorial, how many Pods are still running (did we explain how to clean up?)
  • After following all the steps in the tutorial, how many Pods that have a sidecar container have started and become healthy?
  • After following all the steps in the tutorial, how many other Pods started (eg to help make a point about failure handling)?

If we're not helping the reader to run any Pods that have sidecars, the tutorial needs expanding until that changes.

@sftim
Copy link
Contributor

sftim commented Jun 23, 2024

/retitle Add tutorial about running Pods with sidecar containers

@k8s-ci-robot k8s-ci-robot changed the title Adopting SidecarContainers section Add tutorial about running Pods with sidecar containers Jun 23, 2024
@sftim sftim dismissed their stale review June 27, 2024 19:48

Feedback was stale

@sftim
Copy link
Contributor

sftim commented Jun 27, 2024

My key feedback is (still) the “questions in my mind” from #46825 (review)

@T-Lakshmi
Copy link
Contributor

/label tide/merge-method-squash

@SergeyKanzhelev,
can you please squash the commits.

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 29, 2024
@sftim
Copy link
Contributor

sftim commented Aug 8, 2024

/remove-label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot removed the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 8, 2024
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Some more feedback. Please address the existing feedback as well.

@SergeyKanzhelev
Copy link
Member Author

@sftim I think I addressed most of comments. I really want this first step out. See https://kubernetes.slack.com/archives/C0EG7JC6T/p1723141448471749

I added a few follow up steps on top that can be done. @afro-coder I think it will be easiest if we can merge this PR and if you want to contribute, can do as a follow up.

@matthyx
Copy link
Contributor

matthyx commented Aug 21, 2024

great write, 100% correct regarding technicalities, thanks Sergey!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 86cedfc8871cdd6350c9c87996c2a12dba9b34f1

@tengqm
Copy link
Contributor

tengqm commented Aug 25, 2024

/approve
Let's kick this in and iterate over it.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tengqm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2024
@k8s-ci-robot k8s-ci-robot merged commit cada0a9 into kubernetes:main Aug 25, 2024
6 checks passed
@SergeyKanzhelev SergeyKanzhelev deleted the sidecarsTrobleshooting branch September 3, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Development

Successfully merging this pull request may close these issues.

8 participants