Skip to content

fix(provisioner/terraform/cleanup): use mtime instead of atime #10892

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

Merged
merged 2 commits into from
Nov 27, 2023
Merged

fix(provisioner/terraform/cleanup): use mtime instead of atime #10892

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
0a16268
fix(provisioner): cleanup: use mtime instead of atime
johnstcn Nov 27, 2023
97b9271
remove unnecessary atime argument given we just use atime=now all the…
johnstcn Nov 27, 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
28 changes: 12 additions & 16 deletions provisioner/terraform/cleanup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"strings"
"time"

"github.com/djherbis/times"
Copy link
Member

Choose a reason for hiding this comment

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

nit: tidy go.mod or not yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, still referenced in provisionersdk Sessions cleanup. I'll do a separate PR for this.

"github.com/spf13/afero"
"golang.org/x/xerrors"

Expand Down Expand Up @@ -76,16 +75,16 @@ func CleanStaleTerraformPlugins(ctx context.Context, cachePath string, fs afero.
// Identify stale plugins
var stalePlugins []string
for _, pluginPath := range pluginPaths {
accessTime, err := latestAccessTime(fs, pluginPath)
modTime, err := latestModTime(fs, pluginPath)
if err != nil {
return xerrors.Errorf("unable to evaluate latest access time for directory %q: %w", pluginPath, err)
return xerrors.Errorf("unable to evaluate latest mtime for directory %q: %w", pluginPath, err)
}

if accessTime.Add(staleTerraformPluginRetention).Before(now) {
logger.Info(ctx, "plugin directory is stale and will be removed", slog.F("plugin_path", pluginPath))
if modTime.Add(staleTerraformPluginRetention).Before(now) {
logger.Info(ctx, "plugin directory is stale and will be removed", slog.F("plugin_path", pluginPath), slog.F("mtime", modTime))
stalePlugins = append(stalePlugins, pluginPath)
} else {
logger.Debug(ctx, "plugin directory is not stale", slog.F("plugin_path", pluginPath))
logger.Debug(ctx, "plugin directory is not stale", slog.F("plugin_path", pluginPath), slog.F("mtime", modTime))
}
}

Expand Down Expand Up @@ -127,22 +126,19 @@ func CleanStaleTerraformPlugins(ctx context.Context, cachePath string, fs afero.
return nil
}

// latestAccessTime walks recursively through the directory content, and locates
// the last accessed file.
func latestAccessTime(fs afero.Fs, pluginPath string) (time.Time, error) {
// latestModTime walks recursively through the directory content, and locates
// the last created/modified file.
func latestModTime(fs afero.Fs, pluginPath string) (time.Time, error) {
var latest time.Time
err := afero.Walk(fs, pluginPath, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}

accessTime := info.ModTime() // fallback to modTime if accessTime is not available (afero)
if info.Sys() != nil {
timeSpec := times.Get(info)
accessTime = timeSpec.AccessTime()
}
if latest.Before(accessTime) {
latest = accessTime
// atime is not reliable, so always use mtime.
modTime := info.ModTime()
if modTime.After(latest) {
latest = modTime
}
return nil
})
Expand Down
41 changes: 26 additions & 15 deletions provisioner/terraform/cleanup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,22 @@ const cachePath = "/tmp/coder/provisioner-0/tf"
var updateGoldenFiles = flag.Bool("update", false, "Update golden files")

var (
now = time.Date(2023, 6, 3, 4, 5, 6, 0, time.UTC)
coderPluginPath = filepath.Join("registry.terraform.io", "coder", "coder", "0.11.1", "darwin_arm64")
dockerPluginPath = filepath.Join("registry.terraform.io", "kreuzwerker", "docker", "2.25.0", "darwin_arm64")
)

func TestPluginCache_Golden(t *testing.T) {
t.Parallel()

prepare := func() (afero.Fs, time.Time, slog.Logger) {
fs := afero.NewMemMapFs()
now := time.Date(2023, time.June, 3, 4, 5, 6, 0, time.UTC)
prepare := func() (afero.Fs, slog.Logger) {
// afero.MemMapFs does not modify atimes, so use a real FS instead.
Copy link
Member

Choose a reason for hiding this comment

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

:(

tmpDir := t.TempDir()
fs := afero.NewBasePathFs(afero.NewOsFs(), tmpDir)
logger := slogtest.Make(t, nil).
Leveled(slog.LevelDebug).
Named("cleanup-test")
return fs, now, logger
return fs, logger
}

t.Run("all plugins are stale", func(t *testing.T) {
Expand All @@ -51,7 +53,7 @@ func TestPluginCache_Golden(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
defer cancel()

fs, now, logger := prepare()
fs, logger := prepare()

// given
// This plugin is older than 30 days.
Expand Down Expand Up @@ -79,7 +81,7 @@ func TestPluginCache_Golden(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
defer cancel()

fs, now, logger := prepare()
fs, logger := prepare()

// given
addPluginFile(t, fs, coderPluginPath, "terraform-provider-coder_v0.11.1", now.Add(-2*time.Hour))
Expand All @@ -106,17 +108,17 @@ func TestPluginCache_Golden(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
defer cancel()

fs, now, logger := prepare()
fs, logger := prepare()

// given
addPluginFile(t, fs, coderPluginPath, "terraform-provider-coder_v0.11.1", now.Add(-63*24*time.Hour))
addPluginFile(t, fs, coderPluginPath, "LICENSE", now.Add(-33*24*time.Hour))
addPluginFile(t, fs, coderPluginPath, "README.md", now.Add(-31*24*time.Hour))
addPluginFolder(t, fs, coderPluginPath, "new_folder", now.Add(-4*time.Hour)) // touched
addPluginFile(t, fs, coderPluginPath, filepath.Join("new_folder", "foobar.tf"), now.Add(-43*24*time.Hour))
addPluginFolder(t, fs, coderPluginPath, "new_folder", now.Add(-43*24*time.Hour))
addPluginFile(t, fs, coderPluginPath, filepath.Join("new_folder", "foobar.tf"), now.Add(-4*time.Hour)) // touched

addPluginFile(t, fs, dockerPluginPath, "terraform-provider-docker_v2.25.0", now.Add(-31*24*time.Hour))
addPluginFile(t, fs, dockerPluginPath, "LICENSE", now.Add(-2*time.Hour))
addPluginFile(t, fs, dockerPluginPath, "LICENSE", now.Add(-2*time.Hour)) // also touched
addPluginFile(t, fs, dockerPluginPath, "README.md", now.Add(-33*24*time.Hour))

// when
Expand All @@ -127,25 +129,34 @@ func TestPluginCache_Golden(t *testing.T) {
})
}

func addPluginFile(t *testing.T, fs afero.Fs, pluginPath string, resourcePath string, accessTime time.Time) {
func addPluginFile(t *testing.T, fs afero.Fs, pluginPath string, resourcePath string, mtime time.Time) {
err := fs.MkdirAll(filepath.Join(cachePath, pluginPath), 0o755)
require.NoError(t, err, "can't create test folder for plugin file")

err = fs.Chtimes(filepath.Join(cachePath, pluginPath), accessTime, accessTime)
err = fs.Chtimes(filepath.Join(cachePath, pluginPath), now, mtime)
require.NoError(t, err, "can't set times")

err = afero.WriteFile(fs, filepath.Join(cachePath, pluginPath, resourcePath), []byte("foo"), 0o644)
require.NoError(t, err, "can't create test file")

err = fs.Chtimes(filepath.Join(cachePath, pluginPath, resourcePath), accessTime, accessTime)
err = fs.Chtimes(filepath.Join(cachePath, pluginPath, resourcePath), now, mtime)
require.NoError(t, err, "can't set times")

// as creating a file will update mtime of parent, we also want to
// set the mtime of parent to match that of the new child.
parent, _ := filepath.Split(filepath.Join(cachePath, pluginPath, resourcePath))
parentInfo, err := fs.Stat(parent)
require.NoError(t, err, "can't stat parent")
if parentInfo.ModTime().After(mtime) {
require.NoError(t, fs.Chtimes(parent, now, mtime), "can't set mtime of parent to match child")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

review: bit hacky but we want this to look as close to reality as possible

Copy link
Member

Choose a reason for hiding this comment

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

just double-check: this should work consistency for different OSes? no risk of flakiness?

Copy link
Member Author

Choose a reason for hiding this comment

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

We use a different tempdir for each test and perform the file operations in serial, so it should probably be OK.

}

func addPluginFolder(t *testing.T, fs afero.Fs, pluginPath string, folderPath string, accessTime time.Time) {
func addPluginFolder(t *testing.T, fs afero.Fs, pluginPath string, folderPath string, mtime time.Time) {
err := fs.MkdirAll(filepath.Join(cachePath, pluginPath, folderPath), 0o755)
require.NoError(t, err, "can't create plugin folder")

err = fs.Chtimes(filepath.Join(cachePath, pluginPath, folderPath), accessTime, accessTime)
err = fs.Chtimes(filepath.Join(cachePath, pluginPath, folderPath), now, mtime)
require.NoError(t, err, "can't set times")
}

Expand Down