MGMT-16108: introduce the Resource server #12

danielerez · 2023-11-13T09:48:41Z

Added an initial implementation for the resource server. The server is serving the 'resourcePools' endpoint which fetches data from the search query API and converts the results into O2 objects.

In a nutshell, the mapping is done as follows:

Node object from search API --> O2 Resource object
Cluster object from search API --> O2 ResourcePool object

As some objects/properties don't have direct mapping, for now this PR does not include the following:

Handling for ResourceType objects
Mapping for these properties:
- ResourcePool.location
- ResourcePool.globalLocationID
- ResourcePool.description
- Resource.resourceTypeID

danielerez · 2023-11-13T09:48:53Z

/cc @jhernand
/cc @irinamihai

jhernand · 2023-11-13T10:15:59Z

internal/cmd/server/start_resource_server.go

+	"github.com/openshift-kni/oran-o2ims/internal/exit"
+	"github.com/openshift-kni/oran-o2ims/internal/logging"
+	"github.com/openshift-kni/oran-o2ims/internal/service"
+	searchmodel "github.com/stolostron/search-v2-api/graph/model"


Is there any chance that we can do without importing this? I ask because that means adding all the dependencies of that project, not just the the model. I had bad experiences with that, due to dependency conflicts. Can we maybe copy the required files?

yeah, makes sense. done.

jhernand · 2023-11-13T10:18:44Z

internal/cmd/server/start_resource_server.go

+		} `json:"filters"`
+		Relatedkinds []string `json:"relatedKinds"`
+	} `json:"input"`
+}


This is very similar to the SearchInput struct in the searchmodel package imported above. What is the reason for the additional struct?

Just a left over... removed.

jhernand · 2023-11-13T10:25:14Z

internal/service/resource_pool_fetcher.go

+	logger *slog.Logger,
+	cloudID, backendURL, backendToken string,
+	graphqlQuery string, graphqlVars *searchmodel.SearchInput) ResourcePoolFetcher {
+	return &resourcePoolFetcher{


Can we use the "builder" approach to create this object? Otherwise when this already long list of parameters grow it will be hard to understand.

sure, used the builder pattern instead.

jhernand · 2023-11-13T10:28:22Z

go.mod

@@ -4,20 +4,25 @@ go 1.21

 require (
 	github.com/PaesslerAG/jsonpath v0.1.1
+	github.com/golang/mock v1.6.0


We are already using go.uber.org/mock which is a maintaned fork of github.com/golang/mock please make sure to use it.

oh, missed it! changed.

jhernand · 2023-11-13T10:30:54Z

internal/cmd/server/start_resource_server.go

+// ResourceServerCommand contains the data and logic needed to run the `start
+// resource-server` command.
+type ResourceServerCommand struct {
+}


I see that you are going to use the logger in some methods of this type. In that case please add a logger *slog.Logger field here, initialize it in the run method and avoid passing it as parameter to methods. That is the reason for the existence of this type: to be able to do such things.

right, changed.

jhernand · 2023-11-13T10:31:50Z

internal/cmd/server/start_resource_server.go

+	ctx := cmd.Context()
+
+	// Get the dependencies from the context:
+	logger := internal.LoggerFromContext(ctx)


What I mention above about the logger is doing something like this:

c.logger = internal.LoggerFromContext(ctx)

jhernand · 2023-11-13T10:38:25Z

internal/service/resource_pool_fetcher.go

+
+func (r *resourcePoolFetcher) getSearchResults(ctx context.Context) (result []any, err error) {
+	tr := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},


Using InsecureSkipVerify: true is not acceptable for production environments. It is OK for now, but we need to address it. Please open a ticket explaining that we need to have command line options to configure the trusted certificate authorities.

Used transportWrapper as in DeploymentManagerCollectionHandler:

oran-o2ims/internal/service/deployment_manager_collection_handler.go

Line 123 in 9784fde

InsecureSkipVerify: true,

Will open a ticket to handle it in CLI.

jhernand · 2023-11-13T10:41:30Z

internal/service/resource_pool_fetcher.go

+
+	// Set GraphQL query and vars
+	request := graphql.NewRequest(r.graphqlQuery)
+	request.Var("input", *r.graphqlVars)


What is the advantage of using the graphql package here? Can't we do this with a plain HTTP client? My concern is that it looks like this graphql package will store all the result set in memory before returning control. That means that we can't process them in an streaming approach. Is there any way to do that with the graphql package? Otherwise it negates the advantages of using streams.

It mainly makes the code less clean without it. Can be removed.

jhernand · 2023-11-13T10:43:23Z

internal/service/resource_pool_object_handler.go

+	// Fetch resource pools
+	resourcePools, err := NewResourcePoolFetcher(
+		h.logger, h.cloudID, h.backendURL, h.backendToken, h.graphqlQuery, h.graphqlVars,
+	).FetchItems(ctx)


Is the resource pool fetcher stateless? Can we create one instance and reuse it for all calls to fetchObject?

Yeah, it is called only once per single item request. Moved it to Get func to make it clearer.

jhernand · 2023-11-13T10:53:56Z

internal/service/resource_object_mapper.go

+)
+
+//go:generate mockgen -source=resource_object_mapper.go -package=service -destination=resource_object_mapper_mock.go
+type ResourceObjectMapper interface {


There is already a Mapper type here:

oran-o2ims/internal/streaming/stream.go

Lines 145 to 146 in ccd39ba

// Mapper is a function that transofms one object into another.

type Mapper[F, T any] func(context.Context, F) (T, error)

It is a simple function, so I think we can use that directly instead of creating a new interface and a mock for it.

If using the generic type is unconfortable we can add an alias in the data package:

type Mapper = stream.Mapper[data.Object, data.Object]

As these are indeed simple function, just moves them to the fetcher.

jhernand · 2023-11-13T10:57:11Z

internal/service/resource_pool_fetcher.go

+// Map Cluster to an O2 ResourcePool object.
+func (r *resourcePoolFetcher) mapClusterItem(ctx context.Context,
+	from data.Object) (to data.Object, err error) {
+	to, err = NewResourcePoolObjectMapper(r.cloudID, r.resources).MapItem(ctx, from)


Can we create the resource pool mapper only once and reuse it?

Moved the logic.

jhernand · 2023-11-13T10:57:34Z

internal/service/resource_pool_fetcher.go

+// Map a Node to an O2 Resource object.
+func (rr *resourcePoolFetcher) mapNodeItem(ctx context.Context,
+	from data.Object) (to data.Object, err error) {
+	to, err = NewResourceObjectMapper().MapItem(ctx, from)


Can we create the resource object mapper only once and reuse it?

Moved the logic.

jhernand · 2023-11-13T11:04:42Z

internal/service/resource_pool_object_mapper.go

+}
+
+// Converts a specified data stream of O2 Resources (Nodes) to an array.
+func (r *resourcePoolObjectMapper) getResourceArray(ctx context.Context, items data.Stream, resourcePoolID string) (result []map[string]any) {


In general we should try to avoid things that return arrays, because they negate the value of using streams. But if we do then we can combine the Collect and Select methods. For example:

selector := func(ctx context.Context, item data.Object) (result bool, err error) { result = item["resourcePoolID"] == resourcePoolID return } result = streams.Collect(streams.Select(items, selector))

Added an initial implementation for the resource server. The server is serving the 'resourcePools' endpoint which fetches data from the search query API and converts the results into O2 objects. In a nutshell, the mapping is done as follows: * Node object from search API --> O2 Resource object * Cluster object from search API --> O2 ResourcePool object As some objects/properties don't have direct mapping, for now this PR does *not* include the following: * Handling for ResourceType objects * Mapping for these properties: * ResourcePool.location * ResourcePool.globalLocationID * ResourcePool.description * Resource.resourceTypeID

jhernand

Looks good to me, thanks @danielerez!

danielerez force-pushed the resource_server branch 4 times, most recently from 264bc97 to 166a477 Compare November 13, 2023 10:05

jhernand reviewed Nov 13, 2023

View reviewed changes

danielerez force-pushed the resource_server branch from 166a477 to 3aac8f9 Compare November 13, 2023 17:47

jhernand approved these changes Nov 14, 2023

View reviewed changes

jhernand merged commit 2c8fe5c into openshift-kni:main Nov 14, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-16108: introduce the Resource server #12

MGMT-16108: introduce the Resource server #12

danielerez commented Nov 13, 2023

danielerez commented Nov 13, 2023

jhernand Nov 13, 2023 •

edited

Loading

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand Nov 13, 2023

danielerez Nov 13, 2023

jhernand left a comment

	// Mapper is a function that transofms one object into another.
	type Mapper[F, T any] func(context.Context, F) (T, error)

MGMT-16108: introduce the Resource server #12

MGMT-16108: introduce the Resource server #12

Conversation

danielerez commented Nov 13, 2023

danielerez commented Nov 13, 2023

jhernand Nov 13, 2023 • 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

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

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

jhernand left a comment

Choose a reason for hiding this comment

jhernand Nov 13, 2023 •

edited

Loading