add additional function which creates network policy #498

KPostOffice · 2024-04-03T21:24:58Z

Issue link

What changes have been made

Verification steps

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- Testing is not required for this change

etirelli

I am approving it, but please take a look at my comment. It seems strange to log the error but not return a failure in the reconciliation.

etirelli · 2024-04-03T22:11:22Z

pkg/controllers/raycluster_controller.go

+	_, err = r.kubeClient.NetworkingV1().NetworkPolicies(cluster.Namespace).Apply(ctx, desiredNetworkPolicy(&cluster), metav1.ApplyOptions{FieldManager: controllerName, Force: true})
+	if err != nil {
+		logger.Error(err, "Failed to update NetworkPolicy")
+	}


shouldn't the error be returned? I see the same is done for the previous objects (ClusterRoleBindings, ServiceAccounts, etc)

Just reviewing this and maybe the comment got moved around but was this addressed?

I was planning to open another PR that fixes all the instances of this issue once this is merged

openshift-ci · 2024-04-03T22:17:24Z

@etirelli: changing LGTM is restricted to collaborators

In response to this:

I am approving it, but please take a look at my comment. It seems strange to log the error but not return a failure in the reconciliation.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2024-04-03T22:17:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: etirelli
Once this PR has been reviewed and has the lgtm label, please ask for approval from kpostoffice. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

astefanutti · 2024-04-04T07:57:57Z

pkg/controllers/raycluster_controller.go

+					).WithFrom(
+					networkingapply.NetworkPolicyPeer().WithPodSelector(metav1apply.LabelSelector().
+						WithMatchLabels(map[string]string{"app.kubernetes.io/component": "kuberay-operator"})).
+						WithNamespaceSelector(metav1apply.LabelSelector().WithMatchLabels(map[string]string{"kubernetes.io/metadata.name": "redhat-ods-applications"})),


We need to expand the list of namespaces where ODH / RHOAI are known to be deployed, e.g. opendatahub.

If possible, the DSCInitialization resource should be get to read the .spec.applicationsNamespace field.

pkg/controllers/raycluster_controller.go

astefanutti · 2024-04-05T07:41:35Z

pkg/controllers/raycluster_controller.go

+				networkingapply.NetworkPolicyIngressRule().WithFrom(
+					networkingapply.NetworkPolicyPeer().WithPodSelector(metav1apply.LabelSelector().
+						WithMatchLabels(map[string]string{"app.kubernetes.io/component": "kuberay-operator"})).
+						WithNamespaceSelector(metav1apply.LabelSelector().WithMatchLabels(map[string]string{"opendatahub.io/generated-namespace": "true"})),


I don't think relying on the opendatahub.io/generated-namespace label is correct. The ingress traffic should be allowed from the ODH / RHOAI application namespace. So it should either be sourced from the DSCInitilization resource, or defaulted to the well-known namespaces.

should we hardcode it opendatahub for ODH and redhat-ods-applications for RHOAI to simplify the logic for now?
and think about how to get it from DSCI later?
i mentioned it in another thread, operator does make this namespace configurable, but unlikely in ODH user will change the default value

sutaakar · 2024-04-15T06:47:28Z

pkg/controllers/raycluster_controller.go

+	// - Or fallback to the well-known defaults
+	var kubeRayNamespaces []string
+	dsci := &dsciv1.DSCInitialization{}
+	err := r.Client.Get(ctx, client.ObjectKey{Name: "default-dsci"}, dsci)


Can you load DSCI without hardcoding name? This will make code more resilient for possible naming changes.

default-dsci is the one created by ODH/RHOAI operator.
99% case i would expect it is using this name, but ofc if user tried to create CR by themselves, that will do.
for the time being, we could hardcode with this name.
a follow up PR can add more logic "check CR, get name, set here"

sutaakar · 2024-04-15T09:07:15Z

It would be good to provide a component test coverage, though may be added as separate PR.

sutaakar · 2024-04-15T11:15:46Z

main.go

@@ -70,6 +71,8 @@ func init() {
 	utilruntime.Must(rayv1.AddToScheme(scheme))
 	// OpenShift Route
 	utilruntime.Must(routev1.Install(scheme))
+	// ODH
+	utilruntime.Must(odhv1.AddToScheme(scheme))


Got error when testing this code for RHOAI:

2024-04-15T13:11:03+02:00 ERROR Reconciler error {"controller": "codeflare-raycluster-controller", "controllerGroup": "ray.io", "controllerKind": "RayCluster", "RayCluster": {"name":"jobtest","namespace":"ksuta-test"}, "namespace": "ksuta-test", "name": "jobtest", "reconcileID": "3f44de47-9706-40d1-a470-62a4db9869dc", "error": "no kind is registered for the type v1.DSCInitialization in scheme \"pkg/runtime/scheme.go:100\""} sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler /home/ksuta/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:324

Was able to fix it by using scheme from dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"

astefanutti · 2024-04-19T08:43:22Z

pkg/controllers/raycluster_controller.go

+							WithMatchExpressions(metav1apply.LabelSelectorRequirement().
+								WithKey(corev1.LabelMetadataName).
+								WithOperator(metav1.LabelSelectorOpIn).
+								WithValues("openshift-user-workload-monitoring"))),


I think that should be openshift-monitoring instead.

does SRE monitoring need access redhat-ods-applications (your ray controller) ? or this is for UWM?

pkg/controllers/raycluster_controller.go

Signed-off-by: Kevin <[email protected]>

astefanutti · 2024-04-19T14:14:39Z

/lgtm

openshift-ci bot requested review from Fiona-Waters and sutaakar April 3, 2024 21:25

etirelli approved these changes Apr 3, 2024

View reviewed changes

astefanutti reviewed Apr 4, 2024

View reviewed changes

zdtsw reviewed Apr 4, 2024

View reviewed changes

pkg/controllers/raycluster_controller.go Show resolved Hide resolved

KPostOffice force-pushed the rhaoieng-5089 branch 2 times, most recently from 4675a36 to 9791604 Compare April 4, 2024 16:14

astefanutti reviewed Apr 5, 2024

View reviewed changes

KPostOffice mentioned this pull request Apr 5, 2024

Rhaoieng 5089 #503

Closed

4 tasks

KPostOffice force-pushed the rhaoieng-5089 branch from 9791604 to 950d7a6 Compare April 5, 2024 20:50

openshift-merge-robot added the needs-rebase label Apr 5, 2024

KPostOffice force-pushed the rhaoieng-5089 branch from 950d7a6 to e2dface Compare April 5, 2024 20:53

openshift-merge-robot removed the needs-rebase label Apr 5, 2024

openshift-merge-robot added the needs-rebase label Apr 13, 2024

sutaakar reviewed Apr 15, 2024

View reviewed changes

KPostOffice force-pushed the rhaoieng-5089 branch from 7c6cbc9 to 3086d1a Compare April 15, 2024 15:55

openshift-merge-robot removed the needs-rebase label Apr 15, 2024

KPostOffice force-pushed the rhaoieng-5089 branch from 3086d1a to 827545d Compare April 15, 2024 20:32

openshift-merge-robot added the needs-rebase label Apr 15, 2024

KPostOffice force-pushed the rhaoieng-5089 branch from 827545d to b94c3c4 Compare April 15, 2024 20:34

openshift-merge-robot removed the needs-rebase label Apr 15, 2024

KPostOffice force-pushed the rhaoieng-5089 branch 2 times, most recently from 0969607 to fc84c9c Compare April 18, 2024 16:40

franciscojavierarceo mentioned this pull request Apr 18, 2024

Enable MTLS by default - Webhook #523

Closed

4 tasks

astefanutti reviewed Apr 19, 2024

View reviewed changes

pkg/controllers/raycluster_controller.go Show resolved Hide resolved

KPostOffice force-pushed the rhaoieng-5089 branch from e1ab362 to 416a9a2 Compare April 19, 2024 13:12

openshift-merge-robot added the needs-rebase label Apr 19, 2024

KPostOffice force-pushed the rhaoieng-5089 branch from 416a9a2 to c9d028b Compare April 19, 2024 13:21

KPostOffice and others added 7 commits April 19, 2024 09:29

add additional function which creates network policy

9eafb5b

Signed-off-by: Kevin <[email protected]>

Locate KubeRay deployment from DSCInitialization application namespace

59cadb4

tidy go mod files

0a3a96b

Signed-off-by: Kevin <[email protected]>

err undefined

f9e7c2b

Signed-off-by: Kevin <[email protected]>

add dsci to schema instead of odh

0404e99

Signed-off-by: Kevin <[email protected]>

add roles for creating nwp

97a685a

Signed-off-by: Kevin <[email protected]>

add rule for metrics project

ff169da

Signed-off-by: Kevin <[email protected]>

KPostOffice force-pushed the rhaoieng-5089 branch from c9d028b to b0c2116 Compare April 19, 2024 13:58

openshift-merge-robot removed the needs-rebase label Apr 19, 2024

add missing dsci import

53ee2f2

Signed-off-by: Kevin <[email protected]>

KPostOffice force-pushed the rhaoieng-5089 branch from b0c2116 to 53ee2f2 Compare April 19, 2024 14:10

openshift-ci bot assigned astefanutti Apr 19, 2024

openshift-ci bot added the lgtm label Apr 19, 2024

astefanutti merged commit 07098c0 into project-codeflare:main Apr 19, 2024
7 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add additional function which creates network policy #498

add additional function which creates network policy #498

KPostOffice commented Apr 3, 2024

etirelli left a comment

etirelli Apr 3, 2024

franciscojavierarceo Apr 18, 2024

KPostOffice Apr 19, 2024

openshift-ci bot commented Apr 3, 2024

openshift-ci bot commented Apr 3, 2024

astefanutti Apr 4, 2024

astefanutti Apr 4, 2024

astefanutti Apr 5, 2024 •

edited

Loading

zdtsw Apr 5, 2024

sutaakar Apr 15, 2024

zdtsw Apr 15, 2024

sutaakar commented Apr 15, 2024

sutaakar Apr 15, 2024 •

edited

Loading

astefanutti Apr 19, 2024

zdtsw Apr 19, 2024

astefanutti commented Apr 19, 2024

add additional function which creates network policy #498

add additional function which creates network policy #498

Conversation

KPostOffice commented Apr 3, 2024

Issue link

What changes have been made

Verification steps

Checks

etirelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 3, 2024

openshift-ci bot commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefanutti Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sutaakar commented Apr 15, 2024

sutaakar Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefanutti commented Apr 19, 2024

astefanutti Apr 5, 2024 •

edited

Loading

sutaakar Apr 15, 2024 •

edited

Loading