Introduce integration framework backend #2017

floreks · 2017-06-06T11:57:53Z

There are still lots of things to be done. I have decided to take care of config storage and sync in next PRs as this will be already a big change. Any comments are appreciated.

Major changes

Added IntegrationManager that will be responsible for providing state of integration app (installed, connected, enabled, etc.) and listing all integrations.
Added MetricManager as a dependency of IntegrationManager. It represents group of metric integrations like heapster, prometheus, etc. It will be responsible for controlling status of all metric integrations and providing active metric client.
Added MetricClient interface as a layer of abstraction between resources and metric api. All metric integrations will have to implement that interface.
Removed all hard dependencies on heapster from our sources and replaced them with MetricClient.

Rules for introducing new integration would be:

Implement Integration interface to make sure that every integration app has unique ID and can do a health check to make sure if we can enable it.
Introduce new group-like interface that will be responsible for handling new integration. I'm thinking about adding interface similar to Integration that will make sure that every 'group' can handle integrations, i.e. install/uninstall or enable/disable them.

Later on they should support storage thus implement necessary methods that will make sure that state of integrations is persisted and up to date.

TODO

Agree on the integration framework API
Clean up code
Finish heapster migration (~~graphs are working, sparklines not~~)
Add documentation
Add tests

codecov · 2017-06-06T12:08:15Z

Codecov Report

Merging #2017 into master will increase coverage by 0.12%.
The diff coverage is 52%.

@@            Coverage Diff             @@
##           master    #2017      +/-   ##
==========================================
+ Coverage   59.35%   59.47%   +0.12%     
==========================================
  Files         545      551       +6     
  Lines       11556    11626      +70     
==========================================
+ Hits         6859     6915      +56     
- Misses       4524     4538      +14     
  Partials      173      173

Impacted Files	Coverage Δ
...rc/app/backend/resource/common/resourcechannels.go	`0% <ø> (ø)`	⬆️
src/app/frontend/service/list/stateconfig.js	`83.33% <ø> (ø)`	⬆️
src/app/backend/resource/namespace/detail.go	`17.54% <ø> (ø)`	⬆️
src/app/backend/resource/statefulset/detail.go	`0% <0%> (ø)`	⬆️
...p/backend/resource/replicationcontroller/detail.go	`18.46% <0%> (ø)`	⬆️
src/app/backend/resource/deployment/common.go	`38.7% <0%> (ø)`	⬆️
src/app/backend/resource/daemonset/common.go	`52% <0%> (ø)`	⬆️
src/app/backend/resource/statefulset/common.go	`40% <0%> (ø)`	⬆️
src/app/backend/resource/daemonset/pods.go	`0% <0%> (ø)`	⬆️
src/app/backend/resource/daemonset/detail.go	`14.28% <0%> (ø)`	⬆️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e42a910...3a5e6d8. Read the comment docs.

bryk

Sorry for late review. I thought I commented. Now realized that I didn't click submit.

bryk · 2017-06-19T09:20:33Z

src/app/backend/integration/api/types.go

+
+import "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+// IntegrationID is a unique identification string that every integrated app has to provide.


Can you provide in-comment example of what an id would be? And where I can find other IDs to avoid conflicts.

We could keep all of them as const values in this file. WDYT?

I don't think we have to worry about name collisions here, so simple integration name string should be sufficient for the foreseeable future.

Having all IDs here LGTM.

bryk · 2017-06-19T09:21:33Z

src/app/backend/integration/api/types.go

+// TODO(floreks): Support more information like 'installed' and 'enabled'.
+type IntegrationState struct {
+	Connected   bool    `json:"connected"`
+	LastChecked v1.Time `json:"lastChecked"`


From this I see that our backend code will have state. Can you comment on scalability of this? I.e., how would this work if I have two or twenty dashboard UI replicas?

Until storage sync will be implemented we will not have consistent behavior here with multiple pods since we don't know to which instance kubernetes will proxy the request.

I will implement it next so all data will be kept in a configmap (in the beginning) and backend will be able to sync them across multiple replicas. Backend should be stateless but I didn't want to make this PR bigger by adding storage manager and synchronization.

Storage sync is kind of cache to config map?

What happens if addon is removed with kubectl and the variable connected is still set to true? Does the user get a lot of error message until synchronisation? Do we listen on apiserver or poll?

I was thinking about using watch for dashboard's config map. This way it would be always up to date. Some kind of subscribe/notify mechanism will be needed together with watch to make sure that state is correct.

bryk · 2017-06-19T09:22:52Z

src/app/backend/integration/handler.go

+	manager IntegrationManager
+}
+
+// Install creates new endpoints for integrations.


Hmm... I'm not sure this comment explains what the installation means. And this is the interesting part :) Can you comment? What happens after installation?

I'll describe it better. Right now it's very simple and just exposes integration state but other integrations might want to install their own endpoints also.

bryk · 2017-06-19T09:23:34Z

src/app/backend/integration/api/types.go

+import "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+// IntegrationID is a unique identification string that every integrated app has to provide.
+type IntegrationID string


What's the format of integration ID? Can it be any string?

See previous comment.

bryk · 2017-06-19T09:24:29Z

src/app/backend/integration/manager.go

+	result := make([]api.Integration, 0)
+
+	// Append all types of integrations
+	result = append(result, self.Metric().List()...)


This could be a static list somewhere in a dedicated file. It is hard to find it here. Among business logic code.

I was thinking about that at some point but this list would be empty in the beginning and would have to be filled by some function. In example to create MetricClient integration for heapster we need host and ClientManager args so we can't easily initialize this list.

Additionally slices in go are not thread-safe so keeping static array and operating on it from multiple places might cause some troubles.

Yeah. I meant that we could have integrations-list.go file with this array. Filled the same way you do here, but in a separate file so that the place I register integrations is only one.

Ahh... this makes sense :) I will do that.

cheld · 2017-06-20T12:58:32Z

We could think about combining our config-map to keep settings with the component config proposal:

https://docs.google.com/document/d/1arP4T9Qkp2SovlJZ_y790sBeiWXDO6SG10pZ_UUU-Lc/edit#

floreks · 2017-06-20T13:30:39Z

Unfortunately I don't have access to see it.

cheld · 2017-06-20T13:48:46Z

hmm...it is public

this is issue tracker: kubernetes/enhancements#115

cheld · 2017-06-20T15:06:19Z

src/app/backend/dashboard.go

+	integrationManager := integration.NewIntegrationManager(clientManager)
+	err = integrationManager.Metric().
+		ConfigureHeapster(*argHeapsterHost).
+		Enable(integrationapi.HeapsterIntegrationID)


In the long run, could we move argHeapsterHost into the config map?

So, what is the intended workflow if a user wants to use Prometheus instead of Heapster. Or maybe has already Prometheus installed and then deploys dashboard?

Any integration still requires some code to be pushed upstream. We'd need to implement prometheus client that implements MetricClient interface.

Normally with settings page user will be able to check state of integrations, i.e. if prometheus is not installed in the cluster then user goes to settings page clicks install near prometheus integration and then enables it. That would automatically disable other metric clients.

Once we have more options to choose from we can disable default one and let user decide which one to enable.

hmm...I wonder if this is the right approach. I have not checked the api or anything lately, but my understanding that we do not have to care about which monitoring system is installed. We just use the api facade from apiserver. Also, installing a monitoring system might not be as easy as installing e.g. tiller.

The old issue: #1310

However, we should care about the integration of Graphana.

I have not checked the api or anything lately, but my understanding that we do not have to care about which monitoring system is installed.

There were only plans to introduce interfaces directly in kubernetes that will scrape metrics and work independent of chosen monitoring solution. Unfortunately it is not there and I don't know if or when it will be introduced. Since there is no such facade then we have to take care of that for now.

Anyway if they introduce such API then it will be easy to get rid of monitoring specific code and just rely on this new API.

Also, installing a monitoring system might not be as easy as installing e.g. tiller.

Installing integration is not the hard part. For supported integrations we can versionize required yaml files on our repository and just use them to deploy what's needed (heapster + influx, tiller, etc.).

However, we should care about the integration of Graphana.

Do we? I thought integrations should improve dashboard's experience and grafana is an external UI for monitoring. How would such integration look like?

cheld · 2017-06-22T06:18:00Z

kubeadm also plans an addon manager:

https://docs.google.com/document/d/1Laov9RCOPIexxTMACG6Ffkko9sFMrrZ2ClWEecjYYVg/edit#

we have to think were we want to draw the line so we do not have redundancy. In gerneral we should focus on frontend integration.

floreks · 2017-06-22T06:36:56Z

Integrations have the benefit of providing better UX experience for users but might require more work and code to be pushed upstream.

On the other hand addons could be created by everyone and be maintained independent of dashboard. Also they seem to be more flexible.

Both options have some advantages and disadvantages. IMO integrations are good for popular tools used by many people, like helm. Addons could maybe be able to customize and add simple views.

cheld · 2017-06-26T14:12:04Z

I was wondering which extention should be handled by addon-manager and which extention by Dashboard?
e.g.:
Overlay-network => ok, that is add-on manager.
Weave scope =>Dashboard. However, if I remember correctly I saw it on a feature list of addon manager

So, should we draw some line somewere or accept duplication? IMHO we should check addon manager concepts and also discuss with other SIGs

cheld · 2017-06-30T08:15:40Z

Kops add-on:
https://github.com/kubernetes/kops/tree/master/addons

cheld · 2017-06-30T08:18:23Z

Kops addon manager:
https://github.com/kubernetes/kops/tree/master/channels

floreks · 2017-07-03T08:26:11Z

I think we can merge this part. It does not change current behavior nor add state. It is mostly refactor and also few small bug fixes for getting heapster metrics. Additionally if heapster is not found or is unhealthy on the backend start then it will be disabled so there won't be any unnecessary requests made.

floreks changed the title ~~[WIP] Introduce integration manager backend~~ [WIP] Introduce integration framework backend Jun 6, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 6, 2017

floreks force-pushed the refactor-integrations branch from 3104b78 to 4892b54 Compare June 9, 2017 12:38

floreks requested review from bryk and rf232 June 12, 2017 08:22

floreks mentioned this pull request Jun 12, 2017

Feature/implementation support request: cluster graph integration #2008

Open

floreks force-pushed the refactor-integrations branch 2 times, most recently from 79eb78e to 8e06def Compare June 13, 2017 13:57

floreks self-assigned this Jun 14, 2017

bryk reviewed Jun 19, 2017

View reviewed changes

floreks force-pushed the refactor-integrations branch from 187dc5a to 1c9864c Compare June 19, 2017 13:06

cheld reviewed Jun 20, 2017

View reviewed changes

floreks force-pushed the refactor-integrations branch 2 times, most recently from dc97be9 to 17eac14 Compare June 21, 2017 11:56

floreks changed the title ~~[WIP] Introduce integration framework backend~~ Introduce integration framework backend Jun 21, 2017

floreks force-pushed the refactor-integrations branch from 18950e9 to 813bf04 Compare June 26, 2017 10:27

floreks force-pushed the refactor-integrations branch from 813bf04 to 73bb5f8 Compare June 27, 2017 10:12

floreks added 4 commits July 3, 2017 10:18

Remove hard dependencies on heapster from resources

2024bff

Introduce integration manager

2492fde

Fix tests

a4c3625

Fix state check

5c70859

floreks added 11 commits July 3, 2017 10:18

Add cached resources to metrics api

3d7fd92

Fix sparklines

614bda0

Refactor labels to use resource UID instead of string

919e0f6

Update documentation

e5686aa

Remove unused tests and update docs

d89fd28

Format files

9cf16b8

Fix tests and bug during compression heapster selectors

9c14eb6

Add tests

1797f6b

Improve documentation

169afdf

Move integration list to separate file

565fc94

Resolve conflicts

3a5e6d8

floreks force-pushed the refactor-integrations branch from 73bb5f8 to 3a5e6d8 Compare July 3, 2017 08:22

maciaszczykm approved these changes Jul 3, 2017

View reviewed changes

maciaszczykm merged commit fb10919 into kubernetes:master Jul 3, 2017

This was referenced Jul 5, 2017

Dashboard panics on getting heapster selector when stateful sets not supported by cluster #1916

Closed

Dashboard should load fast, even when heapster is unresponsive #1572

Closed

floreks deleted the refactor-integrations branch August 7, 2017 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce integration framework backend #2017

Introduce integration framework backend #2017

floreks commented Jun 6, 2017 •

edited

Loading

codecov bot commented Jun 6, 2017 •

edited

Loading

bryk left a comment

bryk Jun 19, 2017

floreks Jun 19, 2017 •

edited

Loading

bryk Jun 26, 2017

bryk Jun 19, 2017

floreks Jun 19, 2017

cheld Jun 20, 2017

floreks Jun 21, 2017 •

edited

Loading

bryk Jun 19, 2017

floreks Jun 19, 2017

bryk Jun 19, 2017

floreks Jun 19, 2017

bryk Jun 19, 2017

floreks Jun 19, 2017

bryk Jun 26, 2017

floreks Jun 26, 2017

floreks Jun 26, 2017

cheld commented Jun 20, 2017

floreks commented Jun 20, 2017

cheld commented Jun 20, 2017

cheld Jun 20, 2017

cheld Jun 20, 2017

floreks Jun 20, 2017 •

edited

Loading

cheld Jun 21, 2017

floreks Jun 21, 2017 •

edited

Loading

cheld commented Jun 22, 2017

floreks commented Jun 22, 2017 •

edited

Loading

cheld commented Jun 26, 2017

cheld commented Jun 30, 2017

cheld commented Jun 30, 2017

floreks commented Jul 3, 2017


		import "k8s.io/apimachinery/pkg/apis/meta/v1"

		// IntegrationID is a unique identification string that every integrated app has to provide.

Introduce integration framework backend #2017

Introduce integration framework backend #2017

Conversation

floreks commented Jun 6, 2017 • edited Loading

Major changes

TODO

codecov bot commented Jun 6, 2017 • edited Loading

Codecov Report

bryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

floreks Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

floreks Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheld commented Jun 20, 2017

floreks commented Jun 20, 2017

cheld commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

floreks Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

floreks Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

cheld commented Jun 22, 2017

floreks commented Jun 22, 2017 • edited Loading

cheld commented Jun 26, 2017

cheld commented Jun 30, 2017

cheld commented Jun 30, 2017

floreks commented Jul 3, 2017

floreks commented Jun 6, 2017 •

edited

Loading

codecov bot commented Jun 6, 2017 •

edited

Loading

floreks Jun 19, 2017 •

edited

Loading

floreks Jun 21, 2017 •

edited

Loading

floreks Jun 20, 2017 •

edited

Loading

floreks Jun 21, 2017 •

edited

Loading

floreks commented Jun 22, 2017 •

edited

Loading