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

KEP-22: Add initial design on diagnostics bundles #1310

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

mpereira
Copy link
Member

Fixes #1152.

@mpereira
Copy link
Member Author

The operator developer experience section needs to be fleshed out. It is planned that @gerred will take a look at this whenever he gets a chance.

@mpereira
Copy link
Member Author

mpereira commented Jan 24, 2020

(Pinging @marccampbell as well)

@gerred gerred added this to the 0.11.0 milestone Feb 11, 2020
@gerred
Copy link
Member

gerred commented Feb 11, 2020

Added a lot of bundle-level information here.


Also, may specs may include an `objectRef`. It ALWAYS has the following keys:

- **kind**: The Kubernetes Kind referenced. For example, this may be a
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a CR as well? I would assume so, but in that case do we need a GVK? Or is a GK enough?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm.. Not sure it would make sense to allow CRs here. But in that case we need to specify what kinds we exactly allow here. Probably something that boils down to something runnable?

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's a convention we can follow. You could potentially have a CR that is representative of a set of pods - see the metacontroller DaemonJob example. Additionally, could you just eventually reference a KUDO Instance that is created as part of an operator (see: Flink) and get ALL PODS inside of Flink?

There is some convention here around selectors and labels inside of Deployment, Statefulset, and Pod. Though, service has that too. Could you select ALL PODS that are selected by that Service? It seems weird at first, but it could be very useful (give me the traffic information for all pods that are currently being load balanced by this service).

Copy link
Member

Choose a reason for hiding this comment

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

What's a CR?

Copy link
Contributor

Choose a reason for hiding this comment

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

CR stands here for "Custom Resource"

Comment on lines 396 to 363
- **taskRef**: Name of the task to run. **NOTE**: We MAY need a Pause and Resume
task to be able to copy files and run commands during the running of a task.
Otherwise, we may want to make this an arbitrary job.
Copy link
Member

Choose a reason for hiding this comment

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

Could we maybe utilize the PipeTask for this?

Copy link
Member

Choose a reason for hiding this comment

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

Potentially. There were a few semantics I was worried about and I think that'll just come up in implementation though. Mostly around - what if we're piping around VERY large files? Debugging training models, etc. Maybe that's then a "this app was architected poorly" thing and we should have file limits.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably have file limits in any case ;) But they can probably be bigger than the 1MB of the PipeTask, yes. Also it might be useful to copy more than one file out of a task. I think the bundle.resources.Task type seems to be a feature that might be lower on our prio list for the implementation

Copy link
Member

Choose a reason for hiding this comment

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

agreed. what I was looking to solve here was preventing mega-pods filled with unrelated things. for example, seems silly to have to have nodetool sitting inside of your cassandra pods, but maybe this is false and nobody actually cares. But you may want to debug with dtrace, while not having to ship dtrace along, as a perhaps better example.

reminds me, we will need an optional container field for multi-container pods and exec.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe it's as simple as resources get created by this are deleted at the end of a diagnostics run, enabling you to have the subsequent copy and command bundle tasks that you need 🤔

Copy link
Member

Choose a reason for hiding this comment

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

yeah, this is probably low priority for now compared to Copy and Command.

Copy link
Member

Choose a reason for hiding this comment

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

Having resources deleted at the end of diagnostics and then using Copy&Command sounds actually not too bad, that would be easy to implement.

- **name**: The human readable name of the filter.
- **regex** (optional): Regular expression, not encased in slashes, to use.
Regex flags are not supported, and are global and case sensitive.
- **objectRef** (optional): Required if `jsonPath` is present.
Copy link
Member

Choose a reason for hiding this comment

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

In this context it would probably make sense to be able to reference CRs, i assume? So we probably need to think about what an objectRef exactly should be

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. objectRef, fwiw, is a standard convention. Here we have a bit of a partial one since you're not specifying the full GVK, so I guess we're not technically following convention here 🤔 but we will need to for CRs.

@gerred
Copy link
Member

gerred commented Feb 11, 2020

odd these tests are failing, assuming they're flakes.

@runyontr
Copy link
Member

Overall looks like a great KEP that will provide some good value for debugging. The only thing I would follow up with, is that given a diagnostic bundle: Now what?

  1. We should think about how these get submitted to the Bug team (Operator or KUDO), and maybe make the attachment to a Github issue part of the CLI tooling
  2. Once the bug team receives the bundle, we should be building some sort of tool that can parse/explore it more easily and maybe search for common/reocurring issues/solutions

- **objectRef** (optional): Required if `jsonPath` is present.
- **jsonPath** (optional): JSONPath referencing a non-object key in the
referenced object. All instances of the value of this key will be removed and
replaced with `**FILTERED**`. Required if `objectRef` is present.
Copy link
Member

Choose a reason for hiding this comment

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

Might be a good idea to have an optional replacement value that defaults to "FILTERED".
In this case we could have a filter for IP-Addresses that replaces IPs with "XXX.XXX.XXX.XXX" so we can distinguish between different filtered values?

Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

Nice work! I left a bunch of questions/proposals.


Checks:

- Does the Kubernetes cluster have enough resources to install the operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Is such a preflight check really feasible? How does one determine the amount of memory/disc/cpus/gpus in the whole cluster? And even if they're available right now, how about in 1s? What about things like PVC and PVs? How does one determine everything is needed for a specific plan to run successfully? Do you dry-run execute it and render all resources in the CLI?
Anyway, I believe preflight checks should be a separate issue/KEP/proposal.

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, we'll split out preflight check. @mpereira's original work included it, but the scope of just the support diagnostics is so large it makes sense to take on it's own.

keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
command: # Can be string or array
- nslookup
- google.com
objectRef:
Copy link
Contributor

Choose a reason for hiding this comment

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

What if you wanted to run the command on a specific set of pods (but not necessarily whole StatefulSet). Label selectors might be useful here too (see my comment above ^^).

Copy link
Member

Choose a reason for hiding this comment

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

ya I like label selectors a lot as an option.

Copy link
Member

Choose a reason for hiding this comment

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

(still, think reference-able objects is an even better long term solution, graph graph graph 📈 😉 )

Copy link
Contributor

Choose a reason for hiding this comment

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

Added label selector as @zen-dog suggested, skipped matchExpression as its usage doesn't seem so clear at the moment
Removed templates and name field, because

  • we agree on disliking templates in operator.yaml
  • I can be wrong here, but if objects can be named according to a certain template (pattern), they can as well be labelled according to the pattern
  • in the worst case we'll just add them

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also remove the objectRef?

key: "zookeeper-configuration"
kind: Copy
spec:
path: /opt/zookeeper/server.properties
Copy link
Contributor

Choose a reason for hiding this comment

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

We should allow an array of files here. Otherwise, we'll have to specify N tasks for N files.

Copy link
Member

Choose a reason for hiding this comment

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

👍 good call

keps/0022-diagnostics-bundle.md Show resolved Hide resolved
complexity of selecting resources to run commands and files on.

Steps in a bundle run serially. To prevent the KUDO controller manager from
crashing, the collector process runs in another pod as a job. **TODO**:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I've taken another close look at what data we want to collect and all of it is available from the CLI. We have a mix of k get/describe/logs/events and k exec/cp for executing commands and saving stdout/copy out files from the pods. Frankly, all of that could be a rather simple kudo-cli command.

If our past taught us anything, then we know that logs from the big clusters can easily exceed gigabytes (even gzipped). Handling them, either in the Manager or in a separate Pod, will require arbitrary size PVCs?

Copy link
Member

Choose a reason for hiding this comment

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

A user of an operator collecting these diagnostics might not have RBAC to run all of these locally, ESPECIALLY exec/copy - which can present security issues.

Copy link
Member

Choose a reason for hiding this comment

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

That said, I'm open to this approach too. It simplifies a lot of things. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Could things like resumable/retried diagnostic-fetching (wget -C) be feasibly implemented as a set of operations under a single CLI command?

Handling them, either in the Manager or in a separate Pod, will require arbitrary size PVCs?

That's a good point. It should at a minimum be called out in documentation if that's the path taken, imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

A user of an operator collecting these diagnostics might not have RBAC to run all of these locally, ESPECIALLY exec/copy - which can present security issues.

Fair point. Though I expect most users have access to "their stuff" or having some kind of admin on the line with the right access. It seems like a small price to pay for a massive implementation simplification 🤷‍♀

Copy link
Contributor

Choose a reason for hiding this comment

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

Could things like resumable/retried diagnostic-fetching (wget -C) be feasibly implemented as a set of operations under a single CLI command?

That's the easiest way to implement them IMHO. Everything else adds layers of complexity: extra image to maintain, extra Pod/Job to deploy, extra things that can failover to think about...

Copy link
Member

Choose a reason for hiding this comment

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

A user of an operator collecting these diagnostics might not have RBAC to run all of these locally, ESPECIALLY exec/copy - which can present security issues.

Oooh.. this really opens a can of worms.

This seems to imply that we treat the controller as a privileged entity, and therefore treat OperationVersions as trusted resources (because their diagnostics spec can prescribe whatever), and Instances as untrusted ones. However I do not think we mention this anywhere in the operator developer docs - for example that the developer should treat parameter values as untrusted input.

I think for now we should do something like:

  1. say in the KEP that it's unspecified where the diagnostics collection runs,
  2. actually run it from the CLI for now, and require RBAC to be opened up as needed

And in parallel we can start thinking about KUDO security model more broadly.

@kensipe
Copy link
Member

kensipe commented Mar 10, 2020

@gerred ping... what are the next steps here? are you still owner of this issue.

@kensipe kensipe modified the milestones: 0.11.0, 0.12.0 Mar 10, 2020
@kensipe
Copy link
Member

kensipe commented Mar 10, 2020

needs rebase with dco signoff

@kensipe kensipe self-assigned this Mar 10, 2020
@zen-dog zen-dog changed the title Add initial KEP-22: Diagnostics Bundle. KEP-22: Add initial design on diagnostics bundles Mar 10, 2020
@zen-dog zen-dog force-pushed the kep-22-diagnostics-bundle branch 2 times, most recently from cc60ca2 to 51bc070 Compare March 10, 2020 14:54
@vemelin-epm vemelin-epm self-assigned this Mar 10, 2020
@kensipe
Copy link
Member

kensipe commented Mar 10, 2020

Thanks @vemelin-epm for taking this over. What is expected next? Is this WIP? or ready for review?

Copy link
Member

@porridge porridge left a comment

Choose a reason for hiding this comment

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

This is great work, but I think it's getting bogged down with too much detail. I'd say let's finish the problem statement and rough design overview and merge it. And then do incremental improvements, because this is already taking a long time.

keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved

## Prior art, inspiration, resources

### Diagnostics
Copy link
Member

Choose a reason for hiding this comment

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

Please also take a look at sonobuoy. I've never used it myself, but heard that it's pretty extensible, so please investigate that what we're designing could not be done simply with a sonobuoy plugin. Also a sentence or two of explanation why we are not reusing/extending one of these "prior arts" (these related to k8s of course) would be good.

Copy link
Member

Choose a reason for hiding this comment

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

I like sonobuoy a lot and am open to using it. I've only used it for running conformance tests in the past. If the implementation makes sense more as a series of Sonobuoy plugins that are then just part of someone's spec, that's great. We should just balance this with adding it as an external dependency as another service we need to manage.

keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
### bundle.resources.HTTP

- **serviceRef**: Object containing references to a Kubernetes service. This is
scoped to KUDO-only services.
Copy link
Member

Choose a reason for hiding this comment

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

Please clarify what a "KUDO-only service" is. Personally I'm puzzled by this statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am puzzled as well. @gerred could you please shed some light here?

Copy link
Member

Choose a reason for hiding this comment

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

We're just assuming that the only Kubernetes services we can reached were made by KUDO.

Filters are a list of filters. They contain the following keys:

- **name**: The human readable name of the filter.
- **regex** (optional): Regular expression, not encased in slashes, to use.
Copy link
Member

Choose a reason for hiding this comment

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

OK, but what is the semantics of this regex? What is it applied against? Does a match mean "include in the output" or "omit" or "redact"?

Copy link
Contributor

@vemelin-epm vemelin-epm Mar 18, 2020

Choose a reason for hiding this comment

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

  • semantics: based on the example above I assume it's meant to be applied on the files of certain structure (e.g. the host regex above aims at yaml), but it's not obvious how it's application scope should be restricted. @gerred could you please say some words here?
  • what match means: I think the current text implies quite consistently that filtering is understood as redacting (say, with asterisks) but not removing the sensitive content and therefore whatever matches should be redacted. So, unless my understanding is wrong, I'd personally prefer not to add more explanations.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the original intent here was that after all collection occurred, everything passed through the filters / redaction. @vemelin-epm is correct on the second point.

path: /opt/zookeeper/server.properties
objectRef:
kind: StatefulSet # Runs on ALL pods in the statefulset
name: "{{ .InstanceName }}-zookeeper"
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @zen-dog here. Let's not do this. Let's use label sets to select objects as the convention requires.

Copy link
Contributor

Choose a reason for hiding this comment

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

removed

complexity of selecting resources to run commands and files on.

Steps in a bundle run serially. To prevent the KUDO controller manager from
crashing, the collector process runs in another pod as a job. **TODO**:
Copy link
Member

Choose a reason for hiding this comment

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

A user of an operator collecting these diagnostics might not have RBAC to run all of these locally, ESPECIALLY exec/copy - which can present security issues.

Oooh.. this really opens a can of worms.

This seems to imply that we treat the controller as a privileged entity, and therefore treat OperationVersions as trusted resources (because their diagnostics spec can prescribe whatever), and Instances as untrusted ones. However I do not think we mention this anywhere in the operator developer docs - for example that the developer should treat parameter values as untrusted input.

I think for now we should do something like:

  1. say in the KEP that it's unspecified where the diagnostics collection runs,
  2. actually run it from the CLI for now, and require RBAC to be opened up as needed

And in parallel we can start thinking about KUDO security model more broadly.

Copy link
Member

@porridge porridge left a comment

Choose a reason for hiding this comment

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

I think we're getting there. A bunch of nitpicks inline, and I really think we should change the word filter since it implies omitting information rather than replacing it with something else. I'd suggest redact but maybe someone has a better alternative.

- At least not initially: collection of metrics from monitoring services (e.g.,
Prometheus, Statsd, etc.).
- Automatic fixing of faults
- Preflight checks
Copy link
Member

Choose a reason for hiding this comment

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

It's not entirely clear to me what this point means... 🤔

keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
- **Command**: Run a command on a running pod and copy the stdout/err. Higher level
resources can also be used, which will run the command on all pods selected by
that resource.
- **Task**: Run a KUDO task and copy the stdout and other arbitrary files.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this... a "KUDO task" in general does not have notions of "stdout" or "files". Some specific task types might have something close to those.
However both this and the bundle.resources.Task mention below are so hand-wavy that I'd suggest removing them for now. The paragraph above does say "subject to change" so we can always introduce something like this later, once we have a better idea what's needed.

keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
use magical strings with templates. Future iterations of this will reduce the
complexity of selecting resources to run commands and files on.

Filtering is an important part of diagnostics collection. It enables diagnostics
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest renaming "filtering" with "redaction" everywhere, since this is what it is about. "Filtering" makes me thing of grep while "redaction" is sed 's///' in my mind.

Comment on lines 312 to 322
By default, KUDO filters all resources (and custom resources) of values
contained within the KUDO Instance's secrets. This is configurable with the
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this seems like a promise that is hard to deliver on... for example the secret may be a JSON file which contains a password ({"user":"admin", "password": "foo"}). How do we make KUDO recognize that it should elide that password when it encounters it in a log file in a different context (... logging into 1.2.3.4 with password foo.)?

Copy link
Contributor

Choose a reason for hiding this comment

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

True. But we probably cannot avoid doing this anyway, otherwise the bundle cannot be safely shared. For files with known structure (e.g. yaml) it will be pretty doable. Logs will present a problem indeed, however we can just blindly black out whatever matches any secret as the initial approach, or there may be configurable phrase-based replacement. I may be wrong here, but I believe that placing sensitive information in the logs is a dubious practice after all

resources can also be used, which will run the command on all pods selected by
that resource.
- **Task**: Run a KUDO task and copy the stdout and other arbitrary files.
- **HTTP**: Make an HTTP request from the KUDO controller manager to a named
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to promise that it will be the controller manager who does the fetching... Perhaps this should just say it should be fetched by a process running within the cluster?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, the reason for executing it from the manager is mentioned in the L258. We want to support air-gapped clusters (so no fetching of arbitrary images) and don't want operator developers to maintain curl-containers (though busybox has wget so that might be not an issue).

In general, I think all these "diagnostics forms" were meant to be run from the KUDO manager as some sort of tasks: not necessarily as plan-tasks but something similar (@gerred correct me if I'm wrong). Now, at least for the general diagnostics, we switched to the cli-based approach. We might do the same for extended diagnostics too. E.g. a HTTP request might be a kubectl exec busybox-pod wget ...

keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
@porridge porridge added the kep/22 Diagnostics bundles label Mar 26, 2020
Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

Another pass on it. I think this is in good-enough shape that we can merge it as provisional and update later. The biggest question for me would be whether the extended diagnostics collection happens on the client-side, triggered by the CLI in the cluster, or triggered by the KUDO manager as some sort of tasks. We can mention it and leave it open for now.

@vemelin-epm PTAL and address all the nits and let's merge this.

keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
resources can also be used, which will run the command on all pods selected by
that resource.
- **Task**: Run a KUDO task and copy the stdout and other arbitrary files.
- **HTTP**: Make an HTTP request from the KUDO controller manager to a named
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, the reason for executing it from the manager is mentioned in the L258. We want to support air-gapped clusters (so no fetching of arbitrary images) and don't want operator developers to maintain curl-containers (though busybox has wget so that might be not an issue).

In general, I think all these "diagnostics forms" were meant to be run from the KUDO manager as some sort of tasks: not necessarily as plan-tasks but something similar (@gerred correct me if I'm wrong). Now, at least for the general diagnostics, we switched to the cli-based approach. We might do the same for extended diagnostics too. E.g. a HTTP request might be a kubectl exec busybox-pod wget ...

An individual bundle resource is represented as an element in the list inside of the
`diagnostics.bundle.resources` key. Resources ALWAYS have the following keys:

- **description**: The human-readable name of the file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably optional since the name will be enough most of the time.

command: # Can be string or array
- nslookup
- google.com
objectRef:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also remove the objectRef?

- **spec**: The attributes of a particular kind. This is different for every
kind.

Also, specs may include an `objectRef`. It ALWAYS has the following keys:
Copy link
Contributor

Choose a reason for hiding this comment

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

This and subsequent mentionings of the objectRef are outdated since we're going with the selector?

Copy link
Contributor

Choose a reason for hiding this comment

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

removed

keps/0022-diagnostics-bundle.md Outdated Show resolved Hide resolved
@kensipe kensipe modified the milestones: 0.12.0, 0.13.0 Apr 9, 2020
@vemelin-epm vemelin-epm force-pushed the kep-22-diagnostics-bundle branch 2 times, most recently from 8685305 to d592a88 Compare April 14, 2020 08:40
Copy link
Member

@porridge porridge left a comment

Choose a reason for hiding this comment

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

LGTM, but please:

  • do mention sonobuoy in the prior art section, since you did investigate it.
  • replace the TODO at the bottom with some table (see other KEPs for an example)

vemelin-epm pushed a commit that referenced this pull request Apr 15, 2020
Co-Authored-By: Aleksey Dukhovniy <adukhovniy@mesosphere.io>
Co-Authored-By: Marcin Owsiany <mowsiany@D2iQ.com>

Signed-off-by: Gerred Dillon <hello@gerred.org>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Vasilii Emelin <vasilii_emelin@epam.com>
Co-Authored-By: Gerred Dillon <hello@gerred.org>
Co-Authored-By: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Co-Authored-By: Andreas Neumann <aneumann@mesosphere.com>
Co-Authored-By: Marcin Owsiany <mowsiany@D2iQ.com>

Signed-off-by: Vasilii Emelin <vasilii_emelin@epam.com>
@vemelin-epm vemelin-epm merged commit 5ca644e into master Apr 17, 2020
@vemelin-epm vemelin-epm added this to Done in KUDO Global via automation Apr 17, 2020
@vemelin-epm vemelin-epm deleted the kep-22-diagnostics-bundle branch April 17, 2020 14:10
@mpereira
Copy link
Member Author

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kep/22 Diagnostics bundles kind/kep Kudo Enhancement
Projects
KUDO Global
  
Done
Development

Successfully merging this pull request may close these issues.

Diagnostics Bundle KEP
8 participants