cni icon indicating copy to clipboard operation
cni copied to clipboard

Run `c := exec.CommandContext(ctx, pluginPath)` inside the for loop

Open AbelHu opened this issue 4 years ago • 1 comments

https://github.com/containernetworking/cni/blob/master/pkg/invoke/raw_exec.go#L34

	stdout := &bytes.Buffer{}
	stderr := &bytes.Buffer{}
	c := exec.CommandContext(ctx, pluginPath)
	c.Env = environ
	c.Stdin = bytes.NewBuffer(stdinData)
	c.Stdout = stdout
	c.Stderr = stderr

	// Retry the command on "text file busy" errors
	for i := 0; i <= 5; i++ {
		err := c.Run()

		// Command succeeded
		if err == nil {
			break
		}

		// If the plugin is currently about to be written, then we wait a
		// second and try it again
		if strings.Contains(err.Error(), "text file busy") {
			time.Sleep(time.Second)
			continue
		}

		// All other errors except than the busy text file
		return nil, e.pluginErr(err, stdout.Bytes(), stderr.Bytes())
	}

	// Copy stderr to caller's buffer in case plugin printed to both
	// stdout and stderr for some reason. Ignore failures as stderr is
	// only informational.
	if e.Stderr != nil && stderr.Len() > 0 {
		_, _ = stderr.WriteTo(e.Stderr)
	}
	return stdout.Bytes(), nil

It seems like that response contains unexpected values if it fails with "text file busy" and then succeeds. Should we update it as below?

	var stdout *bytes.Buffer
	var stderr *bytes.Buffer

	// Retry the command on "text file busy" errors
	for i := 0; i <= 5; i++ {
                stdout = &bytes.Buffer{}
                stderr = &bytes.Buffer{}
                c := exec.CommandContext(ctx, pluginPath)
                c.Env = environ
                c.Stdin = bytes.NewBuffer(stdinData)
                c.Stdout = stdout
                c.Stderr = stderr

		err := c.Run()

		// Command succeeded
		if err == nil {
			break
		}

		// If the plugin is currently about to be written, then we wait a
		// second and try it again
		if strings.Contains(err.Error(), "text file busy") {
			time.Sleep(time.Second)
			continue
		}

		// All other errors except than the busy text file
		return nil, e.pluginErr(err, stdout.Bytes(), stderr.Bytes())
	}

	// Copy stderr to caller's buffer in case plugin printed to both
	// stdout and stderr for some reason. Ignore failures as stderr is
	// only informational.
	if e.Stderr != nil && stderr.Len() > 0 {
		_, _ = stderr.WriteTo(e.Stderr)
	}
	return stdout.Bytes(), nil

AbelHu avatar Aug 11 '21 08:08 AbelHu

could you give an example of the unexpected values if it fails with "text file busy" and then succeeds? What is the unexpected values and could a test be written to demonstrate the unexpected behavior?

jsturtevant avatar Aug 11 '21 17:08 jsturtevant