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

Context Sharing POC #1855

Merged
merged 3 commits into from
Oct 20, 2023
Merged

Context Sharing POC #1855

merged 3 commits into from
Oct 20, 2023

Conversation

dsimansk
Copy link
Contributor

Description

This's a bit simplified approach from original Feature Track proposal. I wonder if we should simply start with a few well known keys, that can be consumed in kn core + plugins

Changes

Reference

Fixes #

Release Note

TBD

/cc @rhuss

@knative-prow knative-prow bot requested a review from rhuss August 22, 2023 14:16
@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2023
Comment on lines 37 to 41
mockData := `
{
"service":"hello",
"namespace":"default"
}
Copy link
Contributor Author

@dsimansk dsimansk Aug 22, 2023

Choose a reason for hiding this comment

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

This represents very minimal shared context between every Consumer and Producer to read/write. E.g. in a seems floating value changed on latest usage of kn or plugins.

That's a looser contract then with predefined manifest etc.

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 could be further split into plugin specific sub-context and shared sub-context. Where plugin specific is only accessed by it's owner. In addition with read/write permission field maybe?

return ctxManager, nil
}

func (c *ContextDataManager) Find(key string) string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then defining the API as a set of access methods to retrieve and store the well-known values.

@rhuss
Copy link
Contributor

rhuss commented Aug 25, 2023

@dsimansk thanks for this initial work. I wouldn't mind to restrict ourselves to a set of fixed keys, but I'm not sure what this will buy us. Of course, the core commands themselves should check the Context for any mandatory argument that the user has not provided. These are are well known keys like "service" or "namespace". However, I would not prohibit that a plugin could add any key to the context manager, as this is not harmful as the core commands never will lookup this. However other plugins might make use of it, so that e.g. the function plugin could communicate with the event plugin.

The tricky part really is, how external, non-inlined plugins can add to the context. For inline plugins, this is trivial as they just can add to a global map. For external plugins. that would need to an external call. For optimization, the core should know which plugin supports which context key, therefor the idea of the manifest that is obtained once during startup by calling every plugin that is reachable. This might be dangerous if plugins are looked up from the path and there are some dangling plugins or bogus plugins that would be called. So not really sure if this is the proper way. Alternatively we could keep a central registry remotely that contains this information in a plain file. We could fetch this once from a central place (e.g. GitHub) or fallback to a default one that we have compile into kn. This information ("which plugin supports which keys") should be quite static and don't changes very often.

@rhuss
Copy link
Contributor

rhuss commented Aug 25, 2023

on the other hand, we also could reconsider to implement the "sharing via a local file" first, too. Not sure how far we should go.

@dsimansk
Copy link
Contributor Author

@rhuss updated with a building blocks to Manifest structures and retrieving those from inlined and external plugins. It's starting to be messy, be careful. :)

Copy link
Contributor Author

@dsimansk dsimansk left a comment

Choose a reason for hiding this comment

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

I'm not quiet sure how to allow external plugins to store any context data though. Other than by directly writting to context.json file.

docs/cmd/kn.md Outdated Show resolved Hide resolved

// TODO: We should cautiously execute external binaries
// fetchExternalManifest returns Manifest from external plugin by exec `$plugin manifest get`
func fetchExternalManifest(p Plugin) *Manifest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a bit tricky part. Executing binary half blind if it's desired one even?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Preview of manifest flag: knative-extensions/kn-plugin-service-log@c214f72

That can be a standalone cmd. I.e. kn service log manifest, but it kind of makes single purpose plugins grow the list of cmds. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, the tricky part is not blindly calling each plugin in the path somehow, if possible. Of course if a plugin is in the path, it can be called anyway (maybe by accident), but chances are higher that a malicious plugin is called when it is called by itself. A simple solution could be to keep track of which plugins have been called explicitly and only can them when picking up all manifests (which might be hard to maintain, though ...)

Comment on lines 106 to 110
c, _ := plugin.NewContextManager()
serviceName = c.GetDefault()["service"]
if serviceName == "" {
return errors.New("'service describe' requires the service name given as single argument")
}
Copy link
Contributor Author

@dsimansk dsimansk Sep 1, 2023

Choose a reason for hiding this comment

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

@rhuss what would you say about using Golang's buily-in context.Context since map[string]string is our context now anyway? We can pass it through ExecuteContext() function that's already part of Cobra lib.

Or shoud we be a bit more bold here and inject the params directly into args & flags?

@rhuss
Copy link
Contributor

rhuss commented Sep 1, 2023

Thanks! I hope i can review it today, if not, Iwill do it early next week.

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Attention: 90 lines in your changes are missing coverage. Please review.

Comparison is base (1719498) 79.62% compared to head (7e5bef5) 79.20%.
Report is 6 commits behind head on main.

❗ Current head 7e5bef5 differs from pull request most recent head e2d3a49. Consider uploading reports for the commit e2d3a49 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1855      +/-   ##
==========================================
- Coverage   79.62%   79.20%   -0.43%     
==========================================
  Files         179      180       +1     
  Lines       13927    14075     +148     
==========================================
+ Hits        11090    11148      +58     
- Misses       2070     2155      +85     
- Partials      767      772       +5     
Files Coverage Δ
pkg/kn/config/testing.go 100.00% <100.00%> (ø)
pkg/kn/plugin/manager.go 84.30% <100.00%> (+0.11%) ⬆️
pkg/kn/config/config.go 57.71% <0.00%> (-0.79%) ⬇️
pkg/kn/commands/service/describe.go 70.07% <50.00%> (-1.35%) ⬇️
cmd/kn/main.go 76.69% <15.00%> (-11.14%) ⬇️
pkg/kn/plugin/context_sharing.go 43.96% <43.96%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2023
@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 21, 2023
@dsimansk dsimansk force-pushed the pr/context-sharing branch 2 times, most recently from 63dded1 to c8d0a42 Compare September 21, 2023 13:21
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2023
@dsimansk
Copy link
Contributor Author

@rhuss I'll be adding unit test, but it's ready for a review and feedback I believe.

@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 24, 2023
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thank, I did a first round of review. Beside the usual bike shedding some more substantial comments inline:

  • Consider cleaning the cache, too, if some external plugin is not in the list anymore.
  • Only do the lookup dance and external call to a plugin if really needed. E.g. it should be lazily done, e.g. when the service describe does not find a service name provided, instead of doing it always. That would keep the current behavior and only fallback to a context lookup when needed.


defer func(ctxManager *pluginpkg.ContextDataManager) {
if err := ctxManager.WriteCache(); err != nil {
println("error during write")
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message should contain more context (pun intended :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

also, you propbably want to write this to standard error.

cmd/kn/main.go Outdated Show resolved Hide resolved
@@ -182,7 +190,7 @@ func describe(w io.Writer, service *servingv1.Service, revisions []*revisionDesc
return nil
}

// Write out main service information. Use colors for major items.
// WriteCache out main service information. Use colors for major items.
Copy link
Contributor

Choose a reason for hiding this comment

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

seems to be a wrong search/replace thingy. Same as in the comment below.

@@ -117,7 +117,7 @@ func setupConfig(t *testing.T, configContent string) (string, func()) {
// Save old args
backupArgs := os.Args

// Write out a temporary configContent file
// WriteCache out a temporary configContent file
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong comment

Path string `json:"path,omitempty"`

// Plugin declares its own manifest to be included in Context Sharing feature
HasManifest bool `json:"hasManifest"`
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 flag really necessary or wouldn't it be just good enough to have the slices below, which then could be also be empty to indicate that the plugin does not support context data sharing.

func (c *ContextDataManager) FetchContextData(pluginManager *Manager) context.Context {
ctx := context.Background()
for _, p := range pluginManager.GetInternalPlugins() {
if p.Name() == "kn func" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is just a test case for the prototype and will be generalized later on ?

for _, plugin := range pluginManager.GetInternalPlugins() {
manifest := &Manifest{}
// For the integrity build the same name format as external plugins
pluginName := "kn-" + strings.Join(plugin.CommandParts(), "-")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember whether we haven't added some more sophisticated name handling (e.g. like converting - in command parts to _ before joining them), but I think we should do the same handling here.

// Add manifest to map
c.Manifests[pluginName] = *manifest

if manifest.HasManifest {
Copy link
Contributor

Choose a reason for hiding this comment

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

as said above, maybe just check if there are keys produced by a plugin, just to get rid of this boolean flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could even just call populateDataKeys as this is a noop if the slices for the keys are empty anyways.

if err != nil {
return err
}
for _, plugin := range plugins {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be also cool, to remove plugins from the cache that are not listed anymore. So checking which keys of the c.Manifests[..] have been touched and remove at the end every entry with a key that is neither an internal or external plugin.

pkg/kn/plugin/manager.go Show resolved Hide resolved
@dsimansk dsimansk changed the title WIP: Context Sharing POC Context Sharing POC Oct 20, 2023
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2023
@dsimansk
Copy link
Contributor Author

A corresponding client-pkg with types at least: knative/client-pkg#127.

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

This goes definitely in the write direction. See some comments inline (and the other comments I did before).


if config.GlobalConfig.ContextSharing() {
if pwm, ok := plugin.(pluginpkg.PluginWithManifest); ok {
data, _ := ctxManager.FetchContextData()
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably should not ignore the error here. Either remove the error part of the FectContextData() signature or return the error (I hope, the second arg is an error ;-), nevertheless ignore this here has some smell)

return errors.New("'service describe' requires the service name given as single argument")
}
} else {
serviceName = args[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, but I have the feeling that his access pattern will be repeated in several commands (all that require a service name), so maybe we could extract this handling in a utility method. Usually, I wouldn't opt for this, but this looks complex enough to avoid a C&P approach.

@rhuss
Copy link
Contributor

rhuss commented Oct 20, 2023

Let's get it merged and iterate from there on. Not sure how to preserve the unresolved comments for the next update. But I think they are still kind of valid.

/approve
/lgtm
/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2023
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2023
@knative-prow
Copy link

knative-prow bot commented Oct 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, rhuss

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:

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

@dsimansk
Copy link
Contributor Author

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2023
@knative-prow knative-prow bot merged commit b658574 into knative:main Oct 20, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants