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

Change the way we figure out container to read the artifacts from #2310

Merged
merged 7 commits into from
Sep 7, 2023

Conversation

viveksinghggits
Copy link
Contributor

Change Overview

While running the kanister function, to read the artifact that the phase has produced we get the logs of the kanister function pod (let's say KubeTask). If there is just one container in the KubeTask pod in that case things would work but if there are multiple contianers in the pod (let's say service mesh injected one), in that case the logic that we have to figure out the container name was not efficient. If was relying on the first container of the pod.

This PR changes that logic to read the container name from podOptions or fallback to the default container name. This should work in all the cases, if container name is not specified while creating podOptions we will use default container name everywhere otherwise we will use the container name that is specified in the podOptions.

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

While running the kanister function, to read the artifact that the phase has
produced we get the logs of the kanister function pod (let's say `KubeTask`).
If there is just one container in the KubeTask pod in that case things would
work but if there are multiple contianers in the pod (let's say service mesh
injected one), in that case the logic that we have to figure out the container
name was not efficient. If was relying on the first container of the pod.

This commit changes that logic to read the container name from `podOptions` or
fallback to the default container name. This should work in all the cases, if
container name is not specified while creating `podOptions` we will use default
container name everywhere otherwise we will use the container name that is spec
ified in the podOptions.
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

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 Sep 7, 2023
@viveksinghggits viveksinghggits changed the title Change how we figure out container to get logs from Change the way we figure out container to read the artifacts from Sep 7, 2023
When we create pod for a repo server CR, we try to figure out the `podOverride`
which added (I am not sure why) a container with hardcoded name. This commit
changes that and we figure out the container name from podOptions now.
@viveksinghggits
Copy link
Contributor Author

@kale-amruta FYI, I have added some code related to repo server as well.

@viveksinghggits
Copy link
Contributor Author

@pavannd1 the main change is in pod_controller.go that needs to be reviewed. Rest of the change is just because of that.

pkg/kube/pod_controller.go Outdated Show resolved Hide resolved
pkg/kube/pod_controller_test.go Outdated Show resolved Hide resolved
@viveksinghggits
Copy link
Contributor Author

/ok-to-test

Kanister automation moved this from In Progress to Reviewer approved Sep 7, 2023
@pavannd1 pavannd1 added the kueue label Sep 7, 2023
@mergify mergify bot merged commit 86c1668 into master Sep 7, 2023
14 checks passed
Kanister automation moved this from Reviewer approved to Done Sep 7, 2023
@mergify mergify bot deleted the containerfrom-po branch September 7, 2023 23:33
pavannd1 added a commit that referenced this pull request Sep 8, 2023
mergify bot pushed a commit that referenced this pull request Sep 8, 2023
viveksinghggits added a commit that referenced this pull request Sep 8, 2023
viveksinghggits added a commit that referenced this pull request Sep 20, 2023
mergify bot pushed a commit that referenced this pull request Sep 20, 2023
…tifacts from"" (#2316)

* Revert "Revert "Change the way we figure out container to read the artifacts from (#2310)" (#2314)"

This reverts commit dad529b.

* Make `ContainerNameFromPodOptsOrDefault` nil safe
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

2 participants