Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Add lookup for additional metadata from JS plugins #2099

Merged
merged 4 commits into from
Mar 11, 2021
Merged

Add lookup for additional metadata from JS plugins #2099

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9491df7
Add lookup for additional metadata from JS plugins
Mar 3, 2021
f460354
Avoid memory leak by using new context in a function call
Mar 3, 2021
064abf7
Use context.Cancel to clone context
Mar 5, 2021
0a36383
Extract out key/value extraction into a shared method
Mar 10, 2021
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
1 change: 1 addition & 0 deletions changelogs/unreleased/2099-xtreme-vikram-yadav
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add lookup for additional metadata from JS plugins
10 changes: 10 additions & 0 deletions pkg/plugin/javascript/dashboard_delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ func (d *DashboardDelete) Call(ctx context.Context, vm *goja.Runtime) func(c goj
// This will never error since &key is a pointer to a type.
_ = vm.ExportTo(obj, &key)

if len(c.Arguments) > 1 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to keep the metadata argument optional. I don't have strong opinions about this so let me know if anybody prefers this to always be passed with calls to the data store.

Copy link
Contributor

Choose a reason for hiding this comment

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

Javascript doesn't care one way or the other, so either way seems fine to me.

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 was thinking of a scenario where octant users decide to use the older plugins with new octant release. This will not introduce any breaking change in that case

var metadata map[string]interface{}
metadataObj := c.Argument(1).ToObject(vm)

_ = vm.ExportTo(metadataObj, &metadata)
for key, val := range metadata {
ctx = context.WithValue(ctx, key, val)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like the context passed in here has a long life and adding context here keep increasing the memory imprint of the context. While it doesn't impact the correctness of the functionality, its growth with every execution of this code is concerning to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example when i logged the context(I am passing datetime utc string from JS plugin in this example):

It started off with:
context : context.Background.WithCancel.WithValue(type log.key, val <not Stringer>).WithValue(type context.OctantContextKey, val <not Stringer>).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT)

After a while the context grew:

context : context.Background.WithCancel.WithValue(type log.key, val <not Stringer>).WithValue(type context.OctantContextKey, val <not Stringer>).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT).WithValue(type string, val Wed, 03 Mar 2021 02:02:39 GMT)^C2021-03-02T21:03:19.268-0500```

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because we're iterating over all the keys of the metadata, but only deleting the last key?

It was my impression that context.WithValue creates a copy rather than mutating the existing instance, so because, here, the copied ctx object goes out of scope at the end of this method, I would not have thought a memory leak was possible.

If it is a long-lived object, how does it behave when multiple calls specify different values for a given key?

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 should be using a different variable name than key here to avoid confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because i'm reusing the variable for the new context. I'm experimenting with a new variable for each function call to avoid this.

}
}

if err := d.storage.ObjectStore().Delete(ctx, key); err != nil {
panic(panicMessage(vm, err, ""))
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/plugin/javascript/dashboard_delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ func TestDashboardDelete_Call(t *testing.T) {
name: "in general",
ctorArgs: ctorArgs{
storage: func(ctx context.Context, ctrl *gomock.Controller) octant.Storage {
ctx = context.WithValue(ctx, "accessToken", "secret")
objectStore := fake2.NewMockStore(ctrl)

objectStore.EXPECT().
Delete(ctx, store.Key{
Namespace: "test",
Expand All @@ -61,7 +63,7 @@ func TestDashboardDelete_Call(t *testing.T) {
return storage
},
},
call: `dashClient.Delete({namespace:'test', apiVersion: 'v1', kind:'ReplicaSet', name: 'my-replica-set'})`,
call: `dashClient.Delete({namespace:'test', apiVersion: 'v1', kind:'ReplicaSet', name: 'my-replica-set'},{"accessToken": "secret"})`,
},
{
name: "delete fails",
Expand Down
10 changes: 10 additions & 0 deletions pkg/plugin/javascript/dashboard_get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ func (d *DashboardGet) Call(ctx context.Context, vm *goja.Runtime) func(c goja.F
// This will never error since &key is a pointer to a type.
_ = vm.ExportTo(obj, &key)

if len(c.Arguments) > 1 {
var metadata map[string]interface{}
metadataObj := c.Argument(1).ToObject(vm)

_ = vm.ExportTo(metadataObj, &metadata)
for key, val := range metadata {
ctx = context.WithValue(ctx, key, val)
}
}

u, err := d.storage.ObjectStore().Get(ctx, key)
if err != nil {
panic(panicMessage(vm, err, ""))
Expand Down
3 changes: 2 additions & 1 deletion pkg/plugin/javascript/dashboard_get_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func TestDashboardGet_Call(t *testing.T) {
name: "in general",
ctorArgs: ctorArgs{
storage: func(ctx context.Context, ctrl *gomock.Controller) octant.Storage {
ctx = context.WithValue(ctx, "accessToken", "secret")
objectStore := fake2.NewMockStore(ctrl)
objectStore.EXPECT().
Get(ctx, store.Key{
Expand All @@ -62,7 +63,7 @@ func TestDashboardGet_Call(t *testing.T) {
return storage
},
},
call: `dashClient.Get({namespace:'test', apiVersion: 'v1', kind:'Pod', name: 'pod'})`,
call: `dashClient.Get({namespace:'test', apiVersion: 'v1', kind:'Pod', name: 'pod'},{"accessToken": "secret"})`,
},
{
name: "delete fails",
Expand Down
10 changes: 10 additions & 0 deletions pkg/plugin/javascript/dashboard_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,16 @@ func (d *DashboardList) Call(ctx context.Context, vm *goja.Runtime) func(c goja.
panicMessage(vm, fmt.Errorf("key is invalid: %w", err), "")
}

if len(c.Arguments) > 1 {
var metadata map[string]interface{}
metadataObj := c.Argument(1).ToObject(vm)

_ = vm.ExportTo(metadataObj, &metadata)
wwitzel3 marked this conversation as resolved.
Show resolved Hide resolved
for key, val := range metadata {
ctx = context.WithValue(ctx, key, val)
}
}

u, _, err := d.storage.ObjectStore().List(ctx, key)
if err != nil {
panic(panicMessage(vm, err, ""))
Expand Down
3 changes: 2 additions & 1 deletion pkg/plugin/javascript/dashboard_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func TestDashboardList_Call(t *testing.T) {
name: "in general",
ctorArgs: ctorArgs{
storage: func(ctx context.Context, ctrl *gomock.Controller) octant.Storage {
ctx = context.WithValue(ctx, "accessToken", "secret")
objectStore := fake2.NewMockStore(ctrl)
objectStore.EXPECT().
List(ctx, store.Key{
Expand All @@ -61,7 +62,7 @@ func TestDashboardList_Call(t *testing.T) {
return storage
},
},
call: `dashClient.List({namespace:'test', apiVersion: 'v1', kind:'Pod'})`,
call: `dashClient.List({namespace:'test', apiVersion: 'v1', kind:'Pod'},{"accessToken": "secret"})`,
},
{
name: "list fails",
Expand Down
10 changes: 10 additions & 0 deletions pkg/plugin/javascript/dashboard_update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ func (d *DashboardUpdate) Call(ctx context.Context, vm *goja.Runtime) func(c goj
namespace := c.Argument(0).String()
update := c.Argument(1).String()

if len(c.Arguments) > 2 {
var metadata map[string]interface{}
metadataObj := c.Argument(2).ToObject(vm)

_ = vm.ExportTo(metadataObj, &metadata)
for key, val := range metadata {
ctx = context.WithValue(ctx, key, val)
}
}

results, err := d.storage.ObjectStore().CreateOrUpdateFromYAML(ctx, namespace, update)
if err != nil {
panic(panicMessage(vm, err, ""))
Expand Down
3 changes: 2 additions & 1 deletion pkg/plugin/javascript/dashboard_update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func TestDashboardUpdate_Call(t *testing.T) {
name: "in general",
ctorArgs: ctorArgs{
storage: func(ctx context.Context, ctrl *gomock.Controller) octant.Storage {
ctx = context.WithValue(ctx, "accessToken", "secret")
objectStore := fake2.NewMockStore(ctrl)
objectStore.EXPECT().
CreateOrUpdateFromYAML(ctx, "test", "create-yaml").
Expand All @@ -55,7 +56,7 @@ func TestDashboardUpdate_Call(t *testing.T) {
return storage
},
},
call: `dashClient.Update('test', 'create-yaml')`,
call: `dashClient.Update('test', 'create-yaml',{"accessToken": "secret"})`,
},
{
name: "create fails",
Expand Down