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.

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

Allow ModelMesh and KServe to run in same namespace. #132

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
13 changes: 12 additions & 1 deletion config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,18 @@ metadata:
creationTimestamp: null
name: odh-model-controller-role
rules:
- apiGroups:
- ""
resources:
- serviceaccounts
verbs:
- create
- get
- list
- patch
- update
- watch
- delete
- apiGroups:
- ""
resources:
Expand All @@ -13,7 +25,6 @@ rules:
- namespaces
- pods
- secrets
- serviceaccounts
- services
verbs:
- create
Expand Down
86 changes: 61 additions & 25 deletions controllers/inferenceservice_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,31 +115,12 @@ func (r *OpenshiftInferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager)
Watches(&source.Kind{Type: &kservev1alpha1.ServingRuntime{}},
handler.EnqueueRequestsFromMapFunc(func(o client.Object) []reconcile.Request {
r.log.Info("Reconcile event triggered by serving runtime: " + o.GetName())
inferenceServicesList := &kservev1beta1.InferenceServiceList{}
opts := []client.ListOption{client.InNamespace(o.GetNamespace())}

// Todo: Get only Inference Services that are deploying on the specific serving runtime
err := r.client.List(context.TODO(), inferenceServicesList, opts...)
if err != nil {
r.log.Info("Error getting list of inference services for namespace")
return []reconcile.Request{}
}

if len(inferenceServicesList.Items) == 0 {
r.log.Info("No InferenceServices found for Serving Runtime: " + o.GetName())
return []reconcile.Request{}
}

reconcileRequests := make([]reconcile.Request, 0, len(inferenceServicesList.Items))
for _, inferenceService := range inferenceServicesList.Items {
reconcileRequests = append(reconcileRequests, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: inferenceService.Name,
Namespace: inferenceService.Namespace,
},
})
}
return reconcileRequests
return r.getReconcileRequestsOnUpdateOfServingRuntime(o)
})).
Watches(&source.Kind{Type: &networkingv1.NetworkPolicy{}},
spolti marked this conversation as resolved.
Show resolved Hide resolved
handler.EnqueueRequestsFromMapFunc(func(o client.Object) []reconcile.Request {
r.log.Info("Reconcile event triggered by Network Policy: " + o.GetName())
return r.getReconcileRequestsOnUpdateOfServingRuntime(o)

Choose a reason for hiding this comment

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

Is it right to call the getReconcileRequestsOnUpdateOfServingRuntime and not the getReconcileRequestsOnUpdateOfNetworkPolicy function?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, you are right. I make the wrong function call. Its not tracked in my testing earlier because both function do similiar task.
I had fixed the function call now.

}))
err := builder.Complete(r)
if err != nil {
Expand All @@ -149,6 +130,61 @@ func (r *OpenshiftInferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager)
return nil
}

func (r *OpenshiftInferenceServiceReconciler) getReconcileRequestsOnUpdateOfServingRuntime(o client.Object) []reconcile.Request {
inferenceServicesList := &kservev1beta1.InferenceServiceList{}
opts := []client.ListOption{client.InNamespace(o.GetNamespace())}

// Todo: Get only Inference Services that are deploying on the specific serving runtime
err := r.client.List(context.TODO(), inferenceServicesList, opts...)
if err != nil {
r.log.Info("Error getting list of inference services for namespace")
return []reconcile.Request{}
}

if len(inferenceServicesList.Items) == 0 {
r.log.Info("No InferenceServices found for Network Policy: " + o.GetName())
return []reconcile.Request{}
}

reconcileRequests := make([]reconcile.Request, 0, len(inferenceServicesList.Items))
for _, inferenceService := range inferenceServicesList.Items {
reconcileRequests = append(reconcileRequests, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: inferenceService.Name,
Namespace: inferenceService.Namespace,
},
})
}
return reconcileRequests
}

func (r *OpenshiftInferenceServiceReconciler) getReconcileRequestsOnUpdateOfNetworkPolicy(o client.Object) []reconcile.Request {

Choose a reason for hiding this comment

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

Do we really need this function? It seems to be equal, except 1 comment to the getReconcileRequestsOnUpdateOfServingRuntime

Copy link
Member Author

Choose a reason for hiding this comment

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

for now they are doing same task to return list of all InferenceServices in that namespace but we have task in our backlog to update the logic of getReconcileRequestsOnUpdateOfServingRuntime function.
https://issues.redhat.com/browse/RHOAIENG-1265

inferenceServicesList := &kservev1beta1.InferenceServiceList{}
opts := []client.ListOption{client.InNamespace(o.GetNamespace())}

err := r.client.List(context.TODO(), inferenceServicesList, opts...)
if err != nil {
r.log.Info("Error getting list of inference services for namespace")
return []reconcile.Request{}
}

if len(inferenceServicesList.Items) == 0 {
r.log.Info("No InferenceServices found for Network Policy: " + o.GetName())
return []reconcile.Request{}
}

reconcileRequests := make([]reconcile.Request, 0, len(inferenceServicesList.Items))
for _, inferenceService := range inferenceServicesList.Items {
reconcileRequests = append(reconcileRequests, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: inferenceService.Name,
Namespace: inferenceService.Namespace,
},
})
}
return reconcileRequests
}

// general clean-up, mostly resources in different namespaces from kservev1beta1.InferenceService
func (r *OpenshiftInferenceServiceReconciler) onDeletion(ctx context.Context, log logr.Logger, inferenceService *kservev1beta1.InferenceService) error {
log.V(1).Info("Running cleanup logic")
Expand Down
15 changes: 15 additions & 0 deletions controllers/kserve_inferenceservice_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,21 @@ var _ = Describe("The Openshift Kserve model controller", func() {

})

//Context("Validate Openshift Monitoring Stack", func() {
spolti marked this conversation as resolved.
Show resolved Hide resolved
// It("Namespace should be added to SMMR", func() {
// smmr := &v1.ServiceMeshMemberRoll{}
// err := cli.Get(ctx, types.NamespacedName{Name: reconcilers.ServiceMeshMemberRollName, Namespace: constants.IstioNamespace}, smmr)
// Expect(err).NotTo(HaveOccurred())
//
// })
//
// It("Prometheus cluster role binding should be configured", func() {
// smmr := &v1.ServiceMeshMemberRoll{}
// err := cli.Get(ctx, types.NamespacedName{Name: reconcilers.ServiceMeshMemberRollName, Namespace: constants.IstioNamespace}, smmr)
// Expect(err).NotTo(HaveOccurred())
// })
//})

It("With Kserve InferenceService a Route be created", func() {
inferenceService := &kservev1beta1.InferenceService{}
err := convertToStructuredResource(KserveInferenceServicePath1, inferenceService)
Expand Down
4 changes: 4 additions & 0 deletions controllers/reconcilers/kserve_istio_podmonitor_reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ func (r *KserveIstioPodMonitorReconciler) createDesiredResource(isvc *kservev1be
Key: "istio-prometheus-ignore",
Operator: metav1.LabelSelectorOpDoesNotExist,
},
{
Key: "modelmesh-service",
Operator: metav1.LabelSelectorOpDoesNotExist,
},
},
},
PodMetricsEndpoints: []v1.PodMetricsEndpoint{
Expand Down
141 changes: 141 additions & 0 deletions controllers/reconcilers/mm_controller_networkpolicy_reconciler.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
/*

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 reconcilers

import (
"context"
"github.com/go-logr/logr"
kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1"
"github.com/kserve/kserve/pkg/constants"
"github.com/opendatahub-io/odh-model-controller/controllers/comparators"
"github.com/opendatahub-io/odh-model-controller/controllers/processors"
"github.com/opendatahub-io/odh-model-controller/controllers/resources"
"k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
)

type MMControllerNetworkPolicyReconciler struct {

Choose a reason for hiding this comment

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

Do we need the *Controller* into the name?

Copy link
Member Author

Choose a reason for hiding this comment

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

To fix this issue, I need to implement two NetworkPolicies :

  • allow access to expose Route
  • allow calls from odh-model-controller

To make visible distinction between these two NetworkPolicies, I had create file with below names :

  • mm_route_networkpolicy_reconciler.go
  • mm_controller_networkpolicy_reconciler.go

I am open to change their name to be more meaningful.

client client.Client
scheme *runtime.Scheme
networkPolicyHandler resources.NetworkPolicyHandler
deltaProcessor processors.DeltaProcessor
}

func NewMMControllerNetworkPolicyReconciler(client client.Client, scheme *runtime.Scheme) *MMControllerNetworkPolicyReconciler {
return &MMControllerNetworkPolicyReconciler{
client: client,
scheme: scheme,
networkPolicyHandler: resources.NewNetworkPolicyHandler(client),
deltaProcessor: processors.NewDeltaProcessor(),
}
}

func (r *MMControllerNetworkPolicyReconciler) Reconcile(ctx context.Context, log logr.Logger, isvc *kservev1beta1.InferenceService) error {

// Create Desired resource
desiredResource, err := r.createDesiredResource(isvc)
if err != nil {
return err
}

// Get Existing resource
existingResource, err := r.getExistingResource(ctx, log, isvc)
if err != nil {
return err
}

// Process Delta
if err = r.processDelta(ctx, log, desiredResource, existingResource); err != nil {
return err
}
return nil
}

func (r *MMControllerNetworkPolicyReconciler) createDesiredResource(isvc *kservev1beta1.InferenceService) (*v1.NetworkPolicy, error) {
spolti marked this conversation as resolved.
Show resolved Hide resolved
desiredNetworkPolicy := &v1.NetworkPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: getMMControllerNetworkPolicyName(),
Namespace: isvc.Namespace,
},
Spec: v1.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{},
Ingress: []v1.NetworkPolicyIngressRule{
{
From: []v1.NetworkPolicyPeer{
{
NamespaceSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"kubernetes.io/metadata.name": constants.KServeNamespace,
},
},
},
},
},
},
PolicyTypes: []v1.PolicyType{v1.PolicyTypeIngress},
},
}
return desiredNetworkPolicy, nil
}

func (r *MMControllerNetworkPolicyReconciler) getExistingResource(ctx context.Context, log logr.Logger, isvc *kservev1beta1.InferenceService) (*v1.NetworkPolicy, error) {
return r.networkPolicyHandler.FetchNetworkPolicy(ctx, log, types.NamespacedName{Name: getMMControllerNetworkPolicyName(), Namespace: isvc.Namespace})
}

func (r *MMControllerNetworkPolicyReconciler) processDelta(ctx context.Context, log logr.Logger, desiredPod *v1.NetworkPolicy, existingPod *v1.NetworkPolicy) (err error) {
comparator := comparators.GetNetworkPolicyComparator()
delta := r.deltaProcessor.ComputeDelta(comparator, desiredPod, existingPod)

if !delta.HasChanges() {
log.V(1).Info("No delta found")
return nil
}

if delta.IsAdded() {
log.V(1).Info("Delta found", "create", desiredPod.GetName())
if err = r.client.Create(ctx, desiredPod); err != nil {
return
}
}
if delta.IsUpdated() {
log.V(1).Info("Delta found", "update", existingPod.GetName())
rp := existingPod.DeepCopy()
rp.Labels = desiredPod.Labels
rp.Spec = desiredPod.Spec

if err = r.client.Update(ctx, rp); err != nil {
return
}
}
if delta.IsRemoved() {
log.V(1).Info("Delta found", "delete", existingPod.GetName())
if err = r.client.Delete(ctx, existingPod); err != nil {
return
}
}
return nil
}

func getMMControllerNetworkPolicyName() string {
return "allow-from-" + constants.KServeNamespace + "-ns"
}

func (r *MMControllerNetworkPolicyReconciler) DeleteMMControllerNetworkPolicy(ctx context.Context, isvcNamespace string) error {
return r.networkPolicyHandler.DeleteNetworkPolicy(ctx, types.NamespacedName{Name: getMMControllerNetworkPolicyName(), Namespace: isvcNamespace})
}
25 changes: 25 additions & 0 deletions controllers/reconcilers/mm_inferenceservice_reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ type ModelMeshInferenceServiceReconciler struct {
routeReconciler *ModelMeshRouteReconciler
serviceAccountReconciler *ModelMeshServiceAccountReconciler
clusterRoleBindingReconciler *ModelMeshClusterRoleBindingReconciler
controllerNetworkPolicy *MMControllerNetworkPolicyReconciler
routeNetworkPolicy *MMRouteNetworkPolicyReconciler
}

func NewModelMeshInferenceServiceReconciler(client client.Client, scheme *runtime.Scheme) *ModelMeshInferenceServiceReconciler {
Expand All @@ -37,6 +39,8 @@ func NewModelMeshInferenceServiceReconciler(client client.Client, scheme *runtim
routeReconciler: NewModelMeshRouteReconciler(client, scheme),
serviceAccountReconciler: NewModelMeshServiceAccountReconciler(client, scheme),
clusterRoleBindingReconciler: NewModelMeshClusterRoleBindingReconciler(client, scheme),
controllerNetworkPolicy: NewMMControllerNetworkPolicyReconciler(client, scheme),
routeNetworkPolicy: NewMMRouteNetworkPolicyReconciler(client, scheme),
}
}

Expand All @@ -56,6 +60,17 @@ func (r *ModelMeshInferenceServiceReconciler) Reconcile(ctx context.Context, log
if err := r.clusterRoleBindingReconciler.Reconcile(ctx, log, isvc); err != nil {
return err
}

log.V(1).Info("Reconciling NetworkPolicy to allow traffic from Controller")
if err := r.controllerNetworkPolicy.Reconcile(ctx, log, isvc); err != nil {
return err
}

log.V(1).Info("Reconciling NetworkPolicy to allow traffic from external routes")
if err := r.routeNetworkPolicy.Reconcile(ctx, log, isvc); err != nil {
return err
}

return nil
}

Expand Down Expand Up @@ -84,6 +99,16 @@ func (r *ModelMeshInferenceServiceReconciler) DeleteModelMeshResourcesIfNoMMIsvc
if err := r.clusterRoleBindingReconciler.DeleteClusterRoleBinding(ctx, isvcNamespace); err != nil {
return err
}

log.V(1).Info("Deleting Controller network policy object for target namespace")
if err := r.controllerNetworkPolicy.DeleteMMControllerNetworkPolicy(ctx, isvcNamespace); err != nil {
return err
}

log.V(1).Info("Deleting Route network policy object for target namespace")
if err := r.routeNetworkPolicy.DeleteMMRouteNetworkPolicy(ctx, isvcNamespace); err != nil {
return err
}
}
return nil
}
Loading