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

make policy change handler try all fleet hosts before failing #1329

Merged
merged 14 commits into from
Oct 20, 2022
Merged

make policy change handler try all fleet hosts before failing #1329

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d852a92
make policy change handler try all fleet hosts before failing
AndersonQ Sep 27, 2022
7d26053
refactor remote client
AndersonQ Oct 6, 2022
5885796
adjust tests
AndersonQ Oct 11, 2022
afd1c8e
adjust docs and tests
AndersonQ Oct 11, 2022
13de63e
remove quick and dirty fix
AndersonQ Oct 11, 2022
e7fb0d0
fix typo
AndersonQ Oct 11, 2022
5927b70
fix liting issues
AndersonQ Oct 11, 2022
e3521ef
pr review changes
AndersonQ Oct 20, 2022
9caf99b
add changelog fragment
AndersonQ Oct 20, 2022
bc0e603
wee lint fixes
AndersonQ Oct 20, 2022
3d47fe3
use hashicorp/go-multierror
AndersonQ Oct 20, 2022
0aa99e0
don't double add the error
AndersonQ Oct 20, 2022
e9ee631
make the litner happy
AndersonQ Oct 20, 2022
f99490c
make the linter happy: the return
AndersonQ Oct 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions internal/pkg/remote/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ const (
retryOnBadConnTimeout = 5 * time.Minute
)

var errRequestFailed = errors.New("request failed")

type wrapperFunc func(rt http.RoundTripper) (http.RoundTripper, error)

type requestClient struct {
Expand All @@ -45,10 +47,10 @@ type requestClient struct {
// to the client. For authenticated calls or sending fields on every request, create a custom RoundTripper
// implementation that will take care of the boilerplate.
type Client struct {
log *logger.Logger
clientMu sync.Mutex
clients []*requestClient
config Config
log *logger.Logger
clientLock sync.Mutex
clients []*requestClient
config Config
}

// NewConfigFromURL returns a Config based on a received host.
Expand Down Expand Up @@ -102,7 +104,7 @@ func NewWithConfig(log *logger.Logger, cfg Config, wrapper wrapperFunc) (*Client
hosts := cfg.GetHosts()
hostCount := len(hosts)
log.With("hosts", hosts).Debugf(
"creating remote client with %d hosts", hostCount, hosts)
"creating remote client with %d hosts", hostCount)
clients := make([]*requestClient, hostCount)
for i, host := range hosts {
baseURL, err := urlutil.MakeURL(string(cfg.Protocol), p, host, 0)
Expand Down Expand Up @@ -161,16 +163,15 @@ func (c *Client) Send(
}

c.log.Debugf("Request method: %s, path: %s, reqID: %s", method, path, reqID)
c.clientMu.Lock()
defer c.clientMu.Unlock()
c.clientLock.Lock()
defer c.clientLock.Unlock()

var err error
var req *http.Request
var resp *http.Response
multiErr := errRequestFailed

c.sortClients()
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we should shuffle arrays with each request, it adds unnecessary CPU cycles. maybe it would be better to shuffle on error

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorting on each request will ensure we keep the previous behavior, preferring the no-error last used host to use next. Besides the number of fleet-server hosts are so small, that it's negligible the overhead to sort them.

Copy link
Member

Choose a reason for hiding this comment

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

Could possibly optimize skipping sorting if already known to be sorted before.

Copy link
Member Author

Choose a reason for hiding this comment

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

it needs to be sorted every time because the sorting conditions change. Every time a host is used it loses priority, thus a new sorting is required.

It isn't much different from the original code that would do extensive search on each call to Send. Besides the host list is so small that i's negligible the impact on performance.

for i, requester := range c.clients {
req, err = requester.newRequest(method, path, params, body)
req, err := requester.newRequest(method, path, params, body)
if err != nil {
return nil, errors.Wrapf(err, "fail to create HTTP request using method %s to %s", method, path)
}
Expand Down Expand Up @@ -201,9 +202,12 @@ func (c *Client) Send(
requester.lastErr = err
requester.lastErrOcc = time.Now().UTC()

// Using debug level as the error is only relevant if all clients fail.
c.log.With("error", err).Debugf("requester %d/%d to host %s errored",
msg := fmt.Sprintf("requester %d/%d to host %s errored",
i, len(c.clients), requester.host)
multiErr = fmt.Errorf("%s: %s: %s", msg, err, multiErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels hacky, i'd prefer https://github.com/hashicorp/go-multierror


// Using debug level as the error is only relevant if all clients fail.
c.log.With("error", err).Debugf(msg)
continue
}

Expand All @@ -212,7 +216,7 @@ func (c *Client) Send(
return resp, nil
}

return nil, fmt.Errorf("all hosts failed, last error: %w", err)
return nil, fmt.Errorf("all hosts failed: %w", multiErr)
}

// URI returns the remote URI.
Expand Down