opendatahub-io / model-registry-operator

Apache License 2.0
3 stars 17 forks source link

fix: refactor defaulting and validating webhooks, fixes RHOAIENG-13113 #135

Closed dhirajsb closed 4 days ago

dhirajsb commented 6 days ago

Description

Refactor defaulting and validating webhooks, so all default values and validation are done in webhooks instead of reconcile Fixes RHOAIENG-13113

How Has This Been Tested?

Manually built and deployed locally and created samples. CI will also test operator.

Merge criteria:

dhirajsb commented 6 days ago

@Al-Pragliola I refactored the messy updates, and validation out of the reconcile loop and moved it to the defaulting and validating webhooks respectively. Please take a look.

dhirajsb commented 6 days ago

Seeing a very strange reconcile error below when trying to reconcile an MR created using an older operator version. I'll take a look tomorrow.

2024-09-19T05:59:50Z    ERROR    Reconciler error    {"controller": "modelregistry", "controllerGroup": "modelregistry.op
endatahub.io", "controllerKind": "ModelRegistry", "ModelRegistry": {"name":"modelregistry-sample","namespace":"odh-model-
registries"}, "namespace": "odh-model-registries", "name": "modelregistry-sample", "reconcileID": "6342b41e-38a1-48b3-b37
c-a0ba9dd21138", "error": "Failed to generate strategic merge patch: merging an object in json but data type is not struc
t, instead is: map", "errorVerbose": "merging an object in json but data type is not struct, instead is: map\nFailed to g
enerate strategic merge patch\ngithub.com/banzaicloud/k8s-objectmatcher/patch.(*PatchMaker).Calculate\n\t/opt/app-root/sr
c/go/pkg/mod/github.com/banzaicloud/k8s-objectmatcher@v1.8.0/patch/patch.go:87\ngithub.com/opendatahub-io/model-registry-
operator/internal/controller.(*ModelRegistryReconciler).createOrUpdate\n\t/workspace/internal/controller/modelregistry_co
ntroller.go:861\ngithub.com/opendatahub-io/model-registry-operator/internal/controller.(*ModelRegistryReconciler).createO
rUpdateAuthConfig\n\t/workspace/internal/controller/modelregistry_controller.go:620\ngithub.com/opendatahub-io/model-regi
stry-operator/internal/controller.(*ModelRegistryReconciler).createOrUpdateIstioConfig\n\t/workspace/internal/controller/
modelregistry_controller.go:420\ngithub.com/opendatahub-io/model-registry-operator/internal/controller.(*ModelRegistryRec
onciler).updateRegistryResources\n\t/workspace/internal/controller/modelregistry_controller.go:373\ngithub.com/opendatahu
b-io/model-registry-operator/internal/controller.(*ModelRegistryReconciler).Reconcile\n\t/workspace/internal/controller/m
odelregistry_controller.go:200\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile\n\t/opt/ap
p-root/src/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.16.3/pkg/internal/controller/controller.go:119\nsigs.k8s.io/contr
oller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/opt/app-root/src/go/pkg/mod/sigs.k8s.io/controll
er-runtime@v0.16.3/pkg/internal/controller/controller.go:316\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Co
ntroller).processNextWorkItem\n\t/opt/app-root/src/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.16.3/pkg/internal/control
ler/controller.go:266\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/opt/app-roo
t/src/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.16.3/pkg/internal/controller/controller.go:227\nruntime.goexit\n\t/usr
/lib/golang/src/runtime/asm_amd64.s:1598"}                                                                               
dhirajsb commented 5 days ago

@Al-Pragliola found the tricky issue deep in k8s patch implementation. I've fixed it so that it can handle older model registries and authconfig changes correctly. Please review so we can get this change into this week's odh 2.18.2 release.

dhirajsb commented 4 days ago

@Al-Pragliola good point about the non-strict yaml unmarshal in your feedback in the other issue. I switched it in the operator to avoid that possibility since it was a simple enough change.