Skip to content
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

Implement model registry inference service reconciliation #135

Merged
merged 15 commits into from
Feb 15, 2024
Merged

Implement model registry inference service reconciliation #135

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
490c0c4
Fix overlays/dev deployment
lampajr Dec 19, 2023
4c03910
Setup InferenceService reconciliation to model registry
lampajr Dec 19, 2023
6eba8e2
Setup e2e test for new reconciler
lampajr Dec 20, 2023
d843346
Enable new reconciler
lampajr Dec 20, 2023
3fe6458
Fix model registry inference service reconciler
lampajr Jan 3, 2024
015f6c6
Create serving env when not found
lampajr Jan 4, 2024
609b1b5
Retrieve MR namespace from ISVC label
lampajr Jan 11, 2024
1a765ee
Added Kind cluster configuration for local e2e tests
lampajr Jan 11, 2024
e055b26
Fix MR InferenceService reconciler
lampajr Jan 11, 2024
9a67c4e
Update model-registry library
lampajr Jan 26, 2024
c9cbe07
Apply suggestions from code review
lampajr Feb 8, 2024
25db572
Apply suggestions from code review
lampajr Feb 8, 2024
9c4e7b4
Check if IS already exists in MR
lampajr Feb 8, 2024
962bd76
Improve e2e testing
lampajr Feb 8, 2024
4cbfd11
Refactor dev manifests
lampajr Feb 9, 2024
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: 23 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,11 @@ vet: ## Run go vet against code.

.PHONY: test
test: manifests generate fmt vet envtest ## Run tests.
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" POD_NAMESPACE=default go test ./... -coverprofile cover.out
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" POD_NAMESPACE=default go test ./controllers/... -coverprofile cover.out

.PHONY: e2e-test
e2e-test: manifests generate fmt vet ## Run e2e-tests.
POD_NAMESPACE=default go test ./test/e2e/...
lampajr marked this conversation as resolved.
Show resolved Hide resolved

##@ Build

Expand Down Expand Up @@ -109,6 +113,24 @@ deploy: manifests kustomize ## Deploy controller to the K8s cluster specified in
undeploy: ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion.
$(KUSTOMIZE) build config/default | kubectl delete --ignore-not-found=$(ignore-not-found) -f -

.PHONY: deploy-dev
deploy-dev: manifests kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config.
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/overlays/dev | kubectl apply -f -

.PHONY: undeploy-dev
undeploy-dev: ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion.
$(KUSTOMIZE) build config/overlays/dev | kubectl delete --ignore-not-found=$(ignore-not-found) -f -

.PHONY: deploy-e2e
deploy-e2e: manifests kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config.
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/overlays/e2e | kubectl apply -f -

.PHONY: undeploy-e2e
undeploy-e2e: ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion.
$(KUSTOMIZE) build config/overlays/e2e | kubectl delete --ignore-not-found=$(ignore-not-found) -f -

##@ Build Dependencies

## Location to install dependencies to
Expand Down
8 changes: 7 additions & 1 deletion config/crd/external/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
resources:
- serving.kserve.io_inferenceservices.yaml
# - route.openshift.io_routes.yaml #minikube
- serving.kserve.io_servingruntimes.yaml
- route.openshift.io_routes.yaml
- monitoring.coreos.com_podmonitors.yaml
- monitoring.coreos.com_servicemonitors.yaml
- maistra.io_servicemeshmemberrolls.yaml
- maistra.io_servicemeshmembers.yaml
- telemetry.istio.io_telemetries.yaml
Copy link
Member

Choose a reason for hiding this comment

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

are all these resources needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I had to add all of them to make e2e tests working in the Kind cluster (i.e., they are used just for deploy-dev and deploy-e2e) - not sure if we want to move them directly in the overlay/e2e config folder

Copy link
Contributor

Choose a reason for hiding this comment

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

@VedantMahabaleshwarkar may have a better answer about if these are OK here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lampajr we have a the overlays/dev directory for this purpose (for local testing etc). I think adding these CRDs here is not necessary and IMO if these are needed to make local testing work they should be added to the overlays/dev/kustomization.yaml file and apply that overlay for local testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree with you @VedantMahabaleshwarkar but I don't know how to reference to crd/external from the overlays/dev without directly changing crd/external/kustomization.yaml - I tried to directly add all CRDs into the overlays/dev/kustomization but I get:

Error: accumulating resources: 2 errors occurred:
	* accumulateFile error: "accumulating resources from '../../crd/external/serving.kserve.io_inferenceservices.yaml': security; file '/home/alampare/repos/odh-model-controller/config/crd/external/serving.kserve.io_inferenceservices.yaml' is not in or below '/home/alampare/repos/odh-model-controller/config/overlays/dev'"
	* loader.New error: "error loading ../../crd/external/serving.kserve.io_inferenceservices.yaml with git: url lacks host: ../../crd/external/serving.kserve.io_inferenceservices.yaml, dir: got file 'serving.kserve.io_inferenceservices.yaml', but '/home/alampare/repos/odh-model-controller/config/crd/external/serving.kserve.io_inferenceservices.yaml' must be a directory to be a root, get: invalid source string: ../../crd/external/serving.kserve.io_inferenceservices.yaml"

any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lampajr looking at the manifests again, if the crd/external directory is not in the call chain for the actual deployment, then it's fine to add these here. We just want to avoid applying these CRDs on the cluster outside of development testing scenarios.

37 changes: 36 additions & 1 deletion config/overlays/dev/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,38 @@
resources:
- namespace.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

In practice, this is extending the deploy-dev installation to a more complete one.
I'm not sure if that was the intended outcome of make deploy-dev. I think @VedantMahabaleshwarkar should comment about this.

This is making the overlays quite similar from each other. We may want to rearrange the manifests to reduce duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if that was the intended outcome of make deploy-dev

Oh yeah I don't know what was the original motivation behind that, if that wasn't the original one I can revert the changes affecting overlays/dev

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's a good idea to duplicate the contents of these overlays as it'll increase the difficulty of maintaining changes. For the deploy-dev installation, the overlays/dev/kustomization.yaml can call the overlays/odh/kustomization.yaml directly to avoid duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I couldn't find any reference to that crd/external/kustomization.yaml, at least not in this repo but I don't know if they could be referenced from other places outside this repo itself.

- ../../crd/external
- ../../manager/namespace.yaml
- ../../default

patchesStrategicMerge:
- odh_model_controller_manager_patch.yaml

namespace: opendatahub
configMapGenerator:
- envs:
- params.env
name: odh-model-controller-parameters
generatorOptions:
disableNameSuffixHash: true

vars:
- fieldref:
fieldPath: metadata.namespace
name: mesh-namespace
objref:
apiVersion: v1
kind: ConfigMap
name: odh-model-controller-parameters
- fieldref:
fieldPath: data.monitoring-namespace
name: monitoring-namespace
objref:
apiVersion: v1
kind: ConfigMap
name: odh-model-controller-parameters
- fieldref:
fieldPath: data.odh-model-controller
name: odh-model-controller
objref:
apiVersion: v1
kind: ConfigMap
name: odh-model-controller-parameters
6 changes: 6 additions & 0 deletions config/overlays/dev/namespace.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
apiVersion: v1
kind: Namespace
metadata:
labels:
control-plane: odh-model-controller
name: opendatahub
17 changes: 17 additions & 0 deletions config/overlays/dev/odh_model_controller_manager_patch.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: odh-model-controller
spec:
replicas: 3
template:
spec:
containers:
- args:
- --leader-elect
- "--monitoring-namespace"
- "$(MONITORING_NS)"
env:
- name: MONITORING_NS
value: $(monitoring-namespace)
name: manager
2 changes: 2 additions & 0 deletions config/overlays/dev/params.env
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
monitoring-namespace=opendatahub
odh-model-controller=quay.io/opendatahub/odh-model-controller:fast
38 changes: 38 additions & 0 deletions config/overlays/e2e/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
resources:
lampajr marked this conversation as resolved.
Show resolved Hide resolved
- namespace.yaml
- ../../crd/external
- ../../default

patchesStrategicMerge:
- odh_model_controller_manager_patch.yaml

namespace: opendatahub-e2e
configMapGenerator:
- envs:
- params.env
name: odh-model-controller-parameters
generatorOptions:
disableNameSuffixHash: true

vars:
- fieldref:
fieldPath: metadata.namespace
name: mesh-namespace
objref:
apiVersion: v1
kind: ConfigMap
name: odh-model-controller-parameters
- fieldref:
fieldPath: data.monitoring-namespace
name: monitoring-namespace
objref:
apiVersion: v1
kind: ConfigMap
name: odh-model-controller-parameters
- fieldref:
fieldPath: data.odh-model-controller
name: odh-model-controller
objref:
apiVersion: v1
kind: ConfigMap
name: odh-model-controller-parameters
6 changes: 6 additions & 0 deletions config/overlays/e2e/namespace.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
apiVersion: v1
kind: Namespace
metadata:
labels:
control-plane: odh-model-controller
name: opendatahub-e2e
14 changes: 14 additions & 0 deletions config/overlays/e2e/odh_model_controller_manager_patch.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: odh-model-controller
spec:
replicas: 1
template:
spec:
containers:
- args:
- --leader-elect
- --model-registry-inference-reconcile
name: manager
imagePullPolicy: IfNotPresent
2 changes: 2 additions & 0 deletions config/overlays/e2e/params.env
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
monitoring-namespace=opendatahub-e2e
odh-model-controller=quay.io/opendatahub/odh-model-controller:fast
1 change: 1 addition & 0 deletions config/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ rules:
verbs:
- get
- list
- update
Copy link
Member

Choose a reason for hiding this comment

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

hmm, which rbac needs to be updated?

I am not about operators updating rbac. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reconcile loop has to update InferenceService CRs, adding some ad-hoc labels to keep the links between the CR and the record in model registry - that's why I had to add that update. Is there any other way I can do that?

- watch
- apiGroups:
- serving.kserve.io
Expand Down
34 changes: 34 additions & 0 deletions controllers/comparators/inferenceservice_comparator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package comparators

import (
"reflect"

kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1"
"github.com/opendatahub-io/odh-model-controller/controllers/constants"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func GetInferenceServiceComparator() ResourceComparator {
return func(deployed client.Object, requested client.Object) bool {
deployedInferenceService := deployed.(*kservev1beta1.InferenceService)
requestedInferenceService := requested.(*kservev1beta1.InferenceService)
return reflect.DeepEqual(deployedInferenceService.Labels[constants.ModelRegistryInferenceServiceIdLabel], requestedInferenceService.Labels[constants.ModelRegistryInferenceServiceIdLabel]) &&
reflect.DeepEqual(deployedInferenceService.Labels[constants.ModelRegistryRegisteredModelIdLabel], requestedInferenceService.Labels[constants.ModelRegistryRegisteredModelIdLabel]) &&
reflect.DeepEqual(deployedInferenceService.Labels[constants.ModelRegistryModelVersionIdLabel], requestedInferenceService.Labels[constants.ModelRegistryModelVersionIdLabel])
}
}
9 changes: 9 additions & 0 deletions controllers/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,12 @@ const (
IstioIngressServiceHTTPPortName = "http2"
IstioIngressServiceHTTPSPortName = "https"
)

// model registry
const (
MLMDAddressEnv = "MLMD_ADDRESS"
ModelRegistryNamespaceLabel = "mr-namespace"
ModelRegistryInferenceServiceIdLabel = "mr-inference-service-id"
ModelRegistryModelVersionIdLabel = "mr-model-version-id"
ModelRegistryRegisteredModelIdLabel = "mr-registered-model-id"
lampajr marked this conversation as resolved.
Show resolved Hide resolved
)
Loading