Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
48 changes: 46 additions & 2 deletions pkg/cli/admin/mustgather/mustgather.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
"k8s.io/kubectl/pkg/scheme"
"k8s.io/kubectl/pkg/util/templates"
admissionapi "k8s.io/pod-security-admission/api"
"k8s.io/utils/clock"
"k8s.io/utils/exec"
utilptr "k8s.io/utils/ptr"

Expand Down Expand Up @@ -73,7 +74,9 @@ var (
`)

mustGatherExample = templates.Examples(`
# Gather information using the default plug-in image and command, writing into ./must-gather.local.<rand>
# Gather information using the default plug-in image and command, writing into
# ./must-gather.local.<cluster-id-suffix>.<timestamp>.<rand>
# or ./must-gather.local.<timestamp>.<rand> if the cluster ID is unavailable
oc adm must-gather

# Gather information with a specific local folder to copy to
Expand Down Expand Up @@ -230,7 +233,7 @@ func (o *MustGatherOptions) Complete(f kcmdutil.Factory, cmd *cobra.Command, arg
}
}
if len(o.DestDir) == 0 {
o.DestDir = fmt.Sprintf("must-gather.local.%06d", rand.Int63())
o.DestDir = o.generateDestDir(context.TODO())
}
// TODO: this should be in Validate() method, but added here because of the call to o.completeImages() below
if o.AllImages {
Expand Down Expand Up @@ -266,6 +269,46 @@ func (o *MustGatherOptions) Complete(f kcmdutil.Factory, cmd *cobra.Command, arg
return nil
}

// generateDestDir builds the default destination directory name for must-gather output.
// The format includes a partial cluster ID (last 12 characters), a UTC timestamp, and a
// random ID to help distinguish must-gather archives from different clusters and collection
// times. If the cluster ID cannot be retrieved (e.g. cluster is unreachable), it falls back
// to the timestamp and random ID only.
func (o *MustGatherOptions) getClock() clock.PassiveClock {
if o.clock != nil {
return o.clock
}
return clock.RealClock{}
}

// See: https://issues.redhat.com/browse/RFE-5434
func (o *MustGatherOptions) generateDestDir(ctx context.Context) string {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Honestly would be nicer to pass a clock.Clock interface so that the tests are deterministic. And then align the tests to check for exact names instead of just contains.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good suggestion, done. Added a clock.PassiveClock field to MustGatherOptions — that's a smaller interface from k8s.io/utils/clock that only requires Now() and Since(), which is all we need here (the full Clock interface includes timers that we don't use).

In production, it's initialized with clock.RealClock{} in NewMustGatherOptions(), so it calls the real time.Now(). In tests, I created a simple fakeClock struct that returns a fixed time (2026-04-14T12:00:00Z), which lets us assert exact prefixes like must-gather.local.76708af6b91c.20260414T120000Z. instead of regex patterns.

The random suffix from rand.Int63() is still non-deterministic, but that's fine — the important parts (cluster ID and timestamp) are now fully testable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@tchap just checking in on this. All your feedback has been addressed (reordering, clock.PassiveClock, klog.V(4) debug logging, fallback format in example text). Could you take a quick look when you get a chance? Also, would you be able to run /ok-to-test so CI can pick it up? Thanks!

clusterIDSuffix := ""
if o.ConfigClient != nil {
lookupCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()
cv, err := o.ConfigClient.ConfigV1().ClusterVersions().Get(lookupCtx, "version", metav1.GetOptions{})
if err == nil {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
id := string(cv.Spec.ClusterID)
if len(id) >= 12 {
clusterIDSuffix = id[len(id)-12:]
} else if len(id) > 0 {
clusterIDSuffix = id
}
} else {
klog.V(4).Infof("unable to retrieve cluster ID for directory name: %v", err)
}
}

timestamp := o.getClock().Now().UTC().Format("20060102T150405Z")
randID := rand.Int63()

if clusterIDSuffix != "" {
return fmt.Sprintf("must-gather.local.%s.%s.%06d", clusterIDSuffix, timestamp, randID)
}
return fmt.Sprintf("must-gather.local.%s.%06d", timestamp, randID)
}

func (o *MustGatherOptions) completeImages(ctx context.Context) error {
for _, imageStream := range o.ImageStreams {
if image, err := o.resolveImageStreamTagString(ctx, imageStream); err == nil {
Expand Down Expand Up @@ -398,6 +441,7 @@ type MustGatherOptions struct {
SinceTime string

RsyncRshCmd string
clock clock.PassiveClock

PrinterCreated printers.ResourcePrinter
PrinterDeleted printers.ResourcePrinter
Expand Down
75 changes: 75 additions & 0 deletions pkg/cli/admin/mustgather/mustgather_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"path"
"reflect"
"strings"
"testing"
"time"

Expand All @@ -28,6 +29,80 @@ import (
imageclient "github.com/openshift/client-go/image/clientset/versioned/fake"
)

// fakeClock implements clock.PassiveClock for deterministic tests.
type fakeClock struct {
t time.Time
}

func (f fakeClock) Now() time.Time { return f.t }
func (f fakeClock) Since(ts time.Time) time.Duration { return f.t.Sub(ts) }

func TestGenerateDestDir(t *testing.T) {
fixedTime := time.Date(2026, 4, 14, 12, 0, 0, 0, time.UTC)
fc := fakeClock{t: fixedTime}

tests := []struct {
name string
clusterID string
expectedPrefix string
}{
{
name: "with full cluster ID includes last 12 chars",
clusterID: "0194fffc-f70a-4776-b00d-76708af6b91c",
expectedPrefix: "must-gather.local.76708af6b91c.20260414T120000Z.",
},
{
name: "with short cluster ID uses full ID",
clusterID: "abcdef",
expectedPrefix: "must-gather.local.abcdef.20260414T120000Z.",
},
{
name: "without cluster ID falls back gracefully",
expectedPrefix: "must-gather.local.20260414T120000Z.",
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
var configObjects []runtime.Object
if tc.clusterID != "" {
configObjects = []runtime.Object{
&configv1.ClusterVersion{
ObjectMeta: metav1.ObjectMeta{Name: "version"},
Spec: configv1.ClusterVersionSpec{
ClusterID: configv1.ClusterID(tc.clusterID),
},
},
}
}
options := MustGatherOptions{
IOStreams: genericiooptions.NewTestIOStreamsDiscard(),
ConfigClient: configv1fake.NewSimpleClientset(configObjects...),
clock: fc,
}
destDir := options.generateDestDir(context.TODO())
if !strings.HasPrefix(destDir, tc.expectedPrefix) {
t.Errorf("expected prefix %q, got %q", tc.expectedPrefix, destDir)
}
})
}
}

func TestGenerateDestDirNoConfigClient(t *testing.T) {
fixedTime := time.Date(2026, 4, 14, 12, 0, 0, 0, time.UTC)
fc := fakeClock{t: fixedTime}

options := MustGatherOptions{
IOStreams: genericiooptions.NewTestIOStreamsDiscard(),
ConfigClient: nil,
clock: fc,
}
destDir := options.generateDestDir(context.TODO())
if !strings.HasPrefix(destDir, "must-gather.local.20260414T120000Z.") {
t.Errorf("expected prefix 'must-gather.local.20260414T120000Z.', got %q", destDir)
}
}

func TestImagesAndImageStreams(t *testing.T) {

testCases := []struct {
Expand Down