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.

Sign up for GitHub

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

Jump to bottom

ExecWithOption - get rid of stdout/stderr buffers #2506

Merged
merged 4 commits into from
Jan 4, 2024
Merged

ExecWithOption - get rid of stdout/stderr buffers #2506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe43ee9
`ExecWithOptions` should not return stdout / stdin
e-sumin Nov 30, 2023
8630a46
Adjust `PodCommandExecutor` and tests
e-sumin Nov 30, 2023
af1eedd
Merge branch 'master' into exec-get-rid-of-buffer
e-sumin Dec 14, 2023
d38c197
Merge branch 'master' into exec-get-rid-of-buffer
e-sumin Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 14 additions & 19 deletions pkg/kube/exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,21 @@ type ExecOptions struct {
// Exec is our version of the call to `kubectl exec` that does not depend on
// k8s.io/kubernetes.
func Exec(cli kubernetes.Interface, namespace, pod, container string, command []string, stdin io.Reader) (string, string, error) {
outbuf := &bytes.Buffer{}
errbuf := &bytes.Buffer{}
opts := ExecOptions{
Command: command,
Namespace: namespace,
PodName: pod,
ContainerName: container,
Stdin: stdin,
Stdout: outbuf,
Stderr: errbuf,
}
return ExecWithOptions(cli, opts)

err := ExecWithOptions(cli, opts)

return strings.TrimSpace(outbuf.String()), strings.TrimSpace(errbuf.String()), err
}

// ExecOutput is similar to Exec, except that inbound outputs are written to the
Expand All @@ -113,32 +120,20 @@ func ExecOutput(cli kubernetes.Interface, namespace, pod, container string, comm
},
}

_, _, err := ExecWithOptions(cli, opts)
return err
return ExecWithOptions(cli, opts)
}

// ExecWithOptions executes a command in the specified container,
// returning stdout, stderr and error. `options` allowed for
// additional parameters to be passed.
func ExecWithOptions(kubeCli kubernetes.Interface, options ExecOptions) (string, string, error) {
// ExecWithOptions executes a command in the specified container, returning an error.
// `options` allowed for additional parameters to be passed.
func ExecWithOptions(kubeCli kubernetes.Interface, options ExecOptions) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

After looking at the desc, I thought, are there use cases where some consumers would need Exec and others would need ExecWithOptions, can we not just expose one. So that it's easier to use these APIs?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the ideal case, of course, it would be better to use ExecWithOptions only. But, unfortunately, both are widely used.

Looking on the code, I figured out that ExecOutput is probably even more redundant.

I can do bigger refactoring and reduce number of execs, but it will be bigger change, so maybe we can do that by two or three steps ? This one, drops buffering in ExecWithOptions, Second one will remove ExecOutput and, if we will agree, that we'd like to do that, the third (and the biggest one) can get rid of pure Exec ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, that I think about it again, I don't think we should block the PR in this discussion. I think names are sufficient enough to not cause any confusion.
If we want to exec with some options use ExecWithOptions otherwise use Exec.

config, err := LoadConfig()
if err != nil {
return "", "", err
}

outbuf := &bytes.Buffer{}
if options.Stdout == nil {
options.Stdout = outbuf
}

errbuf := &bytes.Buffer{}
if options.Stderr == nil {
options.Stderr = errbuf
return err
}

errCh := execStream(kubeCli, config, options)
err = <-errCh
return strings.TrimSpace(outbuf.String()), strings.TrimSpace(errbuf.String()), errors.Wrap(err, "Failed to exec command in pod")
return errors.Wrap(err, "Failed to exec command in pod")
}

func execStream(kubeCli kubernetes.Interface, config *restclient.Config, options ExecOptions) chan error {
Expand Down
6 changes: 3 additions & 3 deletions pkg/kube/exec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (s *ExecSuite) TestExecWithWriterOptions(c *C) {
Stdout: bufout,
Stderr: buferr,
}
_, _, err := ExecWithOptions(s.cli, opts)
err := ExecWithOptions(s.cli, opts)
c.Assert(err, IsNil)
c.Assert(bufout.String(), Equals, testCase.expectedOut)
c.Assert(buferr.String(), Equals, testCase.expectedErr)
Expand Down Expand Up @@ -187,7 +187,7 @@ func (s *ExecSuite) TestErrorInExecWithOptions(c *C) {
ContainerName: "", // use default container
Stdin: nil,
}
_, _, err1 := ExecWithOptions(s.cli, opts) // Output is not needed
err1 := ExecWithOptions(s.cli, opts)
c.Assert(err1, Not(IsNil))

var ee1 *ExecError
Expand All @@ -204,7 +204,7 @@ func (s *ExecSuite) TestErrorInExecWithOptions(c *C) {
opts.Stdout = &bufout
opts.Stderr = &buferr

_, _, err2 := ExecWithOptions(s.cli, opts) // Output is not needed
err2 := ExecWithOptions(s.cli, opts)
c.Assert(err2, Not(IsNil))

var ee2 *ExecError
Expand Down
2 changes: 1 addition & 1 deletion pkg/kube/pod_command_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func (p *podCommandExecutor) Exec(ctx context.Context, command []string, stdin i
)

go func() {
_, _, err = p.pcep.ExecWithOptions(opts)
err = p.pcep.ExecWithOptions(opts)
close(cmdDone)
}()

Expand Down
4 changes: 2 additions & 2 deletions pkg/kube/pod_command_executor_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
// PodCommandExecutorProcessor is an interface wrapping kubernetes API invocation
// it is purposed to be replaced by fake implementation in tests
type PodCommandExecutorProcessor interface {
ExecWithOptions(opts ExecOptions) (string, string, error)
ExecWithOptions(opts ExecOptions) error
}

type podCommandExecutorProcessor struct {
Expand All @@ -30,6 +30,6 @@ type podCommandExecutorProcessor struct {

// ExecWithOptions executes a command in the specified pod and container,
// returning stdout, stderr and error.
func (p *podCommandExecutorProcessor) ExecWithOptions(opts ExecOptions) (string, string, error) {
func (p *podCommandExecutorProcessor) ExecWithOptions(opts ExecOptions) error {
return ExecWithOptions(p.cli, opts)
}
6 changes: 2 additions & 4 deletions pkg/kube/pod_command_executor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ type fakePodCommandExecutorProcessor struct {
inExecWithOptionsOpts *ExecOptions
execWithOptionsStdout string
execWithOptionsStderr string
execWithOptionsRet1 string
execWithOptionsRet2 string
execWithOptionsErr error

// Signal to `ExecWithOptions` to start "executing" command.
Expand All @@ -79,7 +77,7 @@ type fakePodCommandExecutorProcessor struct {
execWithOptionsSyncEnd testBarrier
}

func (fprp *fakePodCommandExecutorProcessor) ExecWithOptions(opts ExecOptions) (string, string, error) {
func (fprp *fakePodCommandExecutorProcessor) ExecWithOptions(opts ExecOptions) error {
fprp.inExecWithOptionsOpts = &opts
fprp.execWithOptionsSyncStart.SyncWithController()
if opts.Stdout != nil && len(fprp.execWithOptionsStdout) > 0 {
Expand All @@ -90,7 +88,7 @@ func (fprp *fakePodCommandExecutorProcessor) ExecWithOptions(opts ExecOptions) (
}
fprp.execWithOptionsSyncEnd.SyncWithController()

return fprp.execWithOptionsRet1, fprp.execWithOptionsRet2, fprp.execWithOptionsErr
return fprp.execWithOptionsErr
}

func (s *PodCommandExecutorTestSuite) TestPodRunnerExec(c *C) {
Expand Down