Skip to content

RFE-5434: Add cluster ID and timestamp to must-gather directory name #2252

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

Open
asadawar wants to merge 5 commits into
base: main
Choose a base branch
from
Open

RFE-5434: Add cluster ID and timestamp to must-gather directory name #2252

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dda7aec
RFE-5434: Add cluster ID and timestamp to must-gather directory name
Apr 13, 2026
9a50bdb
Document fallback directory name format in example text
Apr 14, 2026
6483683
Unexport clock field and add getClock() helper per review feedback
Apr 20, 2026
2d3c35a
Fix import ordering: clock before exec
Apr 20, 2026
177b4d9
Fix gofmt formatting in test file
Apr 20, 2026
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
42 changes: 40 additions & 2 deletions pkg/cli/admin/mustgather/mustgather.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
"k8s.io/kubectl/pkg/util/templates"
admissionapi "k8s.io/pod-security-admission/api"
"k8s.io/utils/exec"
"k8s.io/utils/clock"
utilptr "k8s.io/utils/ptr"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

configclient "github.com/openshift/client-go/config/clientset/versioned"
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 @@ -185,6 +188,7 @@ func NewMustGatherOptions(streams genericiooptions.IOStreams) *MustGatherOptions
IOStreams: streams,
Timeout: 10 * time.Minute,
VolumePercentage: defaultVolumePercentage,
Clock: clock.RealClock{},
Copy link
Copy Markdown
Contributor

@tchap tchap Apr 20, 2026

Choose a reason for hiding this comment

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

Better to export as few fields as necessary. I would rename this to clock and only set it in tests. And in generateDestDir() you can do

clock := o.clock
if clock == nil {
    clock = clock.RealClock{}
}

Or feel free to add a helper method on MustGatherOptions like getClock() and that one returns o.clock or RealClock{} when that field is unset.

Copy link
Copy Markdown
Author

@asadawar asadawar Apr 20, 2026

Choose a reason for hiding this comment

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

Good point, updated. Renamed to unexported clock field, removed the default from the constructor, and added a getClock() helper that returns RealClock{} when nil. Went with the helper approach since there's already a similar getNamespace() pattern in the same file. Tests set the field directly since they're in the same package.

}
opts.LogOut = opts.newPrefixWriter(streams.Out, "[must-gather ] OUT", false, true)
opts.RawOut = opts.newPrefixWriter(streams.Out, "", false, false)
Expand Down Expand Up @@ -230,7 +234,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 +270,39 @@ 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.
// See: https://issues.redhat.com/browse/RFE-5434
func (o *MustGatherOptions) generateDestDir(ctx context.Context) string {
Copy link
Copy Markdown
Contributor

@tchap tchap Apr 14, 2026

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

@asadawar asadawar Apr 14, 2026

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

@asadawar asadawar Apr 20, 2026

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.Clock.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 +435,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
View file
Open in desktop
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