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

Fix errors logged on standbys when we try to write versions to storage #13042

Merged
merged 2 commits into from Nov 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/13042.txt
@@ -0,0 +1,3 @@
```release-note:bug
core: Fix warnings logged on perf standbys re stored versions
```
3 changes: 2 additions & 1 deletion vault/activity_log.go
Expand Up @@ -411,6 +411,8 @@ func (a *ActivityLog) saveCurrentSegmentInternal(ctx context.Context, force bool
// be written to storage, since if we remove this code we will incur
// data loss for one segment's worth of TWEs.
if len(a.currentSegment.tokenCount.CountByNamespaceID) > 0 || force {
// We can get away with simply using the oldest version stored because
// the storing of versions was introduced at the same time as this code.
oldestVersion, oldestUpgradeTime, err := a.core.FindOldestVersionTimestamp()
switch {
case err != nil:
Expand Down Expand Up @@ -1086,7 +1088,6 @@ func (c *Core) setupActivityLog(ctx context.Context, wg *sync.WaitGroup) error {
// stopActivityLog removes the ActivityLog from Core
// and frees any resources.
func (c *Core) stopActivityLog() {

// preSeal may run before startActivityLog got a chance to complete.
if c.activityLog != nil {
// Shut down background worker
Expand Down
17 changes: 7 additions & 10 deletions vault/core.go
Expand Up @@ -1047,17 +1047,17 @@ func NewCore(conf *CoreConfig) (*Core, error) {

// HandleVersionTimeStamps stores the current version at the current time to
// storage, and then loads all versions and upgrade timestamps out from storage.
func (c *Core) HandleVersionTimeStamps(ctx context.Context) error {
func (c *Core) handleVersionTimeStamps(ctx context.Context) error {
ncabatoff marked this conversation as resolved.
Show resolved Hide resolved
currentTime := time.Now()
isUpdated, err := c.StoreVersionTimestamp(ctx, version.Version, currentTime)
isUpdated, err := c.storeVersionTimestamp(ctx, version.Version, currentTime)
if err != nil {
return err
return fmt.Errorf("error storing vault version: %w", err)
}
if isUpdated {
c.logger.Info("Recorded vault version", "vault version", version.Version, "upgrade time", currentTime)
}
// Finally, load the versions into core fields
err = c.HandleLoadVersionTimestamps(ctx)
err = c.loadVersionTimestamps(ctx)
if err != nil {
return err
}
Expand Down Expand Up @@ -2000,6 +2000,9 @@ func (s standardUnsealStrategy) unseal(ctx context.Context, logger log.Logger, c
return err
}
}
if err := c.handleVersionTimeStamps(ctx); err != nil {
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

return err
}
if err := c.setupPluginCatalog(ctx); err != nil {
return err
}
Expand Down Expand Up @@ -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)
Copy link
Contributor

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?

Copy link
Collaborator Author

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.


}

c.logger.Info("post-unseal setup complete")
return nil
Expand Down Expand Up @@ -2689,7 +2687,6 @@ func (c *Core) SetConfig(conf *server.Config) {
}

func (c *Core) GetListenerCustomResponseHeaders(listenerAdd string) *ListenerCustomHeaders {

customHeaders := c.customListenerHeader.Load()
if customHeaders == nil {
return nil
Expand Down
29 changes: 4 additions & 25 deletions vault/core_util_common.go
Expand Up @@ -11,9 +11,9 @@ import (

const vaultVersionPath string = "core/versions/"

// StoreVersionTimestamp will store the version and timestamp pair to storage only if no entry
// storeVersionTimestamp will store the version and timestamp pair to storage only if no entry
// for that version already exists in storage.
func (c *Core) StoreVersionTimestamp(ctx context.Context, version string, currentTime time.Time) (bool, error) {
func (c *Core) storeVersionTimestamp(ctx context.Context, version string, currentTime time.Time) (bool, error) {
timeStamp, err := c.barrier.Get(ctx, vaultVersionPath+version)
if err != nil {
return false, err
Expand All @@ -39,27 +39,6 @@ func (c *Core) StoreVersionTimestamp(ctx context.Context, version string, curren
return true, nil
}

// FindMostRecentVersionTimestamp loads the current vault version and associated
// upgrade time from storage.
func (c *Core) FindMostRecentVersionTimestamp() (string, time.Time, error) {
ncabatoff marked this conversation as resolved.
Show resolved Hide resolved
if c.VersionTimestamps == nil || len(c.VersionTimestamps) == 0 {
return "", time.Time{}, fmt.Errorf("Version timestamps are not initialized")
}
var latestUpgradeTime time.Time
var mostRecentVersion string
for version, upgradeTime := range c.VersionTimestamps {
if upgradeTime.After(latestUpgradeTime) {
mostRecentVersion = version
latestUpgradeTime = upgradeTime
}
}
// This if-case should never be hit
if mostRecentVersion == "" {
return "", latestUpgradeTime, fmt.Errorf("Empty vault version was written to storage at time: %+v", latestUpgradeTime)
}
return mostRecentVersion, latestUpgradeTime, nil
}

// FindOldestVersionTimestamp searches for the vault version with the oldest
// upgrade timestamp from storage. The earliest version this can be (barring
// downgrades) is 1.9.0.
Expand All @@ -80,9 +59,9 @@ func (c *Core) FindOldestVersionTimestamp() (string, time.Time, error) {
return oldestVersion, oldestUpgradeTime, nil
}

// HandleLoadVersionTimestamps loads all the vault versions and associated
// loadVersionTimestamps loads all the vault versions and associated
// upgrade timestamps from storage.
func (c *Core) HandleLoadVersionTimestamps(ctx context.Context) (retErr error) {
func (c *Core) loadVersionTimestamps(ctx context.Context) (retErr error) {
vaultVersions, err := c.barrier.List(ctx, vaultVersionPath)
if err != nil {
return fmt.Errorf("unable to retrieve vault versions from storage: %+w", err)
Expand Down
8 changes: 4 additions & 4 deletions vault/core_util_common_test.go
Expand Up @@ -13,7 +13,7 @@ import (
func TestStoreMultipleVaultVersions(t *testing.T) {
c, _, _ := TestCoreUnsealed(t)
upgradeTimePlusEpsilon := time.Now()
wasStored, err := c.StoreVersionTimestamp(context.Background(), version.Version, upgradeTimePlusEpsilon.Add(30*time.Hour))
wasStored, err := c.storeVersionTimestamp(context.Background(), version.Version, upgradeTimePlusEpsilon.Add(30*time.Hour))
if err != nil || wasStored {
t.Fatalf("vault version was re-stored: %v, err is: %s", wasStored, err.Error())
}
Expand All @@ -32,9 +32,9 @@ func TestGetOldestVersion(t *testing.T) {
c, _, _ := TestCoreUnsealed(t)
upgradeTimePlusEpsilon := time.Now()

c.StoreVersionTimestamp(context.Background(), "1.9.1", upgradeTimePlusEpsilon.Add(-4*time.Hour))
c.StoreVersionTimestamp(context.Background(), "1.9.2", upgradeTimePlusEpsilon.Add(2*time.Hour))
c.HandleLoadVersionTimestamps(c.activeContext)
c.storeVersionTimestamp(context.Background(), "1.9.1", upgradeTimePlusEpsilon.Add(-4*time.Hour))
c.storeVersionTimestamp(context.Background(), "1.9.2", upgradeTimePlusEpsilon.Add(2*time.Hour))
c.loadVersionTimestamps(c.activeContext)
if len(c.VersionTimestamps) != 3 {
t.Fatalf("expected 3 entries in timestamps map after refresh, found: %d", len(c.VersionTimestamps))
}
Expand Down