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

error handling when attaching FlexVolume mounts need work #23

Merged
merged 7 commits into from Sep 24, 2019

Conversation

@rafaelvanoni
Copy link
Member

rafaelvanoni commented Aug 21, 2019

No description provided.

@spikecurtis

This comment has been minimized.

Copy link
Member

spikecurtis commented Sep 11, 2019

I don't think this is the fix to problem people are seeing, now that I look closer

The errors they see are

E0727 12:37:49.351357       1 driver-call.go:267] Failed to unmarshal output for command: init, output: "2019/07/27 12:37:49 Unix syslog delivery error\n", error: invalid character '/' after top-level value
W0727 12:37:49.351372       1 driver-call.go:150] FlexVolume: driver call failed: executable: /usr/libexec/kubernetes/kubelet-plugins/volume/exec/nodeagent~uds/uds, args: [init], error: exit status 1, output: "2019/07/27 12:37:49 Unix syslog delivery error\n"
E0727 12:37:49.351384       1 plugins.go:746] Error dynamically probing plugins: Error creating Flexvolume plugin from directory nodeagent~uds, skipping. Error: invalid character '/' after top-level value

In particular, the string Unix syslog delivery error shows up in the Go syslog package not when writing a log, but when opening a connection to the syslog server

The other fishy thing is formatting of the message: "2019/07/27 12:37:49 Unix syslog delivery error\n", error: invalid character '/' after top-level value

in all the places where we log to syslog, we've marshalled the error up into JSON before writing it to standard out, e.g. json.Marshal(&Response{Status: "Failure", Message: msg})

Where do we not do that?

func main() {
    var err error
    logWriter, err = syslog.New(syslog.LOG_WARNING|syslog.LOG_DAEMON, SYSLOGTAG)
    if err != nil {
        log.Fatal(err)
    }
    defer logWriter.Close()

    initConfiguration()

    if err = rootCmd.Execute(); err != nil {
        genericUnsupported("not supported", "", err.Error())
    }
}

if we fail on syslog.New, then we just write the error using go's built in log, rather than JSON encoding

So, I think what's happening is that we're failing to connect to syslog on their system, for some reason.

When that happens, we should either ignore it, and just do our business without logging to syslog, or fail in a way consistent with the FlexVolume spec, and encode the error in the JSON format.

I think I prefer the former, since kubelet or kube-controller-manager can handle logging any errors; we don't need to log to syslog.

Sync with master
flexvol/flexvoldriver.go Outdated Show resolved Hide resolved
@rafaelvanoni rafaelvanoni requested a review from spikecurtis Sep 23, 2019
@@ -275,17 +278,20 @@ func mount(dir, opts string) error {

ninputs, workloadPath, s := checkValidMountOpts(opts)
if !s {
return failure("mount", inp, "Incomplete inputs")
logError("mount", inp, "Incomplete inputs", syslogOnlyTrue)

This comment has been minimized.

Copy link
@spikecurtis

spikecurtis Sep 23, 2019

Member

shouldn't this be sysLogOnlyFalse --- i.e. in the original code it would have logged to stdout

This comment has been minimized.

Copy link
@rafaelvanoni

rafaelvanoni Sep 23, 2019

Author Member

I changed some of the existing behavior to prevent flooding due to periodic or retry-type loops. I think that includes all the code in mount(), unbount() calls. Wdyt?

This comment has been minimized.

Copy link
@spikecurtis

spikecurtis Sep 24, 2019

Member

I don't think so. mount() is the top level command that gets invoked from the command line library, so when it fails we need to return a JSON response with the error.

This comment has been minimized.

Copy link
@spikecurtis

spikecurtis Sep 24, 2019

Member

Same with unmount()

This comment has been minimized.

Copy link
@rafaelvanoni

rafaelvanoni Sep 24, 2019

Author Member

Fixed

}

if err := doMount(dir, ninputs, workloadPath); err != nil {
sErr := "Failure to mount: " + err.Error()
return failure("mount", inp, sErr)
logError("mount", inp, sErr, syslogOnlyTrue)

This comment has been minimized.

Copy link
@spikecurtis

spikecurtis Sep 23, 2019

Member

sysLogOnlyFalse

This comment has been minimized.

Copy link
@rafaelvanoni

rafaelvanoni Sep 24, 2019

Author Member

Fixed

}

if err := addCredentialFile(ninputs); err != nil {
sErr := "Failure to create credentials: " + err.Error()
return failure("mount", inp, sErr)
logError("mount", inp, sErr, syslogOnlyTrue)

This comment has been minimized.

Copy link
@spikecurtis

spikecurtis Sep 23, 2019

Member

sysLogOnlyFalse

This comment has been minimized.

Copy link
@rafaelvanoni

rafaelvanoni Sep 24, 2019

Author Member

Fixed

@@ -300,7 +306,8 @@ func unmount(dir string) error {
comps := strings.Split(dir, "/")
if len(comps) < 6 {
sErr := fmt.Sprintf("Failure to notify nodeagent dir %v", dir)
return failure("unmount", dir, sErr)
logError("unmount", dir, sErr, syslogOnlyTrue)

This comment has been minimized.

Copy link
@spikecurtis

spikecurtis Sep 23, 2019

Member

sysLogOnlyFalse

This comment has been minimized.

Copy link
@rafaelvanoni

rafaelvanoni Sep 24, 2019

Author Member

Fixed

@@ -429,14 +436,14 @@ func initConfiguration() {

bytes, err := ioutil.ReadFile(CONFIG_FILE)
if err != nil {
_ = logWriter.Warning(fmt.Sprintf("Not able to read %s: %s\n", CONFIG_FILE, err.Error()))
logError("initConfiguration", "", fmt.Sprintf("Not able to read %s: %s\n", CONFIG_FILE, err.Error()), syslogOnlyFalse)

This comment has been minimized.

Copy link
@spikecurtis

spikecurtis Sep 23, 2019

Member

syslogOnlyTrue

This comment has been minimized.

Copy link
@rafaelvanoni

rafaelvanoni Sep 24, 2019

Author Member

Fixed

return
}

var config ConfigurationOptions
err = json.Unmarshal(bytes, &config)
if err != nil {
_ = logWriter.Warning(fmt.Sprintf("Not able to parst %s: %s\n", CONFIG_FILE, err.Error()))
logError("initConfiguration", "", fmt.Sprintf("Not able to parst %s: %s\n", CONFIG_FILE, err.Error()), syslogOnlyFalse)

This comment has been minimized.

Copy link
@spikecurtis

spikecurtis Sep 23, 2019

Member

syslogOnlyTrue

This comment has been minimized.

Copy link
@rafaelvanoni

rafaelvanoni Sep 24, 2019

Author Member

Fixed

@rafaelvanoni rafaelvanoni requested a review from spikecurtis Sep 24, 2019
@spikecurtis spikecurtis merged commit 2214d88 into master Sep 24, 2019
2 checks passed
2 checks passed
license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details
@spikecurtis spikecurtis deleted the rafael-pod2daemon-1 branch Sep 24, 2019
@fasaxc fasaxc referenced this pull request Nov 19, 2019
0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.