diff --git a/pkg/drbd/dumpmd.go b/pkg/drbd/dumpmd.go index 3fecc179..b2be8615 100644 --- a/pkg/drbd/dumpmd.go +++ b/pkg/drbd/dumpmd.go @@ -185,11 +185,11 @@ func appendUUIDStatements(out []string, text string) []string { return out } -// uuidStatement answers the UUID a statement is, when it is a bare non-zero -// hex UUID. +// uuidStatement answers the UUID a statement is, when it is a bare generation +// UUID (IsGenerationUUID). func uuidStatement(statement string) (string, bool) { digits, ok := strings.CutPrefix(strings.TrimSpace(statement), "0x") - if !ok || !isHex(digits) || strings.Trim(digits, "0") == "" { + if !ok || !IsGenerationUUID(digits) { return "", false } @@ -208,6 +208,18 @@ func firstStatement(text string) (string, bool) { return "", false } +// generationUUIDDigits is how many hex digits drbdsetup and drbdmeta print a +// generation UUID with: a 64-bit value as %016llX, zero-padded. +const generationUUIDDigits = 16 + +// IsGenerationUUID says whether s is a generation UUID as drbdsetup and +// drbdmeta print it without the 0x prefix: 16 hex digits, not all zero. +// Zero is DRBD's "no generation", which identifies nothing, and a value of +// another length is not one the tools print, so it is damage, not a GI. +func IsGenerationUUID(s string) bool { + return len(s) == generationUUIDDigits && isHex(s) && strings.Trim(s, "0") != "" +} + func isHex(digits string) bool { if digits == "" { return false diff --git a/pkg/drbd/dumpmd_test.go b/pkg/drbd/dumpmd_test.go index f76fdc77..b677d76a 100644 --- a/pkg/drbd/dumpmd_test.go +++ b/pkg/drbd/dumpmd_test.go @@ -144,6 +144,9 @@ func TestParseMetadataUUIDsRefusesWhatItCannotRead(t *testing.T) { "empty": "", "current without 0x": "version \"v09\";\ncurrent-uuid 6E3A5D0B2C1F4A96;\n", "zero current": "version \"v09\";\ncurrent-uuid 0x0000000000000000;\n", + "short current": "version \"v09\";\ncurrent-uuid 0xABCD;\n", + "long current": "version \"v09\";\ncurrent-uuid 0x16E3A5D0B2C1F4A96;\n", + "non-hex current": "version \"v09\";\ncurrent-uuid 0x6E3A5D0B2C1F4A9G;\n", "unreadable current next to a bitmap UUID": "version \"v09\";\ncurrent-uuid 6E3A5D0B2C1F4A96;\n" + "peer[1] {\n bitmap-uuid 0x1234567890ABCDEF;\n}\n", "v08 zero current next to a bitmap UUID": "version \"v08\";\nuuid {\n" + diff --git a/pkg/satellite/createmd_test.go b/pkg/satellite/createmd_test.go index 4578449c..40af624e 100644 --- a/pkg/satellite/createmd_test.go +++ b/pkg/satellite/createmd_test.go @@ -994,6 +994,43 @@ func mustRecordedGIs(t *testing.T, rec *Reconciler, rdName string, vol int32) [] return got } +// Only a GI is recorded: a zero one is DRBD's "none yet", and a value DRBD +// would not print, not hex or not 16 digits, would make the whole record read +// as damaged later. +func TestOnlyGenerationUUIDsAreRecorded(t *testing.T) { + rec, _, _, _, _ := uncleanFixture(t) //nolint:dogsled // only the reconciler is needed + + for _, generation := range []string{ + "0000000000000000", "not-a-gi", "0x" + uncleanReportedGI, "6E3A 5D0B", + "ABCD", "1" + uncleanReportedGI, uncleanReportedGI[1:], "6E3A5D0B2C1F4A9G", + } { + rec.RecordObservedGI("pvc-unclean", 0, generation) + } + + if got := mustRecordedGIs(t, rec, "pvc-unclean", 0); len(got) != 0 { + t.Fatalf("recorded %v, want nothing", got) + } + + rec.RecordObservedGI("pvc-unclean", 0, strings.ToLower(uncleanReportedGI)) + + if got := mustRecordedGIs(t, rec, "pvc-unclean", 0); !slices.Equal(got, []string{uncleanReportedGI}) { + t.Fatalf("recorded %v, want [%s]", got, uncleanReportedGI) + } +} + +// DRBD zero-pads a GI to 16 digits, so a leading zero is part of one. +func TestAZeroPaddedGenerationIsRecorded(t *testing.T) { + rec, _, _, _, _ := uncleanFixture(t) //nolint:dogsled // only the reconciler is needed + + const padded = "006E3A5D0B2C1F4A" + + rec.RecordObservedGI("pvc-unclean", 0, padded) + + if got := mustRecordedGIs(t, rec, "pvc-unclean", 0); !slices.Equal(got, []string{padded}) { + t.Fatalf("recorded %v, want [%s]", got, padded) + } +} + // A record that cannot be read is not "no record": the repair fails on it // with no create-md, on both paths, and an observation does not write over // it with its one GI. A damaged line counts as unreadable: skipping it @@ -1004,6 +1041,11 @@ func TestUnreadableGIRecordFailsClosed(t *testing.T) { "directory": "", "line with no GI": "0\n", "volume not a number": "zero " + uncleanReportedGI + "\n", + "GI not hex": "0 not-a-gi\n", + "zero GI": "0 0000000000000000\n", + "GI too short": "0 ABCD\n", + "GI too long": "0 1" + uncleanReportedGI + "\n", + "GI of 16 not hex": "0 6E3A5D0B2C1F4A9G\n", } { t.Run(path+"/"+damage, func(t *testing.T) { rec, dr, devices, mdMarkerPath, fx := uncleanFixture(t) diff --git a/pkg/satellite/gi_history.go b/pkg/satellite/gi_history.go index 2200a9a2..86ad054d 100644 --- a/pkg/satellite/gi_history.go +++ b/pkg/satellite/gi_history.go @@ -30,6 +30,7 @@ import ( "strconv" "strings" + "github.com/cozystack/blockstor/pkg/drbd" intent "github.com/cozystack/blockstor/pkg/satellite/intent" ) @@ -54,6 +55,19 @@ func (r *Reconciler) RecordObservedGI(rdName string, vol int32, generation strin return } + // A zero GI is DRBD's "none yet" and identifies nothing. Anything else + // that is not a GI as DRBD prints it is not one at all, and a record of + // it would later read as damaged and stop the repair for the whole + // resource. + if !drbd.IsGenerationUUID(generation) { + if strings.Trim(generation, "0") != "" { + slog.Default().Warn("not recording a value that is not a generation UUID", "resource", rdName, + "volume", vol, "generation", generation) + } + + return + } + generation = strings.ToUpper(generation) key := rdName + "/" + strconv.Itoa(int(vol)) @@ -189,7 +203,7 @@ func (r *Reconciler) readGIHistory(rdName string) (map[int32][]string, error) { volField, generation, ok := strings.Cut(line, " ") vol, convErr := strconv.ParseInt(volField, 10, 32) - if !ok || generation == "" || convErr != nil { + if !ok || !drbd.IsGenerationUUID(generation) || convErr != nil { return nil, fmt.Errorf("the generation record of %s: %w: %q", rdName, errDamagedGIRecord, line) }