Skip to content

Surface inaccessible root folders in SourceCrawler status #304

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
gshikhar2021 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Surface inaccessible root folders in SourceCrawler status #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 10 additions & 3 deletions api/v1alpha1/sourcecrawler_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,18 @@ type SourceCrawlerSpec struct {
SourceCrawlerConfig SourceCrawlerConfig `json:"sourceCrawlerConfig,omitempty"`
}

type InaccessibleRootFolder struct {
FolderID string `json:"folderID"`
URL string `json:"url,omitempty"`
Error string `json:"error"`
}

// SourceCrawlerStatus defines the observed state of SourceCrawler.
type SourceCrawlerStatus struct {
LastAppliedGeneration int64 `json:"lastAppliedGeneration,omitempty"`
Conditions []metav1.Condition `json:"conditions,omitempty"`
FilesProcessed int64 `json:"filesProcessed,omitempty"`
LastAppliedGeneration int64 `json:"lastAppliedGeneration,omitempty"`
Conditions []metav1.Condition `json:"conditions,omitempty"`
FilesProcessed int64 `json:"filesProcessed,omitempty"`
InaccessibleRootFolders []InaccessibleRootFolder `json:"inaccessibleRootFolders,omitempty"`
}

// +kubebuilder:object:root=true
Expand Down
20 changes: 20 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions config/crd/bases/operator.dataverse.redhat.com_sourcecrawlers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,20 @@ spec:
filesProcessed:
format: int64
type: integer
inaccessibleRootFolders:
items:
properties:
error:
type: string
folderID:
type: string
url:
type: string
required:
- error
- folderID
type: object
type: array
lastAppliedGeneration:
format: int64
type: integer
Expand Down
34 changes: 34 additions & 0 deletions internal/controller/sourcecrawler_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,14 +139,29 @@ func (r *SourceCrawlerReconciler) Reconcile(ctx context.Context, req ctrl.Reques
}

storedFiles, err := source.SyncFilesToFilestore(ctx, r.fileStore)

var failedRootFolders []operatorv1alpha1.InaccessibleRootFolder
if gds, ok := source.(*unstructured.GDriveSource); ok && len(gds.FailedRootFolders) > 0 {
failedRootFolders = buildInaccessibleRootFolders(gds.FailedRootFolders, sourceCrawlerConfig.GoogleDriveConfig)
}

if err != nil {
if err := controllerutils.StatusPatch(ctx, r.Client, sourceCrawlerCR, func() {
sourceCrawlerCR.Status.InaccessibleRootFolders = failedRootFolders
}); err != nil {
logger.Error(err, "failed to update SourceCrawler CR status with inaccessible root folders")
}
return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("failed to store files to filestore: %w", err))
}
Comment on lines 148 to 155

@coderabbitai coderabbitai Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 12 'func \(r \*SourceCrawlerReconciler\) handleError|UpdateStatus\(' \
  internal/controller/sourcecrawler_controller.go

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 2707

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# First, let's see the full function to understand the context
rg -n 'failedRootFolders' internal/controller/sourcecrawler_controller.go

# Also search for where roots are crawled and where errors from that are handled
rg -n 'source\.' internal/controller/sourcecrawler_controller.go | head -20

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 815

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Get the full Reconcile function to see the exact control flow
sed -n '120,200p' internal/controller/sourcecrawler_controller.go

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 3920

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Let's look at what UpdateStatus does
rg -n 'func.*UpdateStatus' internal/controller/sourcecrawler_controller.go

# Also check if there's a definition in the SourceCrawler type
fd -t f '\.go$' | xargs rg -l 'func.*UpdateStatus' | head -5

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 185

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search for UpdateStatus definition across the codebase
rg -n 'UpdateStatus' --type go | grep -E 'func.*UpdateStatus|UpdateStatus\s*\(' | head -20

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 185

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search for UpdateStatus definition
rg 'UpdateStatus' --type go -n | head -20

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 185

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find all go files and search for UpdateStatus
find . -name "*.go" -type f | xargs grep -n "UpdateStatus" 2>/dev/null | head -30

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 4885

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the SourceCrawler type definition and its UpdateStatus method
find . -name "*.go" -type f | xargs grep -l "type SourceCrawler" | grep -v vendor

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 326

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look at the SourceCrawler type definition and UpdateStatus method
cat -n api/v1alpha1/sourcecrawler_types.go

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 6024

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Now let's verify the exact scenario: when SyncFilesToFilestore returns an error, 
# are failedRootFolders available and is the count included in the message?

# Looking at lines 141-155 in context:
sed -n '141,170p' internal/controller/sourcecrawler_controller.go

echo "---"
echo "The UpdateStatus method from the types file:"
sed -n '122,145p' api/v1alpha1/sourcecrawler_types.go

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 2449

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Let's trace what happens with failedRootFolders in both paths
# 1. Error path (lines 147-155): failedRootFolders is set in status but not in the error message
# 2. Success path (lines 158-165): failedRootFolders is set in status AND included in the message

# Let's check: in the error path, what message is being passed to handleError?
# Line 154 passes: fmt.Errorf("failed to store files to filestore: %w", err)
# This doesn't include failedRootFolders count

# Let's verify by looking at what message is actually constructed
echo "=== Error path message construction ==="
sed -n '148,155p' internal/controller/sourcecrawler_controller.go

echo ""
echo "=== Success path message construction ==="
sed -n '158,165p' internal/controller/sourcecrawler_controller.go

echo ""
echo "=== What handleError does with the message ==="
sed -n '332,343p' internal/controller/sourcecrawler_controller.go

Repository: redhat-data-and-ai/unstructured-data-controller

Length of output: 1720

Include the inaccessible-root count in the error-path status message.

When SyncFilesToFilestore returns an error, failedRootFolders is populated and stored in the CR status. However, the error message passed to handleError at line 154 contains only the storage error, not the count of inaccessible roots. The UpdateStatus method concatenates the message with the error text, so the condition message becomes ", error: ..." without the count.

In contrast, the success path at lines 159–160 explicitly includes the count: fmt.Sprintf(", %d inaccessible root folders", len(failedRootFolders)). For consistency and visibility, pass the count in the error path as well. Modify the message passed to handleError to include len(failedRootFolders) when constructing the error message.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/sourcecrawler_controller.go` around lines 148 - 155,
Update the error message passed to handleError in the SyncFilesToFilestore error
path to include the inaccessible root count using the same format as the success
path. Include len(failedRootFolders) in the fmt.Errorf call so the message
contains both the storage error and the count of inaccessible roots, matching
the consistency and visibility of the success-path message construction.

logger.Info("successfully stored files to filestore", "count", len(storedFiles))

successMessage := fmt.Sprintf("successfully reconciled source crawler: %s", sourceCrawlerCR.Name)
if len(failedRootFolders) > 0 {
successMessage += fmt.Sprintf(", %d inaccessible root folders", len(failedRootFolders))
}
Comment on lines 158 to +161

@coderabbitai coderabbitai Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use singular wording for one inaccessible root folder.

For one failure, the message is 1 inaccessible root folders. Handle the singular form.

Proposed fix 
-		successMessage += fmt.Sprintf(", %d inaccessible root folders", len(failedRootFolders))
+		count := len(failedRootFolders)
+		label := "folders"
+		if count == 1 {
+			label = "folder"
+		}
+		successMessage += fmt.Sprintf(", %d inaccessible root %s", count, label)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
successMessage := fmt.Sprintf("successfully reconciled source crawler: %s", sourceCrawlerCR.Name)
if len(failedRootFolders) > 0 {
successMessage += fmt.Sprintf(", %d inaccessible root folders", len(failedRootFolders))
}
successMessage := fmt.Sprintf("successfully reconciled source crawler: %s", sourceCrawlerCR.Name)
if len(failedRootFolders) > 0 {
count := len(failedRootFolders)
label := "folders"
if count == 1 {
label = "folder"
}
successMessage += fmt.Sprintf(", %d inaccessible root %s", count, label)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/sourcecrawler_controller.go` around lines 158 - 161,
Update the successMessage construction in the source crawler reconciliation flow
to use singular wording when failedRootFolders contains exactly one item, while
retaining the plural wording for multiple inaccessible root folders.

if err := controllerutils.StatusPatch(ctx, r.Client, sourceCrawlerCR, func() {
sourceCrawlerCR.Status.FilesProcessed += int64(len(storedFiles))
sourceCrawlerCR.Status.InaccessibleRootFolders = failedRootFolders
sourceCrawlerCR.UpdateStatus(successMessage, nil)
}); err != nil {
logger.Error(err, "failed to update SourceCrawler CR status")
Expand Down Expand Up @@ -295,6 +310,25 @@ func extractGDriveFolderID(rawURL string) (string, error) {
return "", fmt.Errorf("could not extract folder ID from URL path: %s", u.Path)
}

func buildInaccessibleRootFolders(failed []unstructured.FailedRootFolder, gdriveConfig *operatorv1alpha1.GoogleDriveConfig) []operatorv1alpha1.InaccessibleRootFolder {
folderIDToURL := make(map[string]string, len(gdriveConfig.Folders))
for _, f := range gdriveConfig.Folders {
id, err := extractGDriveFolderID(f.URL)
if err == nil {
folderIDToURL[id] = f.URL
}
}
result := make([]operatorv1alpha1.InaccessibleRootFolder, 0, len(failed))
for _, f := range failed {
result = append(result, operatorv1alpha1.InaccessibleRootFolder{
FolderID: f.FolderID,
URL: folderIDToURL[f.FolderID],
Error: f.Error,
})
}
return result
}

func (r *SourceCrawlerReconciler) handleError(ctx context.Context, sourceCrawlerCR *operatorv1alpha1.SourceCrawler, err error) error {
logger := log.FromContext(ctx)
logger.Error(err, "encountered error")
Expand Down
14 changes: 14 additions & 0 deletions pkg/unstructured/source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,11 @@ func (s *S3BucketSource) s3Key(filestorePath string) string {
return path.Join(s.Prefix, baseName)
}

type FailedRootFolder struct {
FolderID string
Error string
}

// GDriveSource implements DataSource for Google Drive folders.
type GDriveSource struct {
GDriveClient *gdrive.Client
Expand All @@ -245,6 +250,7 @@ type GDriveSource struct {
ConcurrentFolders int
ConcurrentDownloads int
OutputDir string
FailedRootFolders []FailedRootFolder
}

// Close releases resources held by the underlying clients.
Expand Down Expand Up @@ -285,6 +291,10 @@ func (g *GDriveSource) SyncFilesToFilestore(ctx context.Context, fs *filestore.F
for i, r := range results {
if r.err != nil {
logger.Error(r.err, "folder crawl failed", "folderID", g.FolderIDs[i])
g.FailedRootFolders = append(g.FailedRootFolders, FailedRootFolder{
FolderID: g.FolderIDs[i],
Error: r.err.Error(),
})
Comment on lines +294 to +297

@coderabbitai coderabbitai Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Prevent garbage collection from deleting files from failed roots.

When one root crawl fails, this code continues with records from successful roots. The later garbage-collection phase builds currentFiles only from those records and deletes local files and permission files that are not present. Cached files from the inaccessible root are therefore deleted during a partial crawl.

Skip garbage collection while any root folder has failed, or limit deletion to roots whose crawls completed successfully. Add a regression test for a cached file from a failed root.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/unstructured/source.go` around lines 294 - 297, Update the crawl and
garbage-collection flow around FailedRootFolders so files belonging to
inaccessible roots are preserved: either skip garbage collection whenever any
root crawl fails, or restrict deletion to successfully crawled roots. Ensure
cached files and permission files from failed roots are not deleted, and add a
regression test covering a cached file from a failed root.

continue
}
for _, record := range r.result.Records {
Expand All @@ -307,6 +317,10 @@ func (g *GDriveSource) SyncFilesToFilestore(ctx context.Context, fs *filestore.F
}
}

if len(g.FailedRootFolders) == len(g.FolderIDs) {
return nil, errors.New("all configured root folders are inaccessible (service account may lack access)")
}

logger.Info("gdrive crawl complete, starting file download",
"discoveredFiles", len(fileRecords),
"concurrentDownloads", g.ConcurrentDownloads,
Expand Down
Loading