Skip to content
Merged
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
24 changes: 24 additions & 0 deletions pkg/lint/check/result/result.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ import (
"github.com/opendatahub-io/odh-cli/pkg/resources"
)

const (
// AnnotationResourceCRDName is the annotation key for the CRD fully-qualified name
// (e.g., "notebooks.kubeflow.org"). Automatically set by SetImpactedObjects and
// AddImpactedObjects from the ResourceType.
AnnotationResourceCRDName = "resource.opendatahub.io/crd-name"
)

const (
// Validation error messages.
errMsgGroupEmpty = "group must not be empty"
Expand Down Expand Up @@ -324,10 +331,16 @@ func (r *DiagnosticResult) SetCondition(condition Condition) {
}

// SetImpactedObjects replaces all impacted objects from a list of NamespacedNames.
// Also stores the CRD fully-qualified name as an annotation for downstream formatters.
func (r *DiagnosticResult) SetImpactedObjects(
resourceType resources.ResourceType,
names []types.NamespacedName,
) {
if r.Annotations == nil {
r.Annotations = make(map[string]string)
}

r.Annotations[AnnotationResourceCRDName] = resourceType.CRDFQN()
Comment thread
coderabbitai[bot] marked this conversation as resolved.
r.ImpactedObjects = make([]metav1.PartialObjectMetadata, 0, len(names))

for _, n := range names {
Expand All @@ -342,10 +355,21 @@ func (r *DiagnosticResult) SetImpactedObjects(
}

// AddImpactedObjects appends impacted objects from a list of NamespacedNames.
// Stores the CRD fully-qualified name as an annotation only if not already set,
// so a prior SetImpactedObjects call is preserved. Each appended object carries
// its own TypeMeta, which downstream formatters can use for per-object type info.
func (r *DiagnosticResult) AddImpactedObjects(
resourceType resources.ResourceType,
names []types.NamespacedName,
) {
if r.Annotations == nil {
r.Annotations = make(map[string]string)
}

if _, ok := r.Annotations[AnnotationResourceCRDName]; !ok {
r.Annotations[AnnotationResourceCRDName] = resourceType.CRDFQN()
}

for _, n := range names {
r.ImpactedObjects = append(r.ImpactedObjects, metav1.PartialObjectMetadata{
TypeMeta: resourceType.TypeMeta(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
// that will be auto-migrated to HardwareProfiles (infrastructure.opendatahub.io) during RHOAI 3.x upgrade.
type AcceleratorMigrationCheck struct {
check.BaseCheck
NotebookVerboseFormatter
}

func NewAcceleratorMigrationCheck() *AcceleratorMigrationCheck {
Expand Down
1 change: 1 addition & 0 deletions pkg/lint/checks/workloads/notebook/connection_integrity.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
// notebook's namespace.
type ConnectionIntegrityCheck struct {
check.BaseCheck
NotebookVerboseFormatter
}

func NewConnectionIntegrityCheck() *ConnectionIntegrityCheck {
Expand Down
1 change: 1 addition & 0 deletions pkg/lint/checks/workloads/notebook/container_name.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
// Dashboard-managed annotations (accelerator profile or size selection) are checked.
type ContainerNameCheck struct {
check.BaseCheck
NotebookVerboseFormatter
}

// NewContainerNameCheck creates a new ContainerNameCheck.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
// via annotations point to HardwareProfiles that actually exist on the cluster.
type HardwareProfileIntegrityCheck struct {
check.BaseCheck
NotebookVerboseFormatter
}

func NewHardwareProfileIntegrityCheck() *HardwareProfileIntegrityCheck {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
// opendatahub.io/legacy-hardware-profile-name annotation that may need attention.
type HardwareProfileMigrationCheck struct {
check.BaseCheck
NotebookVerboseFormatter
}

// NewHardwareProfileMigrationCheck creates a new HardwareProfileMigrationCheck.
Expand Down
97 changes: 82 additions & 15 deletions pkg/lint/checks/workloads/notebook/impacted.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
iolib "io"
"regexp"
"sort"
"strconv"
"strings"

Expand Down Expand Up @@ -120,15 +121,21 @@ func NewImpactedWorkloadsCheck() *ImpactedWorkloadsCheck {
}

// FormatVerboseOutput implements check.VerboseOutputFormatter.
// Groups notebook impacted objects by image for better readability.
// Groups notebook impacted objects by image, then by namespace within each image group.
//
// Output format:
//
// image: registry/path:tag (N notebooks)
// - namespace/name
// - namespace/name
// <status-label>: registry/path:tag (N notebooks)
// - namespace: <ns>
// - <crd-fqn>/<name>
// - <crd-fqn>/<name>
// - namespace: <ns>
// - <crd-fqn>/<name>
func (c *ImpactedWorkloadsCheck) FormatVerboseOutput(out iolib.Writer, dr *result.DiagnosticResult) {
crdName := crdFQN(dr)

// Group notebooks by image reference, preserving insertion order.
// Within each image group, track notebooks per namespace.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
var groups []imageGroup

imageIndex := make(map[string]int) // imageRef -> index in groups
Expand All @@ -141,38 +148,93 @@ func (c *ImpactedWorkloadsCheck) FormatVerboseOutput(out iolib.Writer, dr *resul

imageStatus := obj.Annotations[AnnotationCheckImageStatus]

ns := obj.Namespace
name := obj.Name
if obj.Namespace != "" {
name = obj.Namespace + "/" + name
}

if idx, ok := imageIndex[imageRef]; ok {
groups[idx].notebooks = append(groups[idx].notebooks, name)
groups[idx].namespaces[ns] = append(groups[idx].namespaces[ns], name)
groups[idx].count++
} else {
imageIndex[imageRef] = len(groups)
groups = append(groups, imageGroup{
imageRef: imageRef,
imageStatus: imageStatus,
notebooks: []string{name},
namespaces: map[string][]string{ns: {name}},
count: 1,
})
}
}

// Sort image groups: problematic (incompatible) before custom, then by imageRef for determinism.
sort.SliceStable(groups, func(i, j int) bool {
oi, oj := imageStatusOrder(groups[i].imageStatus), imageStatusOrder(groups[j].imageStatus)
if oi != oj {
return oi < oj
}

return groups[i].imageRef < groups[j].imageRef
})

for _, g := range groups {
imageLabel := imageStatusLabel(g.imageStatus)
_, _ = fmt.Fprintf(out, " %s: %s (%d notebooks)\n", imageLabel, g.imageRef, len(g.notebooks))
_, _ = fmt.Fprintf(out, " %s: %s (%d notebooks)\n", imageLabel, g.imageRef, g.count)

for _, nb := range g.notebooks {
_, _ = fmt.Fprintf(out, " - %s\n", nb)
// Sort namespaces for deterministic output.
namespaces := make([]string, 0, len(g.namespaces))
for ns := range g.namespaces {
namespaces = append(namespaces, ns)
}
sort.Strings(namespaces)

for _, ns := range namespaces {
names := g.namespaces[ns]
sort.Strings(names)

if ns == "" {
// Cluster-scoped objects listed without namespace header.
for _, name := range names {
_, _ = fmt.Fprintf(out, " - %s/%s\n", crdName, name)
}
} else {
_, _ = fmt.Fprintf(out, " - namespace: %s\n", ns)
for _, name := range names {
_, _ = fmt.Fprintf(out, " - %s/%s\n", crdName, name)
}
}
}

_, _ = fmt.Fprintln(out)
}
}

// imageGroup holds notebooks grouped by their image reference.
// imageGroup holds notebooks grouped by their image reference, with sub-grouping by namespace.
type imageGroup struct {
imageRef string
imageStatus string // CUSTOM, PROBLEMATIC, etc.
notebooks []string // namespace/name format
imageStatus string // CUSTOM, PROBLEMATIC, etc.
namespaces map[string][]string // namespace -> []name
count int // total notebook count across all namespaces
}

// Image status sort priorities (lower = higher severity).
const (
imageStatusOrderProblematic = iota
imageStatusOrderCustom
imageStatusOrderOther
)

// imageStatusOrder returns a sort key for image statuses.
// Lower values sort first: problematic before custom.
func imageStatusOrder(status string) int {
switch ImageStatus(status) {
case ImageStatusProblematic:
return imageStatusOrderProblematic
case ImageStatusCustom:
return imageStatusOrderCustom
case ImageStatusGood, ImageStatusVerifyFailed:
return imageStatusOrderOther
}

return imageStatusOrderOther
}

// imageStatusLabel returns a user-friendly label for the image status.
Expand Down Expand Up @@ -1194,6 +1256,11 @@ func (c *ImpactedWorkloadsCheck) setImpactedObjects(
})
}

if dr.Annotations == nil {
dr.Annotations = make(map[string]string)
}

dr.Annotations[result.AnnotationResourceCRDName] = resources.Notebook.CRDFQN()
Comment thread
coderabbitai[bot] marked this conversation as resolved.
dr.ImpactedObjects = impacted
}

Expand Down
1 change: 1 addition & 0 deletions pkg/lint/checks/workloads/notebook/running_workloads.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
// A Notebook is considered running when it does not have the kubeflow-resource-stopped annotation.
type RunningWorkloadsCheck struct {
check.BaseCheck
NotebookVerboseFormatter
}

func NewRunningWorkloadsCheck() *RunningWorkloadsCheck {
Expand Down
139 changes: 139 additions & 0 deletions pkg/lint/checks/workloads/notebook/verbose_formatter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
package notebook

import (
"fmt"
"io"
"sort"
"strings"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/opendatahub-io/odh-cli/pkg/lint/check/result"
)

// NotebookVerboseFormatter provides a namespace-grouped rendering for impacted objects
// in verbose table output. Embed this in any check struct to get a default
// FormatVerboseOutput that lists objects grouped by namespace with requester info
// and CRD-qualified resource names.
//
// Output format:
//
// namespace: <ns> | requester: <email>
// - <resource>.<group>/<name>
// - <resource>.<group>/<name>
//
// The CRD FQN (e.g. "notebooks.kubeflow.org") is read from the DiagnosticResult's
// AnnotationResourceCRDName annotation, which is automatically set by SetImpactedObjects
// and AddImpactedObjects. This makes the formatter adoptable by any resource type
// without hardcoding.
//
// Checks that need custom formatting (e.g. ImpactedWorkloadsCheck groups by image)
// should define their own FormatVerboseOutput method instead.
type NotebookVerboseFormatter struct {
// NamespaceRequesters maps namespace names to their openshift.io/requester annotation value.
// Populated automatically by the output renderer before FormatVerboseOutput is called.
NamespaceRequesters map[string]string
}

// SetNamespaceRequesters sets the namespace-to-requester mapping.
// Called by the output renderer before FormatVerboseOutput.
func (f *NotebookVerboseFormatter) SetNamespaceRequesters(m map[string]string) {
f.NamespaceRequesters = m
}

// FormatVerboseOutput implements check.VerboseOutputFormatter.
// Renders impacted objects grouped by namespace with requester info and CRD FQN.
func (f *NotebookVerboseFormatter) FormatVerboseOutput(out io.Writer, dr *result.DiagnosticResult) {
crdName := crdFQN(dr)

nsMap := make(map[string][]string)
for _, obj := range dr.ImpactedObjects {
nsMap[obj.Namespace] = append(nsMap[obj.Namespace], obj.Name)
}

namespaces := make([]string, 0, len(nsMap))
for ns := range nsMap {
namespaces = append(namespaces, ns)
}
sort.Strings(namespaces)

for idx, ns := range namespaces {
names := make([]string, len(nsMap[ns]))
copy(names, nsMap[ns])
sort.Strings(names)

if ns == "" {
// Cluster-scoped objects listed without namespace header.
for _, name := range names {
_, _ = fmt.Fprintf(out, " - %s/%s\n", crdName, name)
}
} else {
nsHeader := "namespace: " + ns
if f.NamespaceRequesters != nil {
if requester, ok := f.NamespaceRequesters[ns]; ok && requester != "" {
nsHeader = fmt.Sprintf("namespace: %s | requester: %s", ns, requester)
}
}

_, _ = fmt.Fprintf(out, " %s\n", nsHeader)
for _, name := range names {
_, _ = fmt.Fprintf(out, " - %s/%s\n", crdName, name)
}
}

// Blank line between namespace groups (except after last).
if idx < len(namespaces)-1 {
_, _ = fmt.Fprintln(out)
}
}
}

// crdFQN returns the CRD fully-qualified name for the impacted objects in a DiagnosticResult.
// It first checks the AnnotationResourceCRDName annotation (automatically set by SetImpactedObjects
// and AddImpactedObjects from ResourceType.CRDFQN()). Falls back to deriving the FQN from the
// first impacted object's TypeMeta if the annotation is absent.
func crdFQN(dr *result.DiagnosticResult) string {
// Prefer the annotation set by SetImpactedObjects — uses ResourceType.Resource
// for the correct plural form (e.g. "inferenceservices", not "inferenceservices" from naive pluralization).
if name, ok := dr.Annotations[result.AnnotationResourceCRDName]; ok && name != "" {
return name
}

// Fallback: derive from TypeMeta (for callers that set ImpactedObjects directly).
return deriveCRDFQNFromTypeMeta(dr.ImpactedObjects)
}

// deriveCRDFQNFromTypeMeta derives the CRD FQN from the first impacted object's TypeMeta.
// For example, APIVersion "kubeflow.org/v1" with Kind "Notebook" produces "notebooks.kubeflow.org".
// Falls back to the lowercase Kind if TypeMeta is not populated.
func deriveCRDFQNFromTypeMeta(objects []metav1.PartialObjectMetadata) string {
if len(objects) == 0 {
return ""
}

obj := objects[0]

kind := obj.Kind
if kind == "" {
return ""
}

// Kubernetes CRD plural convention: lowercase kind + "s"
// (e.g. Notebook → notebooks, InferenceService → inferenceservices, RayCluster → rayclusters).
plural := strings.ToLower(kind) + "s"

// Extract API group from APIVersion (e.g. "kubeflow.org/v1" → "kubeflow.org").
// Core resources have no group (APIVersion is just "v1"), so return plural only.
apiVersion := obj.APIVersion
if apiVersion == "" {
return plural
}

group, _, found := strings.Cut(apiVersion, "/")
if !found {
// Core API (e.g. "v1") — no group prefix.
return plural
}

return plural + "." + group
}
Loading