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

Add a delay before reconnecting to MQTT #161

Merged
merged 2 commits into from
Sep 20, 2023
Merged

Add a delay before reconnecting to MQTT #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f6d226
fix: set values of hidden mqtt-* flags to DefaultConfig
subpop Sep 15, 2023
d15eb02
feat: new flag: mqtt-reconnect-delay
subpop Sep 15, 2023
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
34 changes: 22 additions & 12 deletions cmd/yggd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,18 +50,22 @@ func generateDocumentation(c *cli.Context) error {
// CLI flags and arguments.
func setupDefaultConfig(c *cli.Context) {
config.DefaultConfig = config.Config{
LogLevel: c.String(config.FlagNameLogLevel),
ClientID: c.String(config.FlagNameClientID),
Server: c.StringSlice(config.FlagNameServer),
CertFile: c.String(config.FlagNameCertFile),
KeyFile: c.String(config.FlagNameKeyFile),
CARoot: c.StringSlice(config.FlagNameCaRoot),
PathPrefix: c.String(config.FlagNamePathPrefix),
Protocol: c.String(config.FlagNameProtocol),
DataHost: c.String(config.FlagNameDataHost),
CanonicalFacts: c.String(config.FlagNameCanonicalFacts),
HTTPRetries: c.Int(config.FlagNameHTTPRetries),
HTTPTimeout: c.Duration(config.FlagNameHTTPTimeout),
LogLevel: c.String(config.FlagNameLogLevel),
ClientID: c.String(config.FlagNameClientID),
Server: c.StringSlice(config.FlagNameServer),
CertFile: c.String(config.FlagNameCertFile),
KeyFile: c.String(config.FlagNameKeyFile),
CARoot: c.StringSlice(config.FlagNameCaRoot),
PathPrefix: c.String(config.FlagNamePathPrefix),
Protocol: c.String(config.FlagNameProtocol),
DataHost: c.String(config.FlagNameDataHost),
CanonicalFacts: c.String(config.FlagNameCanonicalFacts),
HTTPRetries: c.Int(config.FlagNameHTTPRetries),
HTTPTimeout: c.Duration(config.FlagNameHTTPTimeout),
MQTTConnectRetry: c.Bool(config.FlagNameMQTTConnectRetry),
MQTTConnectRetryInterval: c.Duration(config.FlagNameMQTTConnectRetryInterval),
MQTTAutoReconnect: c.Bool(config.FlagNameMQTTAutoReconnect),
MQTTReconnectDelay: c.Duration(config.FlagNameMQTTReconnectDelay),
}
}

Expand Down Expand Up @@ -505,6 +509,12 @@ func main() {
Value: true,
Hidden: true,
}),
altsrc.NewDurationFlag(&cli.DurationFlag{
Copy link
Contributor

Choose a reason for hiding this comment

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

It is pretty confusing that you cannot use --mqtt-reconnect-delay with simple numeric value. I was expecting that it should work and value would be considered in seconds.

I think that we should emphasize in Usage: that argument should be something like 10s (not only 10 or "10").

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we should also improve Usage text of other occurrences of DurationFlag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Internally, the DurationFlag uses time.ParseDuration to parse the value. That function does not make assumptions about default duration unit if the unit is omitted. The error returned by an invalid value for the flag does not make a suggestion about how to specify a valid format. That error comes from the cli package though; we can't change it.

These are hidden flags though; their Usage won't be visible to a user in the --help output. I changed the hidden value to true to see what the usage text looks like:

--mqtt-reconnect-delay DURATION  Sets the time to wait before attempting to reconnect to DURATION (default: 0s)

The default value there hints at using a duration unit, so if we do ever make these visible flags, the usage does suggest to the user how to specify a correct value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you are right, when the flag is hidden, then the usage could not visible.

Name: config.FlagNameMQTTReconnectDelay,
Usage: "Sets the time to wait before attempting to reconnect to `DURATION`",
Value: 0 * time.Second,
Hidden: true,
}),
}

app.EnableBashCompletion = true
Expand Down
5 changes: 5 additions & 0 deletions internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const (
FlagNameMQTTConnectRetry = "mqtt-connect-retry"
FlagNameMQTTConnectRetryInterval = "mqtt-connect-retry-interval"
FlagNameMQTTAutoReconnect = "mqtt-auto-reconnect"
FlagNameMQTTReconnectDelay = "mqtt-reconnect-delay"
)

var DefaultConfig = Config{
Expand Down Expand Up @@ -91,6 +92,10 @@ type Config struct {
// MQTTAutoReconnect is the MQTT client option that enables automatic
// reconnection logic when the client unexpectedly disconnects.
MQTTAutoReconnect bool

// MQTTReconnectDelay is the duration the client with wait before attempting
// to reconnect to the MQTT broker.
MQTTReconnectDelay time.Duration
}

// CreateTLSConfig creates a tls.Config object from the current configuration.
Expand Down
11 changes: 11 additions & 0 deletions internal/transport/mqtt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ func NewMQTTTransport(clientID string, brokers []string, tlsConfig *tls.Config)
t.events <- TransporterEventDisconnected
})

opts.SetReconnectingHandler(func(c mqtt.Client, co *mqtt.ClientOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When --mqtt-connect-retry is used, then this handler function is also used. I hope that we could improve messaging in this case. When yggd is not able to reconnect, then some exponential backoff algorithm is used for computing next attempt. It would be nice to see something like, when reconnecting failed: reconnecting failed, next reconnect attempt in X seconds. I don't see where to print such message. I also do not know if it is possible to get information about value of delay computed by exponential backoff algorithm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, the MQTT client package does the backing off itself. We just get a handler that's called before the reconnect attempt happens. I don't think the value is exported by the package at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that the delay is printed, when MQTT_DEBUG is set.

if config.DefaultConfig.MQTTReconnectDelay > 0 {
log.Infof(
"delaying for %v before reconnecting...",
config.DefaultConfig.MQTTReconnectDelay,
)
time.Sleep(config.DefaultConfig.MQTTReconnectDelay)
}
log.Debugf("reconnecting to broker: %v", co.Servers)
})

data, err := json.Marshal(&yggdrasil.ConnectionStatus{
Type: yggdrasil.MessageTypeConnectionStatus,
MessageID: uuid.New().String(),
Expand Down