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

exec and other operations should support a default container #4355

Closed
shawkins opened this issue Aug 26, 2022 · 6 comments · Fixed by #4505
Closed

exec and other operations should support a default container #4355

shawkins opened this issue Aug 26, 2022 · 6 comments · Fixed by #4505
Assignees
Milestone

Comments

@shawkins
Copy link
Contributor

Is your enhancement related to a problem? Please describe

kubectl if you don't specify the container argument will assume the first container for operations like exec. This may be leading to confusion / errors such as #4353 and #4319

Describe the solution you'd like

Similar to kubectl the fabric8 client should default to the first container when working with a multi-container pod.

Describe alternatives you've considered

No response

Additional context

No response

@scottmarlow
Copy link

scottmarlow commented Aug 26, 2022

Is the idea to change all calls to io.fabric8.kubernetes.client.dsl.internal.PodOperationContext#withContainerId(String containerId) to use the first containerId from an internal collection of container IDs? I don't know any of the internals and likely wouldn't be helpful in creating a pr but still am curious. Thanks for opening the issue!

@shawkins
Copy link
Contributor Author

Is the idea to change all calls to io.fabric8.kubernetes.client.dsl.internal.PodOperationContext#withContainerId(String containerId) to use the first containerId from an internal collection of container IDs?

The logic would roughly be - if a containerId is not set, get the deployment and use the first container name as the id.

If there's a concern about backwards compatibility with how we're currently doing things, then we'll only use the first container when there are multiple - probably just need to check the kubectl logic for consistency.

@scottmarlow
Copy link

The logic would roughly be - if a containerId is not set, get the deployment and use the first container name as the id.

👍

If there's a concern about backwards compatibility with how we're currently doing things, then we'll only use the first container when there are multiple - probably just need to check the kubectl logic for consistency.

What are the possible user cases to be handled? Perhaps something like:

  1. multiple containers in a single pod.
  2. single container in a single pod.
  3. Something else?

@shawkins
Copy link
Contributor Author

What are the possible user cases to be handled?

Just the first one is what is currently problematic wrt user expectations. kubectl defaults to the first container, the fabric8 client does not.

However a narrow fix just for exec may not be the most appropriate - there are a lot of locations where a container may be specified so we'll need to determine all the places were defaulting is applicable.

@manusa
Copy link
Member

manusa commented Aug 29, 2022

Similar to kubectl the fabric8 client should default to the first container when working with a multi-container pod.

I thought this was already implemented (we do have something like this on JKube for Watch and Log operations).

What are the possible user cases to be handled? Perhaps something like:

Summary of exec and other Pod operations proposed behaviors with regards to containers:

  1. Pod has no Containers, should <blank>
  2. Pod has a single Container, the single Container should be used by default (no need to call inContainer)
  3. Pod has a single Container and inContainer is used with a different id/name, should _<blank_throw_exception?or_use_single_and_log_warning?>
  4. Pod has >1 Container, the first container should be used by default (no need to call inContainer) <and_log_warning?>
  5. Pod has >1 Container and inContainer is used with a non-existent id/name, should _<blank_throw_exception?or_use_first_and_log_warning?>

@shawkins shawkins added this to the 6.2.0 milestone Aug 30, 2022
@scottmarlow
Copy link

scottmarlow commented Sep 1, 2022

What is the impact of this issue if I have two (Jenkins pipeline defined) containers, the first is called jakartaeetck-ci and the second is called james-mail. Please assume that my first container jakartaeetck-ci is considered the main (test) application and the second container james-mail is considered a sidecar that runs an email server in the background.

With the fix applied, will the first container always be considered the main application and other containers always be considered sidecars? Is this the right terminology?

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

Successfully merging a pull request may close this issue.

3 participants