Skip to content

Commit b9b9ad1

Browse files
authored
snapshotting: ability to specify timestamp location != UTC (#801)
This PR adds a new field optional field `timestamp_location` that allows the user to specify a timezone different than the default UTC for use in the snapshot suffix. I took @Mjasnik 's PR #785 and refactored+extended it as follows: * move all formatting logic into its own package * disallow `dense` and `human` with formats != UTC to protect users from stupidity * document behavior more clearly * regression test for existing users
1 parent 904c151 commit b9b9ad1

11 files changed

Lines changed: 322 additions & 66 deletions

File tree

‎config/config.go‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -218,11 +218,16 @@ type SnapshottingEnum struct {
218218
}
219219

220220
type SnapshottingPeriodic struct {
221-
Type string `yaml:"type"`
222-
Prefix string `yaml:"prefix"`
223-
Interval *PositiveDuration `yaml:"interval"`
224-
Hooks HookList `yaml:"hooks,optional"`
225-
TimestampFormat string `yaml:"timestamp_format,optional,default=dense"`
221+
Type string `yaml:"type"`
222+
Prefix string `yaml:"prefix"`
223+
Interval *PositiveDuration `yaml:"interval"`
224+
Hooks HookList `yaml:"hooks,optional"`
225+
TimestampFormattingSpec `yaml:",inline"`
226+
}
227+
228+
type TimestampFormattingSpec struct {
229+
TimestampFormat string `yaml:"timestamp_format,optional,default=dense"`
230+
TimestampLocation string `yaml:"timestamp_location,optional,default=UTC"`
226231
}
227232

228233
type CronSpec struct {
@@ -251,11 +256,11 @@ func (s *CronSpec) UnmarshalYAML(unmarshal func(v interface{}, not_strict bool)
251256
}
252257

253258
type SnapshottingCron struct {
254-
Type string `yaml:"type"`
255-
Prefix string `yaml:"prefix"`
256-
Cron CronSpec `yaml:"cron"`
257-
Hooks HookList `yaml:"hooks,optional"`
258-
TimestampFormat string `yaml:"timestamp_format,optional,default=dense"`
259+
Type string `yaml:"type"`
260+
Prefix string `yaml:"prefix"`
261+
Cron CronSpec `yaml:"cron"`
262+
Hooks HookList `yaml:"hooks,optional"`
263+
TimestampFormattingSpec `yaml:",inline"`
259264
}
260265

261266
type SnapshottingManual struct {

‎config/config_snapshotting_test.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,14 +163,16 @@ jobs:
163163
assert.Equal(t, "periodic", snp.Type)
164164
assert.Equal(t, 10*time.Minute, snp.Interval.Duration())
165165
assert.Equal(t, "zrepl_", snp.Prefix)
166-
assert.Equal(t, "dense", snp.TimestampFormat) // default was set correctly
166+
assert.Equal(t, snp.TimestampFormat, "dense")
167+
assert.Equal(t, snp.TimestampLocation, "UTC")
167168
})
168169

169170
t.Run("cron", func(t *testing.T) {
170171
c = testValidConfig(t, fillSnapshotting(cron))
171172
snp := c.Jobs[0].Ret.(*PushJob).Snapshotting.Ret.(*SnapshottingCron)
172173
assert.Equal(t, "cron", snp.Type)
173174
assert.Equal(t, "zrepl_", snp.Prefix)
174-
assert.Equal(t, "dense", snp.TimestampFormat) // default was set correctly
175+
assert.Equal(t, snp.TimestampFormat, "dense")
176+
assert.Equal(t, snp.TimestampLocation, "UTC")
175177
})
176178
}

‎daemon/snapper/cron.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"github.com/zrepl/zrepl/config"
1212
"github.com/zrepl/zrepl/daemon/hooks"
13+
"github.com/zrepl/zrepl/daemon/snapper/snapname"
1314
"github.com/zrepl/zrepl/util/suspendresumesafetimer"
1415
"github.com/zrepl/zrepl/zfs"
1516
)
@@ -20,10 +21,15 @@ func cronFromConfig(fsf zfs.DatasetFilter, in config.SnapshottingCron) (*Cron, e
2021
if err != nil {
2122
return nil, errors.Wrap(err, "hook config error")
2223
}
24+
25+
formatter, err := snapname.New(in.Prefix, in.TimestampFormat, in.TimestampLocation)
26+
if err != nil {
27+
return nil, errors.Wrap(err, "build snapshot name formatter")
28+
}
29+
2330
planArgs := planArgs{
24-
prefix: in.Prefix,
25-
timestampFormat: in.TimestampFormat,
26-
hooks: hooksList,
31+
formatter: formatter,
32+
hooks: hooksList,
2733
}
2834
return &Cron{config: in, fsf: fsf, planArgs: planArgs}, nil
2935
}

‎daemon/snapper/impl.go‎

Lines changed: 4 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,19 @@ import (
44
"context"
55
"fmt"
66
"sort"
7-
"strconv"
87
"strings"
98
"time"
109

1110
"github.com/zrepl/zrepl/daemon/hooks"
1211
"github.com/zrepl/zrepl/daemon/logging"
12+
"github.com/zrepl/zrepl/daemon/snapper/snapname"
1313
"github.com/zrepl/zrepl/util/chainlock"
1414
"github.com/zrepl/zrepl/zfs"
1515
)
1616

1717
type planArgs struct {
18-
prefix string
19-
timestampFormat string
20-
hooks *hooks.List
18+
formatter *snapname.Formatter
19+
hooks *hooks.List
2120
}
2221

2322
type plan struct {
@@ -60,21 +59,6 @@ type snapProgress struct {
6059
runResults hooks.PlanReport
6160
}
6261

63-
func (plan *plan) formatNow(format string) string {
64-
now := time.Now().UTC()
65-
switch strings.ToLower(format) {
66-
case "dense":
67-
format = "20060102_150405_000"
68-
case "human":
69-
format = "2006-01-02_15:04:05"
70-
case "iso-8601":
71-
format = "2006-01-02T15:04:05.000Z"
72-
case "unix-seconds":
73-
return strconv.FormatInt(now.Unix(), 10)
74-
}
75-
return now.Format(format)
76-
}
77-
7862
func (plan *plan) execute(ctx context.Context, dryRun bool) (ok bool) {
7963

8064
hookMatchCount := make(map[hooks.Hook]int, len(*plan.args.hooks))
@@ -85,8 +69,7 @@ func (plan *plan) execute(ctx context.Context, dryRun bool) (ok bool) {
8569
anyFsHadErr := false
8670
// TODO channel programs -> allow a little jitter?
8771
for fs, progress := range plan.snaps {
88-
suffix := plan.formatNow(plan.args.timestampFormat)
89-
snapname := fmt.Sprintf("%s%s", plan.args.prefix, suffix)
72+
snapname := plan.args.formatter.Format(time.Now())
9073

9174
ctx := logging.WithInjectedField(ctx, "fs", fs.ToString())
9275
ctx = logging.WithInjectedField(ctx, "snap", snapname)

‎daemon/snapper/periodic.go‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/pkg/errors"
1111

1212
"github.com/zrepl/zrepl/daemon/logging/trace"
13+
"github.com/zrepl/zrepl/daemon/snapper/snapname"
1314

1415
"github.com/zrepl/zrepl/config"
1516
"github.com/zrepl/zrepl/daemon/hooks"
@@ -32,13 +33,17 @@ func periodicFromConfig(g *config.Global, fsf zfs.DatasetFilter, in *config.Snap
3233
return nil, errors.Wrap(err, "hook config error")
3334
}
3435

36+
formatter, err := snapname.New(in.Prefix, in.TimestampFormat, in.TimestampLocation)
37+
if err != nil {
38+
return nil, errors.Wrap(err, "build snapshot name formatter")
39+
}
40+
3541
args := periodicArgs{
3642
interval: in.Interval.Duration(),
3743
fsf: fsf,
3844
planArgs: planArgs{
39-
prefix: in.Prefix,
40-
timestampFormat: in.TimestampFormat,
41-
hooks: hookList,
45+
formatter: formatter,
46+
hooks: hookList,
4247
},
4348
// ctx and log is set in Run()
4449
}
@@ -166,7 +171,7 @@ func periodicStateSyncUp(a periodicArgs, u updater) state {
166171
if err != nil {
167172
return onErr(err, u)
168173
}
169-
syncPoint, err := findSyncPoint(a.ctx, fss, a.planArgs.prefix, a.interval)
174+
syncPoint, err := findSyncPoint(a.ctx, fss, a.planArgs.formatter.Prefix(), a.interval)
170175
if err != nil {
171176
return onErr(err, u)
172177
}
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
package snapname
2+
3+
import (
4+
"fmt"
5+
"time"
6+
7+
"github.com/pkg/errors"
8+
9+
"github.com/zrepl/zrepl/daemon/snapper/snapname/timestamp"
10+
"github.com/zrepl/zrepl/zfs"
11+
)
12+
13+
type Formatter struct {
14+
prefix string
15+
timestamp *timestamp.Formatter
16+
}
17+
18+
func New(prefix, tsFormat, tsLocation string) (*Formatter, error) {
19+
timestamp, err := timestamp.New(tsFormat, tsLocation)
20+
if err != nil {
21+
return nil, errors.Wrap(err, "build timestamp formatter")
22+
}
23+
formatter := &Formatter{
24+
prefix: prefix,
25+
timestamp: timestamp,
26+
}
27+
// Best-effort check to detect whether the result would be an invalid name.
28+
// Test two dates that in most places have will have different time zone offsets due to DST.
29+
check := func(t time.Time) error {
30+
testFormat := formatter.Format(t)
31+
if err := zfs.ComponentNamecheck(testFormat); err != nil {
32+
// testFormat last, can be quite long
33+
return fmt.Errorf("`invalid snapshot name would result from `prefix+$timestamp`: %s: %q", err, testFormat)
34+
}
35+
return nil
36+
}
37+
if err := check(time.Date(2020, 6, 1, 0, 0, 0, 0, time.UTC)); err != nil {
38+
return nil, err
39+
}
40+
if err := check(time.Date(2020, 12, 1, 0, 0, 0, 0, time.UTC)); err != nil {
41+
return nil, err
42+
}
43+
return formatter, nil
44+
}
45+
46+
func (f *Formatter) Format(now time.Time) string {
47+
return f.prefix + f.timestamp.Format(now)
48+
}
49+
50+
func (f *Formatter) Prefix() string {
51+
return f.prefix
52+
}
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package timestamp
2+
3+
import (
4+
"fmt"
5+
"strconv"
6+
"strings"
7+
"time"
8+
9+
"github.com/pkg/errors"
10+
)
11+
12+
type Formatter struct {
13+
format func(time.Time) string
14+
location *time.Location
15+
}
16+
17+
func New(formatString string, locationString string) (*Formatter, error) {
18+
location, err := time.LoadLocation(locationString) // no shadow
19+
if err != nil {
20+
return nil, errors.Wrapf(err, "load location from string %q", locationString)
21+
}
22+
makeFormatFunc := func(formatString string) (func(time.Time) string, error) {
23+
// NB: we use zfs.EntityNamecheck in higher-level code to filter out all invalid characters.
24+
// This check here is specifically so that we know for sure that the `+`=>`_` replacement
25+
// that we do in the returned func replaces exactly the timezone offset `+` and not some other `+`.
26+
if strings.Contains(formatString, "+") {
27+
return nil, fmt.Errorf("character '+' is not allowed in ZFS snapshot names and has special handling")
28+
}
29+
return func(t time.Time) string {
30+
res := t.Format(formatString)
31+
// if the formatString contains a time zone specifier
32+
// and the location would result in a positive offset to UTC
33+
// then the result of t.Format would contain a '+' sign.
34+
if isLocationPositiveOffsetToUTC(location) {
35+
// the only source of `+` can be the positive time zone offset because we disallowed `+` as a character in the format string
36+
res = strings.Replace(res, "+", "_", 1)
37+
}
38+
if strings.Contains(res, "+") {
39+
panic(fmt.Sprintf("format produced a string containing illegal character '+' that wasn't the expected case of positive time zone offset: format=%q location=%q unix=%q result=%q", formatString, location, t.Unix(), res))
40+
}
41+
return res
42+
}, nil
43+
}
44+
var formatFunc func(time.Time) string
45+
mustUseUtcError := func() error {
46+
return fmt.Errorf("format string requires UTC location")
47+
}
48+
switch strings.ToLower(formatString) {
49+
case "dense":
50+
if location != time.UTC {
51+
err = mustUseUtcError()
52+
} else {
53+
formatFunc, err = makeFormatFunc("20060102_150405_000")
54+
}
55+
case "human":
56+
if location != time.UTC {
57+
err = mustUseUtcError()
58+
} else {
59+
formatFunc, err = makeFormatFunc("2006-01-02_15:04:05")
60+
}
61+
case "iso-8601":
62+
formatFunc, err = makeFormatFunc("2006-01-02T15:04:05.000Z0700")
63+
case "unix-seconds":
64+
if location != time.UTC {
65+
// Technically not required because unix time is by definition in UTC
66+
// but let's make that clear to confused users...
67+
err = mustUseUtcError()
68+
} else {
69+
formatFunc = func(t time.Time) string {
70+
return strconv.FormatInt(t.Unix(), 10)
71+
}
72+
}
73+
default:
74+
formatFunc, err = makeFormatFunc(formatString)
75+
}
76+
if err != nil {
77+
return nil, errors.Wrapf(err, "invalid format string %q or location %q", formatString, locationString)
78+
}
79+
return &Formatter{
80+
format: formatFunc,
81+
location: location,
82+
}, nil
83+
}
84+
85+
func isLocationPositiveOffsetToUTC(location *time.Location) bool {
86+
_, offsetSeconds := time.Now().In(location).Zone()
87+
return offsetSeconds > 0
88+
}
89+
90+
func (f *Formatter) Format(t time.Time) string {
91+
t = t.In(f.location)
92+
return f.format(t)
93+
}

0 commit comments

Comments
 (0)