dnn-fn #70

henderiw · 2023-03-30T06:15:55Z

DNN-fn operates on DNN resource and can request IP Allocations.

linux-foundation-easycla · 2023-04-19T15:24:57Z

The committers listed above are authorized under a signed CLA.

✅ login: henderiw / name: Wim Henderickx (03af1ec, ee398b0, 4cc18e1, bd78db3, 8b9dd6e, f07bf9a, ae5148b, aa17894, d3c6319, f8a320a)
✅ login: kispaljr / name: Istvan Kispal (ae5148b, aa17894, d3c6319, f8a320a, 581f821, ec457be, 685d3cb, 15ad53c, ca6a30f)

ganchandrasekaran · 2023-04-20T04:05:14Z

krm-functions/dnn-fn/main.go

+			Watch: map[corev1.ObjectReference]kptcondsdk.WatchCallbackFn{
+				{
+					APIVersion: infrav1alpha1.GroupVersion.Identifier(),
+					Kind:       "ClusterContext",


Suggestion: Instead of embeded "ClusterContext" string you can use..

Kind: reflect.TypeOf(infrav1alpha1.ClusterContext{}).Name(),

you are absolutely right. the string literal was meant to be temporary, I just forgot to put a TODO next to it :)
Since ClusterContext is a proper kubebuilder-style K8s API object I am planning to use the usual "Scheme" method to get the name of it.

Fixed in 3395624

ganchandrasekaran · 2023-04-20T04:09:27Z

krm-functions/lib/parser/parser.go

@@ -17,11 +17,218 @@ limitations under the License.
 package parser


All the lib files are already part of seperate PR, these needs to be removed from this PR.

that's actually a very good topic. What I did is that I merged that PR's branch into mine, and this is how the files from the sdk PR ended up in my branch.
I thought this is the way to base your PR to another one. Do we have a better method on basing PR on other PRs? Was it already discussed, or is it written somewhere?

Fixed in 3395624

ganchandrasekaran · 2023-04-20T04:09:36Z

krm-functions/lib/interface/v1alpha1/interface.go

@@ -21,12 +21,7 @@ import (



All the lib files are already part of seperate PR, these needs to be removed from this PR.

Fixed in 3395624

ganchandrasekaran · 2023-04-20T04:09:53Z

krm-functions/lib/ipallocation/v1alpha1/ipallocation.go

@@ -0,0 +1,436 @@
+package v1alpha1


All the lib files are already part of seperate PR, these needs to be removed from this PR.

Fixed in 3395624

ganchandrasekaran · 2023-04-20T04:10:42Z

krm-functions/lib/condkptsdk/design.md

@@ -0,0 +1,180 @@
+# Conditional kpt fn sdk


All the lib files are already part of seperate PR, these needs to be removed from this PR.

Fixed in 3395624

ganchandrasekaran · 2023-04-20T05:38:17Z

krm-functions/dnn-fn/main.go

+				},
+				Selector: &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"nephio.org/site": f.site,


req to use SiteAnnotationKey instead of embeded string

oops, you are right of course :)

Fixed in 3395624

ganchandrasekaran · 2023-04-20T12:21:19Z

krm-functions/dnn-fn/main.go

+	if err != nil {
+		return err
+	}
+	f.site = *cluster.Spec.SiteCode


Here if the siteCode field is not available on clustercontext yaml then following occurs:
panic: runtime error: invalid memory address or nil pointer dereference

it would be better to handle it here..

Fixed in 3395624

henderiw

why are we using the runtime scheme if we can access the schema direct. Avoiding it removes some code

henderiw · 2023-04-25T07:10:36Z

krm-functions/dnn-fn/main.go

+
+// var _ fn.Runner = &DnnFn{}
+
+var theScheme *runtime.Scheme = runtime.NewScheme()


why do we add this here? we can access the schema direct w/o going to runtime scheme. avoids some code

you are right, of course. in this case using runtime is way too complicated to obtain the Kind of a go type.

These helper functions are just experimental code that I hoped can be the basis for future convenience methods (like FilterByType in lib.go). I am happy to remove it, if it is confusing, or we can keep it, so that it inspires others to create other convenient generic helper functions.

henderiw · 2023-04-25T07:11:11Z

krm-functions/dnn-fn/main.go

+
+// GetKindOrPanic returns with the Kind of a Kubernetes API resource type `T`.
+// Panics if `T` is not registered in the `theScheme`
+func GetKindOrPanic[T any, PT interface {


can be avoided if we don't go to the scheme

henderiw · 2023-04-25T07:18:28Z

krm-functions/dnn-fn/main.go

+		if found {
+			pools = append(pools, DnnPoolStatus{Name: poolName, Ip: ipalloc.Status})
+		} else {
+			f.rl.Results.Warningf("found an owned IPAllocation with a suspicious name: %v", ipalloc.Name)


is it not better to log rather than adding to resourceList? I would like to avoid adding to resourceList.

henderiw · 2023-04-25T07:21:07Z

Need to discuss the test strategy if adding all these files is a good approach or not

kispaljr · 2023-04-26T15:53:29Z

why are we using the runtime scheme if we can access the schema direct. Avoiding it removes some code

you are right, of course. in this case using runtime is way too complicated to obtain the Kind of a go type.

These helper functions are just experimental code that I hoped can be the basis for future convenience methods (like FilterByType in lib.go). I am happy to remove it, if it is confusing, or we can keep it, so that it inspires others to create other convenient generic helper functions.

kispaljr · 2023-04-26T15:54:29Z

Need to discuss the test strategy if adding all these files is a good approach or not

Based on our discussion yesterday I've created a PR for moving my test helper functions into lib/: #134

henderiw · 2023-04-26T17:19:52Z

@kispaljr can you look at the build issues ?

gvbalaji · 2023-05-05T23:47:40Z

Thanks @henderiw . A rebase needed for this PR.

kispaljr · 2023-05-08T12:51:23Z

Tests will pass only after #170 is merged

henderiw · 2023-05-08T21:53:11Z

krm-functions/dnn-fn/lib.go

+
+var theScheme *runtime.Scheme = runtime.NewScheme()
+
+func init() {


do you still need the schema stuff?

I don't use it to get the Kind of ClusterContext and others, but I use it in functions like FilterByType[IPAllocation], where I think it's quite elegant (however I admit that it is subjective)

Here is an example how I did it in the interface-fn. You basically know the types based on GVK as it is 1:1 mapping

ipallocs := objs.Where(fn.IsGroupVersionKind(ipamv1alpha1.IPAllocationGroupVersionKind))
for _, ipalloc := range ipallocs {
if ipalloc.GetName() == forObj.GetName() {
alloc, err := kubeobject.NewFromKubeObjectipamv1alpha1.IPAllocation
if err != nil {
return nil, err
}
allocGoStruct, err := alloc.GetGoStruct()
if err != nil {
return nil, err
}
itfce.Status.IPAllocationStatus = &allocGoStruct.Status
}
}

Yes, exactly. FilterByType[IPAllocation] implements the exact same functionality as the first 12 lines of your code. Since this code is copy-pasted into all of our KRM functions, I thought it is a prime candidate to be factored out into a utility function.
Does this cause a problem?

Btw with latest lib, the above doesn't work instead it has to be:
objs.Where(fn.IsGroupVersionKind(schema.GroupVersionKind(ipamv1alpha1.IPAllocationGroupVersionKind)))

gvbalaji · 2023-05-08T22:42:30Z

krm-functions/dnn-fn/.gitignore

@@ -0,0 +1 @@
+_actual_output.yaml


Should this file be ignored? Plus new line

newline added (will be pushed in a sec)
_actual_output.yaml files are a byproduct of running the tests, but they are not needed in the repo

gvbalaji · 2023-05-08T22:49:50Z

krm-functions/dnn-fn/Dockerfile

+
+FROM gcr.io/distroless/static:latest
+COPY --from=0 /usr/local/bin/function /usr/local/bin/function
+ENTRYPOINT ["function"]


gvbalaji · 2023-05-08T23:37:31Z

krm-functions/dnn-fn/testdata/golden/garbage_collection/ipallocations.yaml

+    matchLabels:
+      nephio.org/site: edge1
+  networkInstance: {}
+  prefixLength: 8


fixing it
we should consider adding a presubmit test that automatically checks newline-at-the-end-of-file :)

henderiw · 2023-05-09T21:59:22Z

krm-functions/dnn-fn/main.go

+	myFn := DnnFn{rl: rl}
+
+	// get ClusterContext
+	myFn.ClusterContext, err = GetSingleton[infrav1alpha1.ClusterContext](rl.Items)


we want to avoid doing this. We should use the watch to handle this. The watch adds functionality you will not get if we use it this way.

This was meant to be a temporary solution until the singleton functionality we talked about will be part of the SDK. Is it already available? I will update this to the SDK if it is available.

I looked, and didn't find, so I am reverting back to the old Watch callback-based version

we can add singleton to the sdk. it would be better to have it there

reverted back from calling the GetSingleton[ClusterContext]() utility function to manually checking in ClusterContextCallback and PopulateFnCallback the validity of ClusterContexts

henderiw · 2023-05-09T22:06:42Z

krm-functions/dnn-fn/lib.go

+
+var theScheme *runtime.Scheme = runtime.NewScheme()
+
+func init() {


Here is an example how I did it in the interface-fn. You basically know the types based on GVK as it is 1:1 mapping

ipallocs := objs.Where(fn.IsGroupVersionKind(ipamv1alpha1.IPAllocationGroupVersionKind))
for _, ipalloc := range ipallocs {
if ipalloc.GetName() == forObj.GetName() {
alloc, err := kubeobject.NewFromKubeObjectipamv1alpha1.IPAllocation
if err != nil {
return nil, err
}
allocGoStruct, err := alloc.GetGoStruct()
if err != nil {
return nil, err
}
itfce.Status.IPAllocationStatus = &allocGoStruct.Status
}
}

kispaljr · 2023-05-10T09:44:03Z

GitHub working strange to me, somehow I have trouble replying to this, so I will just copy-paste:

Here is an example how I did it in the interface-fn. You basically know the types based on GVK as it is 1:1 mapping

ipallocs := objs.Where(fn.IsGroupVersionKind(ipamv1alpha1.IPAllocationGroupVersionKind))
for _, ipalloc := range ipallocs {
if ipalloc.GetName() == forObj.GetName() {
alloc, err := kubeobject.NewFromKubeObject[ipamv1alpha1.IPAllocation](https://github.com/nephio-project/nephio/pull/ipalloc)
if err != nil {
return nil, err
}
allocGoStruct, err := alloc.GetGoStruct()
if err != nil {
return nil, err
}
itfce.Status.IPAllocationStatus = &allocGoStruct.Status
}
}

Yes, exactly. FilterByType[IPAllocation] implements the exact same functionality as the first 12 lines of your code. Since this code is copy-pasted into all of our KRM functions, I thought it is a prime candidate to be factored out into a utility function.
Does this cause a problem?

…ests depend on.

kispaljr · 2023-05-12T10:28:00Z

/retest

ganchandrasekaran · 2023-05-12T11:22:12Z

krm-functions/dnn-fn/README.md

+
+<!--mdtogo:Long-->
+
+## Usage


Just a suggestion, for the sake of alignment what do you think about changing the content of usage sub-section?

https://github.com/nephio-project/nephio/tree/main/krm-functions/interface-fn#usage
https://github.com/nephio-project/nephio/blob/main/krm-functions/ipam-fn/README.md
https://github.com/nephio-project/nephio/blob/main/krm-functions/nad-fn/README.md#usage

This section can be named as Implementation details, perhaps?

henderiw · 2023-05-12T12:01:26Z

/approve

nephio-prow · 2023-05-12T12:01:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: henderiw

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [henderiw]

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

ganchandrasekaran · 2023-05-12T12:17:42Z

/lgtm

henderiw added this to the sprint1 milestone Mar 30, 2023

henderiw linked an issue Mar 30, 2023 that may be closed by this pull request

Develop DNN for package specialization #63

Closed

3 tasks

henderiw added the area/package-specialization SIG Automation Package Specialization Subproject label Mar 30, 2023

henderiw assigned kispaljr Mar 30, 2023

gvbalaji modified the milestones: sprint1, sprint2 Apr 11, 2023

nephio-prow bot added the approved label Apr 19, 2023

ganchandrasekaran reviewed Apr 20, 2023

View reviewed changes

kispaljr force-pushed the dnn-fn branch from ca6a30f to 4c66cf4 Compare April 24, 2023 15:18

gvbalaji modified the milestones: sprint2, sprint3 Apr 25, 2023

henderiw commented Apr 25, 2023

View reviewed changes

nephio-prow bot removed the approved label Apr 27, 2023

kispaljr force-pushed the dnn-fn branch from 82d5b1d to 0554e7d Compare April 28, 2023 14:43

kispaljr mentioned this pull request May 2, 2023

interface-fn #69

Merged

kispaljr force-pushed the dnn-fn branch from 416cb2b to b2a60b5 Compare May 8, 2023 12:13

henderiw commented May 8, 2023

View reviewed changes

gvbalaji reviewed May 8, 2023

View reviewed changes

gvbalaji modified the milestones: sprint3, sprint4 May 9, 2023

henderiw commented May 9, 2023

View reviewed changes

kispaljr and others added 3 commits May 11, 2023 20:34

New helpers are added to krm-functions/lib that dnn-fn and pipeline-t…

0d3c208

…ests depend on.

Add tests to lists.go functions

b07b891

Add implementation of the DNN KRM function

d1e7660

kispaljr force-pushed the dnn-fn branch from 137d82a to d1e7660 Compare May 12, 2023 10:03

kispaljr added 3 commits May 12, 2023 12:47

Update go.mod to include latest changes in krm-functions/lib

f792dc1

Reorganize dnn-fn tests

654f434

Merge remote-tracking branch 'remotes/nephio/main' into dnn-fn

9429708

kispaljr requested a review from aakashchan May 12, 2023 11:15

ganchandrasekaran reviewed May 12, 2023

View reviewed changes

nephio-prow bot added the approved label May 12, 2023

dnn-fn: minor adjustment of README.md

bebdc46

nephio-prow bot assigned ganchandrasekaran May 12, 2023

nephio-prow bot added the lgtm label May 12, 2023

nephio-prow bot merged commit a81cdef into nephio-project:main May 12, 2023
5 checks passed

		@@ -17,11 +17,218 @@ limitations under the License.
		package parser


		// var _ fn.Runner = &DnnFn{}

		var theScheme *runtime.Scheme = runtime.NewScheme()


		var theScheme *runtime.Scheme = runtime.NewScheme()

		func init() {

		@@ -0,0 +1 @@
		_actual_output.yaml


		<!--mdtogo:Long-->

		## Usage

dnn-fn #70

dnn-fn #70

Conversation

henderiw commented Mar 30, 2023

linux-foundation-easycla bot commented Apr 19, 2023 • edited

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

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

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

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

Choose a reason for hiding this comment

henderiw 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

Choose a reason for hiding this comment

henderiw commented Apr 25, 2023

kispaljr commented Apr 26, 2023

kispaljr commented Apr 26, 2023

henderiw commented Apr 26, 2023

gvbalaji commented May 5, 2023

kispaljr commented May 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kispaljr May 10, 2023 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kispaljr May 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kispaljr commented May 10, 2023 • edited

kispaljr commented May 12, 2023

ganchandrasekaran May 12, 2023 • edited

Choose a reason for hiding this comment

henderiw commented May 12, 2023

nephio-prow bot commented May 12, 2023

ganchandrasekaran commented May 12, 2023

linux-foundation-easycla bot commented Apr 19, 2023 •

edited

kispaljr May 10, 2023 •

edited

kispaljr May 10, 2023 •

edited

kispaljr commented May 10, 2023 •

edited

ganchandrasekaran May 12, 2023 •

edited