-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix errors logged on standbys when we try to write versions to storage #13042
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions on the PR, but looks good to me overall!
@@ -2000,6 +2000,9 @@ func (s standardUnsealStrategy) unseal(ctx context.Context, logger log.Logger, c | |||
return err | |||
} | |||
} | |||
if err := c.handleVersionTimeStamps(ctx); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this executes on a perf standby when the standby unseals, will it cause the standby to error on the storage write?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it here because standardUnsealStrategy doesn't apply to perf standbys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to load but not store the values on perf standbys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, good point. Will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess I'll fix in ent once this is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided against loading on perf standbys since it wouldn't be reliable: #13081
@@ -2157,11 +2160,6 @@ func (c *Core) postUnseal(ctx context.Context, ctxCancelFunc context.CancelFunc, | |||
c.logger.Warn("post-unseal post seal migration failed", "error", err) | |||
} | |||
} | |||
err := c.HandleVersionTimeStamps(c.activeContext) | |||
if err != nil { | |||
c.logger.Warn("post-unseal version timestamp setup failed", "error", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the extra log line you were talking about in the PR description, where essentially the standby would always warn about the timestamp loading/storing not succeeding in the logs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't actually test and see the log line myself, I speculated that we'd see this line on perf standbys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
I haven't actually seen error logs, but we know that only the active node should be allowed to write to storage.
While I was at it, I made some methods private that didn't need to be public and removed some dead code.