-
Notifications
You must be signed in to change notification settings - Fork 160
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.
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
(feat): Add migration logic for odhdashboardconfig #1709
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -615,3 +615,179 @@ | |
|
||
return nil | ||
} | ||
|
||
// TODO: to be removed: https://issues.redhat.com/browse/RHOAIENG-21080 | ||
func PatchOdhDashboardConfig(ctx context.Context, cli client.Client) error { | ||
log := logf.FromContext(ctx) | ||
|
||
var dashboardConfigs unstructured.UnstructuredList | ||
dashboardConfigs.SetGroupVersionKind(gvk.OdhDashboardConfig) | ||
|
||
if err := cli.List(ctx, &dashboardConfigs); err != nil { | ||
log.Error(err, "Failed to list odhdashboardconfig CRs") | ||
return fmt.Errorf("error listing odhdashboardconfig CRs: %w", err) | ||
} | ||
|
||
for _, cr := range dashboardConfigs.Items { | ||
if !cluster.IsNotReservedNamespace(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cr.GetNamespace()}}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this required ? my understanding is that there is only one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have changed the logic to handle the singleton case |
||
continue | ||
} | ||
|
||
log.Info("Found CR, applying patch", "namespace", cr.GetNamespace(), "name", cr.GetName()) | ||
|
||
patch := cr.DeepCopy() | ||
updates := map[string][]any{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nip: since the updates map is always the same, can't this be just a var ? not sure I see the benefits of having the values returned by functions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. var would do fine 😄 |
||
"notebookSizes": getNotebookSizesData(), | ||
"modelServerSizes": getModelServerSizeData(), | ||
} | ||
|
||
updated, err := updateSpecFields(patch, updates) | ||
if err != nil { | ||
return fmt.Errorf("failed to update odhdashboardconfig spec fields: %w", err) | ||
} | ||
|
||
if !updated { | ||
log.Info("No changes needed, skipping patch", "namespace", cr.GetNamespace(), "name", cr.GetName()) | ||
continue | ||
} | ||
|
||
if err := cli.Patch(ctx, patch, client.MergeFrom(&cr)); err != nil { | ||
return fmt.Errorf("failed to patch CR %s in namespace %s: %w", cr.GetName(), cr.GetNamespace(), err) | ||
} | ||
|
||
log.Info("Patched odhdashboardconfig successfully", "namespace", cr.GetNamespace(), "name", cr.GetName()) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// TODO: to be removed: https://issues.redhat.com/browse/RHOAIENG-21080 | ||
func updateSpecFields(obj *unstructured.Unstructured, updates map[string][]any) (bool, error) { | ||
updated := false | ||
|
||
for field, newData := range updates { | ||
existingField, exists, err := unstructured.NestedSlice(obj.Object, "spec", field) | ||
if err != nil { | ||
return false, fmt.Errorf("failed to get field '%s': %w", field, err) | ||
} | ||
|
||
if !exists || len(existingField) == 0 { | ||
if err := unstructured.SetNestedSlice(obj.Object, newData, "spec", field); err != nil { | ||
return false, fmt.Errorf("failed to set field '%s': %w", field, err) | ||
} | ||
updated = true | ||
} | ||
} | ||
|
||
return updated, nil | ||
} | ||
|
||
// TODO: to be removed: https://issues.redhat.com/browse/RHOAIENG-21080 | ||
func getNotebookSizesData() []any { | ||
return []any{ | ||
map[string]any{ | ||
"name": "Small", | ||
"resources": map[string]any{ | ||
"requests": map[string]any{ | ||
"cpu": "1", | ||
"memory": "8Gi", | ||
}, | ||
"limits": map[string]any{ | ||
"cpu": "2", | ||
"memory": "8Gi", | ||
}, | ||
}, | ||
}, | ||
map[string]any{ | ||
"name": "Medium", | ||
"resources": map[string]any{ | ||
"requests": map[string]any{ | ||
"cpu": "3", | ||
"memory": "24Gi", | ||
}, | ||
"limits": map[string]any{ | ||
"cpu": "6", | ||
"memory": "24Gi", | ||
}, | ||
}, | ||
}, | ||
map[string]any{ | ||
"name": "Large", | ||
"resources": map[string]any{ | ||
"requests": map[string]any{ | ||
"cpu": "7", | ||
"memory": "56Gi", | ||
}, | ||
"limits": map[string]any{ | ||
"cpu": "14", | ||
"memory": "56Gi", | ||
}, | ||
}, | ||
}, | ||
map[string]any{ | ||
"name": "X Large", | ||
"resources": map[string]any{ | ||
"requests": map[string]any{ | ||
"cpu": "15", | ||
"memory": "120Gi", | ||
}, | ||
"limits": map[string]any{ | ||
"cpu": "30", | ||
"memory": "120Gi", | ||
}, | ||
}, | ||
}, | ||
} | ||
} | ||
|
||
// TODO: to be removed: https://issues.redhat.com/browse/RHOAIENG-21080 | ||
func getModelServerSizeData() []any { | ||
return []any{ | ||
map[string]any{ | ||
"name": "Small", | ||
"resources": map[string]any{ | ||
"requests": map[string]any{ | ||
"cpu": "1", | ||
"memory": "4Gi", | ||
}, | ||
"limits": map[string]any{ | ||
"cpu": "2", | ||
"memory": "8Gi", | ||
}, | ||
}, | ||
}, | ||
map[string]any{ | ||
"name": "Medium", | ||
"resources": map[string]any{ | ||
"requests": map[string]any{ | ||
"cpu": "4", | ||
"memory": "8Gi", | ||
}, | ||
"limits": map[string]any{ | ||
"cpu": "8", | ||
"memory": "10Gi", | ||
}, | ||
}, | ||
}, | ||
map[string]any{ | ||
"name": "Large", | ||
"resources": map[string]any{ | ||
"requests": map[string]any{ | ||
"cpu": "6", | ||
"memory": "16Gi", | ||
}, | ||
"limits": map[string]any{ | ||
"cpu": "10", | ||
"memory": "20Gi", | ||
}, | ||
}, | ||
}, | ||
map[string]any{ | ||
"name": "Custom", | ||
"resources": map[string]any{ | ||
"requests": map[string]any{}, | ||
"limits": map[string]any{}, | ||
}, | ||
}, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this function should also check the current and previous version to determine if the patch should be applied ? something like https://github.com/opendatahub-io/opendatahub-operator/blob/main/pkg/upgrade/upgrade.go#L503
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep it can be done, although I may need some guidance on which versions should be patched
@Gkrumbach07 ^