PureStorage-OpenConnect / pure-fa-openmetrics-exporter

Pure Storage OpenMetrics exporter for FlashArray
Apache License 2.0
20 stars 27 forks source link

[fatal bug] Fix if statements on error handling when encountering 401 on scrape. #127

Closed chrroberts-pure closed 6 months ago

chrroberts-pure commented 6 months ago

See stacktrace

2023/11/01 14:17:20 Start Pure FlashArray exporter v1.0.9 on 0.0.0.0:9490
fatal error: concurrent map writes

goroutine 317 [running]:
net/textproto.MIMEHeader.Set(...)
    /usr/local/go/src/net/textproto/header.go:22
net/http.Header.Set(...)
    /usr/local/go/src/net/http/header.go:40
github.com/go-resty/resty/v2.(*Client).SetHeader(...)
    /Users/jlaing/go/pkg/mod/github.com/go-resty/resty/v2@v2.7.0/client.go:195
purestorage/fa-openmetrics-exporter/internal/rest-client.(*FAClient).RefreshSession(0x14000397440)
    /Users/jlaing/git/pure/pure-fa-openmetrics-exporter/internal/rest-client/flasharray_client.go:119 +0x3f0
purestorage/fa-openmetrics-exporter/internal/rest-client.(*FAClient).GetHardware(0x14000397440)
    /Users/jlaing/git/pure/pure-fa-openmetrics-exporter/internal/rest-client/hardware.go:36 +0x2d0
purestorage/fa-openmetrics-exporter/internal/openmetrics-exporter.(*HardwareCollector).Collect(0x1400006a8c0, 0x140004b2f58?)
    /Users/jlaing/git/pure/pure-fa-openmetrics-exporter/internal/openmetrics-exporter/hardware_collector.go:21 +0x34
github.com/prometheus/client_golang/prometheus.(*Registry).Gather.func1()
    /Users/jlaing/go/pkg/mod/github.com/prometheus/client_golang@v1.16.0/prometheus/registry.go:457 +0xc0
created by github.com/prometheus/client_golang/prometheus.(*Registry).Gather
    /Users/jlaing/go/pkg/mod/github.com/prometheus/client_golang@v1.16.0/prometheus/registry.go:547 +0x9f4

This is the interesting line

    /Users/jlaing/go/pkg/mod/github.com/go-resty/resty/v2@v2.7.0/client.go:195
purestorage/fa-openmetrics-exporter/internal/rest-client.(*FAClient).RefreshSession(0x14000397440)

currently on all the internal/rest-client/[uri].go files, there is some error handling on if a 401 error is encountered.

it currently stands like this

    if res.StatusCode() == 401 {
        fa.RefreshSession()
    }
    res, err = fa.RestClient.R().
        SetResult(&result).
        Get(uri)
    if err != nil {
        fa.Error = err
        }

This could definitely cause the concurrent map write since the rest of the code that is supposed to be in the if is running out of the if.

The closing bracket needs to be moved to the end of the error handling statement ( see #117 ) to correctly handle the error like such, also the var res needs to be _ because its never used after that statement.

    if res.StatusCode() == 401 {
        fa.RefreshSession()
        _, err = fa.RestClient.R().
            SetResult(&result).
            Get(uri)
        if err != nil {
            fa.Error = err
        }
        }
chrroberts-pure commented 6 months ago

I am currently testing this in my lab - will run for a day or two to ensure this bug isn't hit.

chrroberts-pure commented 6 months ago

Good news, the error has changed, but it still crashed,

2024/03/18 22:24:54 Start Pure FlashArray exporter v1.0.16 on 0.0.0.0:9490
fatal error: concurrent map iteration and map write

goroutine 21416 [running]:
github.com/go-resty/resty/v2.parseRequestHeader(0xc006d4a008?, 0xc001274700)
    /go/pkg/mod/github.com/go-resty/resty/v2@v2.11.0/middleware.go:163 +0x79
github.com/go-resty/resty/v2.(*Client).execute(0xc006d4a008, 0xc001274700)
    /go/pkg/mod/github.com/go-resty/resty/v2@v2.11.0/client.go:1164 +0x3c2
github.com/go-resty/resty/v2.(*Request).Execute(0xc001274700, {0x8e2f87?, 0x0?}, {0x8e6418, 0xc})
    /go/pkg/mod/github.com/go-resty/resty/v2@v2.11.0/request.go:936 +0x75e
github.com/go-resty/resty/v2.(*Request).Get(...)
    /go/pkg/mod/github.com/go-resty/resty/v2@v2.11.0/request.go:852
purestorage/fa-openmetrics-exporter/internal/rest-client.(*FAClient).GetDirectories(0xc00348ea20)
    /usr/src/app/internal/rest-client/directories.go:60 +0x55d
purestorage/fa-openmetrics-exporter/internal/openmetrics-exporter.(*DirectoriesSpaceCollector).Collect(0xc007326cf0, 0xc0001045a0)
    /usr/src/app/internal/openmetrics-exporter/dirs_space_collector.go:20 +0x39
github.com/prometheus/client_golang/prometheus.(*Registry).Gather.func1()
    /go/pkg/mod/github.com/prometheus/client_golang@v1.19.0/prometheus/registry.go:457 +0xe5
created by github.com/prometheus/client_golang/prometheus.(*Registry).Gather in goroutine 21409
    /go/pkg/mod/github.com/prometheus/client_golang@v1.19.0/prometheus/registry.go:547 +0xbab

I changed to removed the error handling after 401, because if it wasn't 401, the error should of been handled already

chrroberts-pure commented 6 months ago

keeping in draft until fully tested

chrroberts-pure commented 6 months ago

closes #85 #117