api: Add 'detail' field in ListPods and ListImages request. #1786

yifan-gu · 2015-11-24T01:24:53Z

This enables the API service to return detailed pod/image infos
if required. So for some use case where the users want to get N
pods' detailed info, this saves N grpc round trips.

cc @jonboulle @alban
cc @dgonyeo you might take advantage of this to not to inspect each pod later.

jonboulle · 2015-11-24T02:30:18Z

Any performance data to suggest why we shouldn't just make this the default behaviour?

yifan-gu · 2015-11-24T03:07:10Z

@jonboulle Will make some benchmark tomorrow.

yifan-gu · 2015-11-24T21:47:00Z

Benchmark result:

BenchmarkListSinglePodDetail          30      44337812 ns/op
BenchmarkListSinglePodNoDetail      2000        731483 ns/op
BenchmarkList10PodsDetail          3     469036344 ns/op
BenchmarkList10PodsNoDetail      500       4590260 ns/op
BenchmarkInspectPodIn10Pods       30      35155285 ns/op
BenchmarkListSingleImageDetail        30      43079439 ns/op
BenchmarkListSingleImageNoDetail          30      41183525 ns/op
BenchmarkList10ImagesDetail       50      47760750 ns/op
BenchmarkList10ImagesNoDetail         30      41520293 ns/op
BenchmarkInspectImageIn10Images       30      45591321 ns/op
ok      github.com/coreos/rkt/tests 283.566s

yifan-gu · 2015-11-30T22:26:39Z

Fixed the benchmark:

PASS
BenchmarkList1PodNoDetail       2000        712538 ns/op
BenchmarkList5PodsNoDetail      1000       2263804 ns/op
BenchmarkList50PodsNoDetail      100      17961261 ns/op
BenchmarkList1PodDetail     1000       2124999 ns/op
BenchmarkList5PodsDetail         200       9159586 ns/op
BenchmarkList50PodsDetail         20      91928159 ns/op
BenchmarkInspectPodIn5Pods       500       2263786 ns/op
BenchmarkInspectPodIn50Pods     1000       2334586 ns/op
BenchmarkList1ImageNoDetail     1000       1865547 ns/op
BenchmarkList5ImagesNoDetail        1000       1897841 ns/op
BenchmarkList50ImagesNoDetail       1000       1857811 ns/op
BenchmarkList1ImageDetail       1000       1752596 ns/op
BenchmarkList5ImagesDetail      1000       1832656 ns/op
BenchmarkList50ImagesDetail     1000       1851894 ns/op
BenchmarkInspectImageIn5Images      1000       1800548 ns/op
BenchmarkInspectImageIn50Images     1000       1798823 ns/op

PASS
BenchmarkList1PodNoDetail       2000        738889 ns/op
BenchmarkList10PodsNoDetail      300       4093447 ns/op
BenchmarkList100PodsNoDetail          30      36345471 ns/op
BenchmarkList1PodDetail     1000       2182867 ns/op
BenchmarkList10PodsDetail        100      19460217 ns/op
BenchmarkList100PodsDetail        10     174948011 ns/op
BenchmarkInspectPodIn10Pods     1000       2326570 ns/op
BenchmarkInspectPodIn100Pods        1000       2315756 ns/op
BenchmarkList1ImageNoDetail     1000       1600702 ns/op
BenchmarkList10ImagesNoDetail       1000       1898855 ns/op
BenchmarkList100ImagesNoDetail      1000       1638985 ns/op
BenchmarkList1ImageDetail       1000       1878780 ns/op
BenchmarkList10ImagesDetail     1000       1845593 ns/op
BenchmarkList100ImagesDetail        1000       1628955 ns/op
BenchmarkInspectImageIn10Images     1000       1844565 ns/op
BenchmarkInspectImageIn100Images        1000       1644327 ns/op

From the result I saw:

ListPods doesn't scale well.
ListPods(detail=false) is much faster then ListPods(detail=true).
InspectPods() pretty much takes constant time .
ListImages(detail=false), ListImages(detail=true) takes similar time, (ListImages(detail=true) just adds one more encoding/decoding operation of the manifest)
InspectImage() and ListImages() takes similar time, as InspectImage() returns same amount of info as ListImages(detail=true)

jonboulle · 2015-12-01T00:35:01Z

I guess this is okay - @alban @dgonyeo thoughts?

cgonyeo · 2015-12-01T00:53:18Z

Seems fine to me

alban · 2015-12-01T10:47:56Z

rkt/api_service.go

@@ -332,8 +332,15 @@ func (s *v1AlphaAPIServer) ListPods(ctx context.Context, request *v1alpha.ListPo
 			return
 		}

-		if !filterPod(pod, manifest, request.Filter) {
+		if request.Detail {
+			if err := fillAppInfo(s.store, p, pod); err != nil { // Do not return partial pods.


Why not calling fillAppInfo only if the pod is not filtered by filterPod?

alban · 2015-12-01T10:59:56Z

Can you explain in tests/README.md how to try the benchmarks? Do you want to run them in Semaphore or is it just for testing manually?

The idea to add this detail field sounds good to me.

yifan-gu · 2015-12-01T19:12:53Z

Can you explain in tests/README.md how to try the benchmarks? Do you want to run them in Semaphore or is it just for testing manually?

@alban I think we need to be able to run the bench with something like make benchmark, but I actually don't know how to do this, needs help from @krnowak :)
Basically it's similar to run the functional tests, just with some more flags -bench=. -run=Benchmark

Besides, there's not plan to run them in semaphore as it takes a long time.

yifan-gu · 2015-12-05T02:34:37Z

ping ?

alban · 2015-12-07T10:35:26Z

rkt/api_service.go

+
+		if request.Detail {
+			if err := fillAppInfo(s.store, p, pod); err != nil { // Do not return partial pods.
+				return


The following comment was hidden by GitHub:

Why not calling fillAppInfo only if the pod is not filtered by filterPod?

alban · 2015-12-07T10:40:36Z

Basically it's similar to run the functional tests, just with some more flags -bench=. -run=Benchmark

Can you just say that in the tests/README.md?

## Running the benchmarks

make check GO_TEST_FUNC_ARGS='-bench=. -run=Benchmark'

yifan-gu · 2015-12-07T18:52:57Z

@alban Fixed, added readme.

alban · 2015-12-08T09:50:20Z

LGTM after a rebase :)

This enables the API service to return detailed pod/image infos if required. So for some use case where the users want to get N pods' detailed info, this saves N grpc round trips.

yifan-gu · 2015-12-08T18:04:01Z

Rebased

vcaputo · 2015-12-08T18:46:39Z

tests/rkt_api_service_bench_test.go

+}
+
+func BenchmarkList1PodNoDetail(b *testing.B) {
+	benchPods(b, 1, false, true, false)


I know this is just test/benchmark code, but benchPods() use of these bools makes for pretty unreadable code.

yifan-gu · 2015-12-09T23:25:37Z

@vcaputo I updated the tests, splitted into smaller functions. How does this look like?

yifan-gu · 2015-12-14T19:19:09Z

@vcaputo I am merging this one, if there are more comments wrt to tests, I can iterate later.

api: Add 'detail' field in ListPods and ListImages request.

yifan-gu added needs/review priority/P1 component/api server area/api labels Nov 24, 2015

yifan-gu force-pushed the api_detail branch 4 times, most recently from af31e0f to f00d78c Compare November 24, 2015 21:42

yifan-gu force-pushed the api_detail branch 6 times, most recently from e427946 to 500f810 Compare November 25, 2015 23:09

yifan-gu mentioned this pull request Nov 30, 2015

rkt: rewrote GetPods to use rkt's api service kubernetes/kubernetes#17969

Merged

yifan-gu force-pushed the api_detail branch 3 times, most recently from 195b7b8 to 334d86c Compare November 30, 2015 22:00

yifan-gu force-pushed the api_detail branch from 334d86c to 70cfc46 Compare November 30, 2015 23:46

alban reviewed Dec 1, 2015
View reviewed changes

yifan-gu force-pushed the api_detail branch from 70cfc46 to c90205e Compare December 1, 2015 18:34

alban reviewed Dec 7, 2015
View reviewed changes

yifan-gu force-pushed the api_detail branch from c90205e to 74f2456 Compare December 7, 2015 18:52

alban added reviewed/lgtm and removed needs/review labels Dec 8, 2015

api: Add 'detail' field in ListPods and ListImages request.

e0bcb18

This enables the API service to return detailed pod/image infos if required. So for some use case where the users want to get N pods' detailed info, this saves N grpc round trips.

yifan-gu force-pushed the api_detail branch from 74f2456 to 557ab71 Compare December 8, 2015 18:03

vcaputo reviewed Dec 8, 2015
View reviewed changes

yifan-gu force-pushed the api_detail branch from 557ab71 to e05b96f Compare December 9, 2015 21:00

yifan-gu removed the reviewed/lgtm label Dec 9, 2015

tests: Add benchmarks for api service.

c9f6994

yifan-gu force-pushed the api_detail branch from e05b96f to c9f6994 Compare December 9, 2015 23:24

yifan-gu mentioned this pull request Dec 10, 2015

replace single filter with multiple filters #1853

Merged

yifan-gu added the needs/review label Dec 10, 2015

yifan-gu removed the needs/review label Dec 14, 2015

yifan-gu added a commit that referenced this pull request Dec 14, 2015

Merge pull request #1786 from yifan-gu/api_detail

77f16f3

api: Add 'detail' field in ListPods and ListImages request.

yifan-gu merged commit 77f16f3 into rkt:master Dec 14, 2015

yifan-gu deleted the api_detail branch December 14, 2015 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: Add 'detail' field in ListPods and ListImages request. #1786

api: Add 'detail' field in ListPods and ListImages request. #1786

yifan-gu commented Nov 24, 2015

jonboulle commented Nov 24, 2015

yifan-gu commented Nov 24, 2015

yifan-gu commented Nov 24, 2015

yifan-gu commented Nov 30, 2015

jonboulle commented Dec 1, 2015

cgonyeo commented Dec 1, 2015

alban Dec 1, 2015

alban commented Dec 1, 2015

yifan-gu commented Dec 1, 2015

yifan-gu commented Dec 5, 2015

alban Dec 7, 2015

alban commented Dec 7, 2015

yifan-gu commented Dec 7, 2015

alban commented Dec 8, 2015

yifan-gu commented Dec 8, 2015

vcaputo Dec 8, 2015

yifan-gu commented Dec 9, 2015

yifan-gu commented Dec 14, 2015

api: Add 'detail' field in ListPods and ListImages request. #1786

api: Add 'detail' field in ListPods and ListImages request. #1786

Conversation

yifan-gu commented Nov 24, 2015

jonboulle commented Nov 24, 2015

yifan-gu commented Nov 24, 2015

yifan-gu commented Nov 24, 2015

yifan-gu commented Nov 30, 2015

jonboulle commented Dec 1, 2015

cgonyeo commented Dec 1, 2015

alban Dec 1, 2015

Choose a reason for hiding this comment

alban commented Dec 1, 2015

yifan-gu commented Dec 1, 2015

yifan-gu commented Dec 5, 2015

alban Dec 7, 2015

Choose a reason for hiding this comment

alban commented Dec 7, 2015

yifan-gu commented Dec 7, 2015

alban commented Dec 8, 2015

yifan-gu commented Dec 8, 2015

vcaputo Dec 8, 2015

Choose a reason for hiding this comment

yifan-gu commented Dec 9, 2015

yifan-gu commented Dec 14, 2015