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

Fix issue with missing container name and namespace #2269

Merged
merged 7 commits into from
Aug 16, 2023

Conversation

chaitalisg
Copy link
Contributor

@chaitalisg chaitalisg commented Aug 15, 2023

Change Overview

When creating pods with sidecars, we have an issue with writing files or streaming logs from these pods, because such operations requires container name.
This PR fixes this issue.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@github-actions
Copy link
Contributor

Thanks for submitting this pull request 🎉. The team will review it soon and get back to you.

If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document.

@infraq infraq added this to In Progress in Kanister Aug 15, 2023
@chaitalisg chaitalisg marked this pull request as ready for review August 16, 2023 16:49
@chaitalisg chaitalisg changed the title Fix issue with missing container name Fix issue with missing container name and namespace Aug 16, 2023
Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/kube/pod_controller.go Outdated Show resolved Hide resolved
Kanister automation moved this from In Progress to Reviewer approved Aug 16, 2023
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
@mergify mergify bot merged commit 0fa348f into master Aug 16, 2023
14 checks passed
Kanister automation moved this from Reviewer approved to Done Aug 16, 2023
@mergify mergify bot deleted the copy-of-container-name-for-filewriter branch August 16, 2023 21:41
@viveksinghggits
Copy link
Contributor

Hi @chaitalisg,
Like mentioned in the PR description in case of multiple containers in a pod when we try to read logs there was a problem. But how do we know that it's the first container that we want to read the logs from?
cc @pavannd1

@chaitalisg
Copy link
Contributor Author

Hi @chaitalisg,
Like mentioned in the PR description in case of multiple containers in a pod when we try to read logs there was a problem. But how do we know that it's the first container that we want to read the logs from?
cc @pavannd1

Good question. It is a calculated risk. When pods are created with sidecars, sidecar containers are placed after main container. So, I am using the first container. Let me know if you know a way to make this better!

@viveksinghggits
Copy link
Contributor

When pods are created with sidecars, sidecar containers are placed after main container.

I don't think this is the case always.
I think we should pass the container name explicitly instead. For example if we have a utility to exec into a pod it should expect a container name, similarly in this case since we are trying to get logs of a container we should explicitly expect the container name.
This can default to the container name that we set in podOptions while creating the pod because the is the that we would be interested in.

@viveksinghggits
Copy link
Contributor

I have raised this PR that fixes above problem
#2310 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants