Issue170-AddingKappnav.subkindAnnotationToAppsodyNodejsStacks

juniartisu commented 4 years ago

Checklist:

[x] Read the Code of Conduct and Contributing Guidelines.
[x] Followed the commit message guidelines.
[x] Stack adheres to Appsody stack structure.

Modifying an existing stack:

[ ] Updated the stack version in stack.yaml

Contributing a new stack:

Describe how application dependencies are managed:
Explain how Appsody file watcher is utilized:
Describe other Appsody environment variables defined by the stack image:
Describe any limitations and known issues:

Related Issues:

CLAassistant commented 4 years ago

All committers have signed the CLA.

sam-github commented 4 years ago

I'm OK with this. Probably the stack versions should be bumped in the stack.yaml.

Also, be aware that I think the kAppNav demo assumes that the Nodejs subkind implies that dashboard is present.... but it is in fact only present on dev machines with appsody run, its not a production ("day 2") feature, and won't be present when apps are deployed to kube.

sam-github commented 4 years ago

^--- following on the comment above, what is that you assume to be common to all apps with this label? Simply the language of the runtime? loopback never has the dashboard, ever, even in dev runs.

Also, there is an experimental/nodejs-functions stack you likely want to add the annotation to. It, like loopback, never has the dashboard.

sam-github commented 4 years ago

.... and... last comment, sorry! I'm surprised its an annotation, not a label, because I understood that labels could be used to find all matching pods, but annotations can only be looked up once the pod is fetched from the kube API.

neeraj-laad commented 4 years ago

I think we need to understand what the implied with a deployment contains this annotation. We might also want to mention it in the README for each stack too, if it is relevant for user to be aware of.

cvignola commented 4 years ago

@sam-github @neeraj-laad The sole purpose of the kappnav.subkind annotation is for app navigator to include in the resource's action menu those actions defined in config map:

kappnav.actions.${kind}-${subkind}

The default actions for kappnav.subkind=Nodejs are defined in this config map:

apiVersion: v1
kind: ConfigMap
metadata:
  name: kappnav.actions.deployment-nodejs
data:
  url-actions: |
    [
      { 
        "name":"appmetrics-dash", 
        "text":"View App Metrics", 
        "text.nls":"action.url.node.js.metrics.text", 
        "description":"View Node.js App Metrics dashboard", 
        "description.nls":"action.url.node.js.metrics.desc", 
        "url-pattern":"http://${snippet.get_nodejs_route_host(${func.kubectlGet(Route,${resource.$.metadata.name},-n,${resource.$.metadata.namespace},-o,json)})}/appmetrics-dash",
        "open-window": "tab", 
        "menu-item": "true",
        "enablement-label": "kappnav.action.appmetrics"
      }
    ]

  snippets: |
    {
        "get_nodejs_route_host": "function getNodeJSRouteHost(nodeJSRoute) { 
            var nodeJSRouteJSON = JSON.parse(nodeJSRoute);
            var host = nodeJSRouteJSON.spec.host;
            return host;
        }"
    }

cvignola commented 4 years ago

@sam-github @neeraj-laad Note that the "appmetrics-dash" action (previous comment) is not displayed in the App Navigator action menu unless the resource also specifies label: 'kappnav.action.appmetrics' - note from the action definition:

"enablement-label": "kappnav.action.appmetrics"

This could alternatively have been specified as an annotation. i.e.

"enablement-annotation": "kappnav.action.appmetrics"

There was no particular reason I can remember why we chose to use a label for enablement. App Navigator supports both labels and annotations for action enablement.

It would be cool if Appsody knew when a Node.js project included appmetrics and set that label automatically.

cvignola commented 4 years ago

@sam-github @neeraj-laad In App Nav v0.8 there is now a way for appsody to own and deploy its own action config maps if it wanted to. I wonder if these runtime specific actions should be owned/delivered by appsody? We'd be happy to work with you guys on that if you're interested.

cvignola commented 4 years ago

@sam-github @neeraj-laad Sounds like Node.js and Express are the only stacks for which the user would ever use App Metrics dash. Is that correct?

sam-github commented 4 years ago

@cvignola appmetrics-dash is enabled only when appsody run is done, its deliberately not used in production by those stacks, so should never be present if an app built with the nodejs or nodejs-express stack is deployed into Kube. Its a kindof light-weight webapp equivalent to the eclipse health centre UI for Java, appropriate for local use.

Thus my question about what it means.

Does calling it a "Nodejs" kind imply that the actions in the ConfigMap will actually be supported?

Or are ConfigMap defined actions intended to be a superset of actions that may be supported on containers of that kind, but aren't necessarily supported on any specific container?

cvignola commented 4 years ago

@sam-github

Ok, so appmetrics is intended primarily for local dev purposes; users are not encouraged to use it in a container on Kubernetes. Therefore it would not make sense for us to expose a link to it in AppNavigator. Ever. Ok.

The subkind annotation is used to assign actions to a resource for access through AppNavigator, based on criteria more specific than just its Kubernetes kind (e.g. Deployment or Pod). (E.g. we use subkind=Liberty to expose Liberty-specific actions on Liberty Deployments, like 'View Liberty Kibana Dashboard').

So the question for Node.js is, are there any actions that are unique to Node.js applications that we would want to expose to the Administrator? E.g. heap dump ?

If the answer is 'no', there are administrator actions that are unique to Node.js, then we do not need to annotate any Node.js resources with kappnav.subkind annotation.

sam-github commented 4 years ago

clarification: "appmetrics" is an APM, it is present in production

"appmetrics-dashboard" is an optional dashboard, that hooks itself into the user's web app and is present (with the appsody stacks) only in local development.

But anyway, yes, the config map for -dashboard doesn't really apply.

Node.js has only 1 management function available by default: it will open a debug port (reachable by the chrome inspector, for example, or any debugger client that supports the protocol). The only way to turn it on is to send SIGUSR1.

https://github.com/appsody/stacks/pull/722 exposes a second management function, a diagnostic report in JSON to stdout that will show up in the Kibana/LogDNA/whatever log ingestion. If the ConfigMap can exec a shell command on a target container, it could be implemented, even without a Node.js specific operator (like https://github.com/sam-github/operator-nodejs), by sending SIGUSR2 to node.js.

Note that if node is NOT passed CLI flags to enable the diagnostic dump SIGUSR2, then SIGUSR2 will do the default... terminate the process, so some kind of label/annotation has to be present to indicate the feature is supported.

I think this would be a useful feature for appsody stacks, or odov2 stacks, but the stacks would need an owner with bandwidth to complete the feature.

I might be able to finish that draft PR, its trivial, but not until the features it depends on arrive in 12.x from Node.js master.

cvignola commented 4 years ago

@sam-github Thanks for the response. It provides much clarity. And yes, I meant appmetrics-dash, specifically. For the time being, I see no reason for Node.js stacks using the kappnav.subkind annotation, so I think we should discard this PR and cancel the associated issue.

Going forward, it is definitely possible to expose the diagnostic report as an action in the App Navigator menu for a Node.js Deployment. The existing App Navigator action definition already allows for command actions:

    cmd-actions: 
      [
        {  
          "name": "<name>", 
          "text": "<text for menu item>", 
          "description": "<brief description>", 
          "image": "<image-name:tag>",
          "cmd-pattern": "<substitution-pattern>",
           "menu-item": "true",
           "requires-input": "<input-name>"
        } 
      ],

Moreover, new support in AppNavigator for mapping actions to resources ("Kind-Action-Mapping") makes it possible for operators to dynamically create and expose actions to App Navigator. This can be done by kind, by subkind, by instance, by ownerRef and other ways - it's quite powerful.

And even now, we are working on an extension to the operator pattern that couples day 2 operators as part of the operator definition. We are defining this day 2 operations in such as way as to enable them to be dynamically discovered and placed on App Navigator menus in the right places.

So if Appsody ever wants to expose day 2 operations (aka "actions") for the app components it creates, the tools to do so will exist. This way, Appsody can control its own destiny for such things, without dependency on App Navigator. At such time it might be useful to annotate Node.js deployments with the subkind annotation: since App Navigator displays the annotation, users might find it to be helpful information.

@juniartisu FYI

juniartisu commented 4 years ago

Thank you @cvignola , @sam-github and @neeraj-laad I will close this PR as the changes is not needed at this moment.

appsody / stacks