Skip to content

Commit 9f4a8af

Browse files
fix: ensure numeric sorting of manifest group directories
# Background / Context The lifecycle-agent processes manifests from subdirectories in a base path using the LoadGroupedManifestsFromPath function. This function reads subdirectories using os.ReadDir, which returns directory entries in an alphabetically sorted order by default in Go. The manifest groups are typically named with numeric suffixes (e.g., restore1, restore2, restore10) to indicate their processing order. The current implementation processes these directories in the order returned by os.ReadDir without any additional sorting logic to handle numeric suffixes. # Issue / Requirement / Reason for change When manifest group directories have numeric suffixes, alphabetical sorting causes incorrect ordering. Specifically: - restore1, restore10, restore11, restore12, restore2, restore3, ... This is because alphabetically "restore10" comes before "restore2". This violates the expected numeric ordering and can cause manifests to be applied in the wrong sequence, potentially leading to restore failures or unexpected behavior. Related to: https://issues.redhat.com/browse/OCPBUGS-77043 # Solution / Feature Overview Implemented numeric sorting for manifest group directories by extracting trailing numbers from directory names and sorting based on those numeric values. This ensures that directories are processed in the correct numeric order regardless of whether they have single-digit or multi-digit suffixes. # Implementation Details 1. Added a new extractTrailingNumber helper function that: - Uses a regex pattern (\d+$) to extract trailing numbers from strings - Returns the numeric value, or 0 if no trailing number exists - Handles conversion errors gracefully 2. Modified LoadGroupedManifestsFromPath to sort directories after reading: - Added sort.Slice call that compares directories using extractTrailingNumber - This happens before the loop that processes each directory - Maintains backward compatibility for directories without numeric suffixes 3. Added comprehensive test coverage: - TestLoadGroupedManifestsFromPathWithNumericSorting: Creates 12 directories (restore1 through restore12) and verifies they're processed in numeric order - TestExtractTrailingNumber: Unit tests for the helper function covering single/double/triple digits, no numbers, and edge cases # Other Information This is a bug fix that ensures correct ordering without changing the external API or behavior for properly named directories. The fix is defensive - if a directory has no trailing number, it defaults to 0, maintaining deterministic behavior. No breaking changes are introduced. Existing manifest groups will continue to work, and this fix only affects the order in which directories are processed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 0c10d92 commit 9f4a8af

File tree

2 files changed

+121
-0
lines changed

2 files changed

+121
-0
lines changed

utils/utils.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212
"path/filepath"
1313
"reflect"
1414
"regexp"
15+
"sort"
16+
"strconv"
1517
"text/template"
1618

1719
"github.com/samber/lo"
@@ -465,6 +467,12 @@ func LoadGroupedManifestsFromPath(basePath string, log *logr.Logger) ([][]*unstr
465467
return nil, fmt.Errorf("failed to read manifest groups subdirs in %s: %w", basePath, err)
466468
}
467469

470+
// Sort directories numerically by extracting trailing numbers from names
471+
// This fixes the issue where restore10, restore11 come before restore2 when sorted alphabetically
472+
sort.Slice(groupSubDirs, func(i, j int) bool {
473+
return extractTrailingNumber(groupSubDirs[i].Name()) < extractTrailingNumber(groupSubDirs[j].Name())
474+
})
475+
468476
for _, groupSubDir := range groupSubDirs {
469477
if !groupSubDir.IsDir() {
470478
log.Info("Unexpected file found, skipping...", "file",
@@ -502,6 +510,22 @@ func LoadGroupedManifestsFromPath(basePath string, log *logr.Logger) ([][]*unstr
502510
return sortedManifests, nil
503511
}
504512

513+
// extractTrailingNumber extracts the numeric suffix from a string.
514+
// For example: "restore1" -> 1, "restore10" -> 10, "group2" -> 2.
515+
// Returns 0 if no trailing number is found.
516+
func extractTrailingNumber(s string) int {
517+
re := regexp.MustCompile(`\d+$`)
518+
match := re.FindString(s)
519+
if match == "" {
520+
return 0
521+
}
522+
num, err := strconv.Atoi(match)
523+
if err != nil {
524+
return 0
525+
}
526+
return num
527+
}
528+
505529
// BuildKernelArguementsFromMCOFile reads the kernel arguments from MCO file
506530
// and builds the string arguments that ostree admin deploy requires
507531
func BuildKernelArgumentsFromMCOFile(path string) ([]string, error) {

utils/utils_test.go

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package utils
33
import (
44
"context"
55
"encoding/json"
6+
"fmt"
67
"log"
78
"os"
89
"path/filepath"
@@ -186,6 +187,102 @@ func TestLoadGroupedManifestsFromPath(t *testing.T) {
186187

187188
}
188189

190+
func TestLoadGroupedManifestsFromPathWithNumericSorting(t *testing.T) {
191+
// Create temporary directory
192+
tmpDir, err := os.MkdirTemp("", "restore-numeric-test")
193+
if err != nil {
194+
t.Fatalf("Failed to create temporary directory: %v", err)
195+
}
196+
defer os.RemoveAll(tmpDir)
197+
198+
// Create restores directory
199+
restoreDir := filepath.Join(tmpDir, "manifests")
200+
if err := os.MkdirAll(restoreDir, 0755); err != nil {
201+
t.Fatalf("Failed to create restore directory: %v", err)
202+
}
203+
204+
// Create 12 subdirectories to test numeric sorting (restore1 through restore12)
205+
// This tests the bug where restore10, restore11, restore12 would come before restore2 with alphabetical sorting
206+
for i := 1; i <= 12; i++ {
207+
restoreSubDir := filepath.Join(restoreDir, fmt.Sprintf("restore%d", i))
208+
if err := os.Mkdir(restoreSubDir, 0755); err != nil {
209+
t.Fatalf("Failed to create restore subdirectory %d: %v", i, err)
210+
}
211+
212+
// Create a restore file in each subdirectory
213+
restoreFile := filepath.Join(restoreSubDir, fmt.Sprintf("1_default-restore%d.yaml", i))
214+
content := fmt.Sprintf("apiVersion: velero.io/v1\nkind: Restore\nmetadata:\n name: restore%d\nspec:\n backupName: backup%d\n", i, i)
215+
if err := os.WriteFile(restoreFile, []byte(content), 0644); err != nil {
216+
t.Fatalf("Failed to create restore file %d: %v", i, err)
217+
}
218+
}
219+
220+
manifests, err := LoadGroupedManifestsFromPath(restoreDir, &logr.Logger{})
221+
if err != nil {
222+
t.Fatalf("Failed to load restores: %v", err)
223+
}
224+
225+
// Verify we have 12 groups
226+
assert.Equal(t, 12, len(manifests), "Expected 12 restore groups")
227+
228+
// Verify each group has 1 manifest and they are in the correct numeric order
229+
for i := 0; i < 12; i++ {
230+
assert.Equal(t, 1, len(manifests[i]), "Expected 1 manifest in group %d", i)
231+
232+
// Verify the restore name matches the expected numeric order
233+
expectedName := fmt.Sprintf("restore%d", i+1)
234+
actualName := manifests[i][0].GetName()
235+
assert.Equal(t, expectedName, actualName,
236+
"Expected restore at index %d to be %s but got %s", i, expectedName, actualName)
237+
}
238+
}
239+
240+
func TestExtractTrailingNumber(t *testing.T) {
241+
testcases := []struct {
242+
name string
243+
input string
244+
expected int
245+
}{
246+
{
247+
name: "single digit",
248+
input: "restore1",
249+
expected: 1,
250+
},
251+
{
252+
name: "double digit",
253+
input: "restore10",
254+
expected: 10,
255+
},
256+
{
257+
name: "triple digit",
258+
input: "restore123",
259+
expected: 123,
260+
},
261+
{
262+
name: "no number",
263+
input: "restore",
264+
expected: 0,
265+
},
266+
{
267+
name: "number in middle",
268+
input: "restore1test",
269+
expected: 0,
270+
},
271+
{
272+
name: "group prefix",
273+
input: "group2",
274+
expected: 2,
275+
},
276+
}
277+
278+
for _, tc := range testcases {
279+
t.Run(tc.name, func(t *testing.T) {
280+
result := extractTrailingNumber(tc.input)
281+
assert.Equal(t, tc.expected, result)
282+
})
283+
}
284+
}
285+
189286
func TestReadSeedReconfigurationFromFile(t *testing.T) {
190287
type dummySeedReconfiguration struct {
191288
seedreconfig.SeedReconfiguration

0 commit comments

Comments
 (0)