Skip to content

Surface inaccessible GDrive items and root folders in SourceCrawler status #294

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 4 commits into
base: main
Choose a base branch
from
Open

Surface inaccessible GDrive items and root folders in SourceCrawler status #294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aeece59
Surface inaccessible GDrive items in SourceCrawler status
Jul 20, 2026
336a92b
Address comments
Jul 20, 2026
7fd1d32
Address comments
Jul 27, 2026
02cca15
Surface inaccessible root folders in SourceCrawler status
Jul 29, 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
29 changes: 26 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,34 @@ type SourceCrawlerSpec struct {
SourceCrawlerConfig SourceCrawlerConfig `json:"sourceCrawlerConfig,omitempty"`
}

type InaccessibleItem struct {
Kind string `json:"kind"`
ItemID string `json:"itemID"`
Name string `json:"name,omitempty"`
TargetID string `json:"targetID,omitempty"`
RootFolderID string `json:"rootFolderID"`
Reason string `json:"reason,omitempty"`
}

type InaccessibleSummary struct {
// +listType=atomic
Items []InaccessibleItem `json:"items,omitempty"`
Count int `json:"count"`
}

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"`
InaccessibleItems *InaccessibleSummary `json:"inaccessibleItems,omitempty"`
InaccessibleRootFolders []InaccessibleRootFolder `json:"inaccessibleRootFolders,omitempty"`
}

// +kubebuilder:object:root=true
Expand Down
60 changes: 60 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.

43 changes: 43 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,49 @@ spec:
filesProcessed:
format: int64
type: integer
inaccessibleItems:
properties:
count:
type: integer
items:
items:
properties:
itemID:
type: string
kind:
type: string
name:
type: string
reason:
type: string
rootFolderID:
type: string
targetID:
type: string
required:
- itemID
- kind
- rootFolderID
type: object
type: array
x-kubernetes-list-type: atomic
required:
- count
type: object
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
83 changes: 78 additions & 5 deletions internal/controller/sourcecrawler_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,15 +138,44 @@ func (r *SourceCrawlerReconciler) Reconcile(ctx context.Context, req ctrl.Reques
return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("unsupported source type: %s", sourceCrawlerConfig.Type))
}

storedFiles, err := source.SyncFilesToFilestore(ctx, r.fileStore)
if err != nil {
return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("failed to store files to filestore: %w", err))
syncResult, syncErr := source.SyncFilesToFilestore(ctx, r.fileStore)

// Build inaccessible root folders status from GDriveSource if applicable
var failedRootFolders []operatorv1alpha1.InaccessibleRootFolder
if gds, ok := source.(*unstructured.GDriveSource); ok && len(gds.FailedRootFolders) > 0 {
failedRootFolders = buildInaccessibleRootFolders(gds.FailedRootFolders, sourceCrawlerConfig.GoogleDriveConfig)
}

if syncErr != nil {
if syncResult != nil {
inaccessible, _ := buildInaccessibleSummary(syncResult.InaccessibleItems)
if err := controllerutils.StatusPatch(ctx, r.Client, sourceCrawlerCR, func() {
sourceCrawlerCR.Status.FilesProcessed += int64(len(syncResult.StoredFiles))
sourceCrawlerCR.Status.InaccessibleItems = inaccessible

@vinamra28 vinamra28 Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we might have to see what's the upper cap that CR can hold else we might end up blowing 1.5MB limit

sourceCrawlerCR.Status.InaccessibleRootFolders = failedRootFolders
sourceCrawlerCR.UpdateStatus("", syncErr)

@PuneetPunamiya PuneetPunamiya Jul 22, 2026

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.

Are we updating the status twice here ?

@vinamra28 vinamra28 Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agreed, we are writing error twice, @gshikhar2021, can you please fix this?

@PuneetPunamiya PuneetPunamiya Jul 28, 2026

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.

@gshikhar2021 can we address this comment please, as this will hit the kube api server redundantly

Comment on lines +149 to +156

@coderabbitai coderabbitai Bot Jul 29, 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

Include the inaccessible-item count in failure conditions.

The error path discards total (inaccessible, _) and calls UpdateStatus("", syncErr), while only the success path appends the count. Partial failures therefore publish the summary but omit its count from the reconciliation condition message. Reuse total when constructing the error condition 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 149 - 156, The
failure path in the sync status update discards the inaccessible-item total and
omits it from the condition message. In the syncErr handling block, retain the
total returned by buildInaccessibleSummary and pass a message to
sourceCrawlerCR.UpdateStatus that includes that count, matching the success-path
condition format.

}); err != nil {
logger.Error(err, "failed to update SourceCrawler CR status with partial results")
return ctrl.Result{}, err
}
}
return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, syncErr)
Comment on lines +149 to +162

@coderabbitai coderabbitai Bot Jul 29, 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.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Avoid the second status patch on partial sync errors.

When syncResult != nil and syncErr != nil, Lines 152-160 patch partial status, then Line 162 calls handleError, which issues another status patch. Combine the partial fields and failing condition in one StatusPatch, then return the error after that patch; this is the same redundant-write issue previously raised.

🤖 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 149 - 162,
Update the sync-error path in the controller reconciliation flow so the partial
status fields and failing condition are applied in a single StatusPatch when
syncResult is non-nil. Avoid calling handleError after that patch, and return
the sync error directly while preserving existing patch-error handling.

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

successMessage := fmt.Sprintf("successfully reconciled source crawler: %s", sourceCrawlerCR.Name)
if len(failedRootFolders) > 0 {
successMessage += fmt.Sprintf(", %d inaccessible root folders", len(failedRootFolders))
}
inaccessible, total := buildInaccessibleSummary(syncResult.InaccessibleItems)
if inaccessible != nil {
successMessage += fmt.Sprintf(", %d inaccessible items (service account may lack access)", total)
}

if err := controllerutils.StatusPatch(ctx, r.Client, sourceCrawlerCR, func() {
sourceCrawlerCR.Status.FilesProcessed += int64(len(storedFiles))
sourceCrawlerCR.Status.FilesProcessed += int64(len(syncResult.StoredFiles))
sourceCrawlerCR.Status.InaccessibleItems = inaccessible
sourceCrawlerCR.Status.InaccessibleRootFolders = failedRootFolders
sourceCrawlerCR.UpdateStatus(successMessage, nil)
}); err != nil {
logger.Error(err, "failed to update SourceCrawler CR status")
Expand Down Expand Up @@ -308,6 +337,50 @@ func (r *SourceCrawlerReconciler) handleError(ctx context.Context, sourceCrawler
return reconcileErr
}

const maxInaccessibleItems = 100

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
Comment on lines +342 to +358

@coderabbitai coderabbitai Bot Jul 29, 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.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Bound inaccessible root-folder status details.

maxInaccessibleItems caps only item details; buildInaccessibleRootFolders copies every failed root and its unbounded error string into status. Many inaccessible roots can therefore make the status patch exceed Kubernetes object-size limits. Cap this list too and preserve the uncapped total in the condition/message, or enforce equivalent input limits.

🤖 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 342 - 358,
Update buildInaccessibleRootFolders to cap the returned InaccessibleRootFolder
entries using the existing maxInaccessibleItems limit, including only the
allowed prefix while preserving the total failed-root count for the
condition/message that reports status. Ensure oversized Error strings or
equivalent unbounded root-folder details cannot bypass the status-size bound.

}

func buildInaccessibleSummary(items []unstructured.InaccessibleItem) (*operatorv1alpha1.InaccessibleSummary, int) {
total := len(items)
if total == 0 {
return nil, 0
}
capped := items
if len(capped) > maxInaccessibleItems {
capped = capped[:maxInaccessibleItems]
}
apiItems := make([]operatorv1alpha1.InaccessibleItem, 0, len(capped))
for _, item := range capped {
apiItems = append(apiItems, operatorv1alpha1.InaccessibleItem{
Kind: item.Kind,
ItemID: item.ItemID,
Name: item.Name,
TargetID: item.TargetID,
RootFolderID: item.RootFolderID,
Reason: item.Reason,
})
}
return &operatorv1alpha1.InaccessibleSummary{Items: apiItems, Count: total}, total
}

// findDependents maps a changed pipeline stage back to the SourceCrawlers that depend on it.
//
// Given a SourceCrawler CR like:
Expand Down
Loading
Loading