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

Add daemonsets #2526

Merged
merged 4 commits into from
May 22, 2017
Merged

Add daemonsets #2526

merged 4 commits into from
May 22, 2017

Conversation

ekimekim
Copy link
Contributor

Fixes #1444

This adds collection of daemonset info to the probe,
and two new views to the app. One showing only daemonsets,
one showing deployments + daemonsets.

This is too many views (see discussion in #1444), but was useful for initial debugging - we can remove the daemonsets view before merging.

We need to decide how we are going to present this new 'combined view' to the user in a way that makes it obvious what's going on. For now I've labelled it 'Replica groups' but that's not good enough.

@ekimekim ekimekim requested a review from 2opremio May 16, 2017 20:58
selector, err := unversioned.LabelSelectorAsSelector(d.Spec.Selector)
if err != nil {
// TODO(paulbellamy): Remove the panic!
panic(err)

This comment was marked as abuse.

render/pod.go Outdated
// in (for now) a very naive way.
var ReplicaGroupRenderer = MakeReduce(
DeploymentRenderer,
DaemonSetRenderer,

This comment was marked as abuse.

@2opremio
Copy link
Contributor

2opremio commented May 17, 2017

Done with the review: Minor code comment and a fundamental design question.

I don't mind going ahead with this a refining it later, but as a kubernetes user, I find the ReplicaGroup view a bit odd.

@ekimekim
Copy link
Contributor Author

(addressing design question at the top level instead of at the specific comment)

Yeah, I agree. The view is confusing and right now badly named. Controllers is a better name (though I'm unsure if it's a good name either). I added the daemonset view itself because it was trivial to do and let me ensure it was working correctly.

The ReplicaGroup view was an attempt at addressing concerns in #1444, and yeah I'm looking for ideas on how to make this less confusing.

As I see it, we have three options. In my order of preference:

  1. Drop the combined view from this PR and merge it with just the seperate views, as was the original intent of Add view for DaemonSets #1444. We can then work on a better, non-rushed version of the combined view.
  2. Merge this PR as-is, with both daemonset view and combined view, and iterate on these issues in a followup.
  3. Delay merging this PR (which adds any support for daemonsets at all) until the combined issue is resolved.

@2opremio
Copy link
Contributor

I would go for (1) as well. @rade ?

@rade
Copy link
Member

rade commented May 17, 2017

go for (1), but make sure there is a follow up, in particular peeling off bits from #552, specifically replacing (nearly) all existing views with a view that shows you everything in existence, at the highest level of abstraction in which it has a representation.

@ekimekim ekimekim force-pushed the mike/k8s/add-daemonsets branch from 4cc432c to 9055edf Compare May 17, 2017 18:48
@ekimekim
Copy link
Contributor Author

I've removed the combined view parts and otherwise rebased fixups. PTAL.

@2opremio
Copy link
Contributor

Looks good. There is still a comment on a TODO

@ekimekim ekimekim force-pushed the mike/k8s/add-daemonsets branch from 9055edf to c0751cd Compare May 19, 2017 22:08
@ekimekim
Copy link
Contributor Author

Rebased and resolved conflicts with #2528.
Now propagates errors from getting label selectors for all types, not just daemonsets.
PTAL.

@2opremio
Copy link
Contributor

I am exceptionally merging this myself (and not the PR creator) to move forward with the release

@2opremio 2opremio merged commit 0ee3a3f into master May 22, 2017
@2opremio 2opremio deleted the mike/k8s/add-daemonsets branch May 22, 2017 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants