Re: [kubernetes/kubernetes] Bulk watch API - experimental implementation (#57792)
Jordan Liggitt
Glad to see activity on this
Cc @kubernetes/sig-api-machinery-api-reviews @kubernetes/sig-api-machinery-pr-reviews
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Jordan Liggitt
Can you include in the description sample API calls/responses, especially where hey diverge from the original proposal?
k8s-ci-robot
@anjensan: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-e2e-gce | 0787b79 | link | /test pull-kubernetes-e2e-gce |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
k8s-ci-robot
k8s-ci-robot
Andrei Zhlobich
Notable difference with original proposal is routing/multiplexing of different watches.
Instead of 'websocket channels' a simpler approach with explicit 'watchId's is used.
The main workflow for watches looks like:
- client opens a new websocket connection (http GET)
- to subscribe for a watch of a given (set of) objects,
user sends "watch"/"watchList" with "unique" ids attached - "requestId" and "watchId" - this results in sending a single confirmation response to the client,
either '{requestId: "...", failure: {status: "Failure", ...}}' or '{requestId: "...", watchStarted: {...}}' - once subscribed, all objects matching a given selector will be send over websocket
all such responses does not contain 'requestId', field 'watchId' should be used instead - to stop watching for a given (set of) objects, user sends 'stopWatch' request over the the channel;
in response the corresponding watch is stopped and server sends 'watchStopped'.
I've used https://github.com/websockets/wscat to get responses from local dev apiserver.
Symbols '<' and '>' are not presented in real communication (added by wscat).
Also lines started with '#' are my comments.
Run:
wscat \
--key /var/run/kubernetes/client-admin.key \
--cert /var/run/kubernetes/client-admin.crt \
--ca /var/run/kubernetes/server-ca.crt \
--connect wss://localhost:6443/bulk
Examples:
# Invalid request
> {"unknown-field": 123}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","failure":{"metadata":{},"status":"Failure","message":"...","reason":"Invalid","details":{...},"code":422}}
# Get list of resources (single response)
> {"requestId": "1", "list": {"resource": "namespaces"}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","requestId":"1","listResult":
{"list":
{"kind":"NamespaceList","apiVersion":"v1",
"metadata":{"resourceVersion":"897"},
"items": [
{"metadata":{"name":"default", ...},"spec":{"finalizers":["kubernetes"]},"status":{"phase":"Active"}},
{"metadata":{"name":"kube-public", ...},"spec":{"finalizers":["kubernetes"]},"status":{"phase":"Active"}},
{"metadata":{"name":"kube-system", ...},"spec":{"finalizers":["kubernetes"]},"status":{"phase":"Active"}}]}}}
# Get single resource by name
> {"requestId": "2", "get": {"resource": "namespaces", "name": "kube-public"}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","requestId":"2","getResult":
{"item":{"kind":"Namespace","apiVersion":"v1","metadata":{"name":"kube-public", ...},"spec":{"finalizers":["kubernetes"]},"status":{"phase":"Active"}}}}
# Resource does not exist
> {"requestId": "3", "get": {"resource": "namespaces", "name": "xxx"}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","requestId":"3","failure":{"metadata":{},"status":"Failure","message":" \"xxx\" not found","reason":"NotFound","details":{"name":"xxx"},"code":404}}
# Watch for single item
> {"requestId": "4", "watch": {"watchId": "w1", "resource": "nodes", "name": "127.0.0.1"}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","requestId":"4","watchStarted":{"watchId":"w1"}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w1","type":"ADDED","object":{...}}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w1","type":"MODIFIED","object":{...}}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w1","type":"MODIFIED","object":{...}}}
# Watch for list
> {"requestId": "5", "watchList": {"watchId": "w2", "resource": "namespaces"}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","requestId":"5","watchStarted":{"watchId":"w2"}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w2","type":"ADDED","object":{"kind":"Namespace","apiVersion":"v1","metadata":{"name":"kube-public", ...}}}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w2","type":"ADDED","object":{"kind":"Namespace","apiVersion":"v1","metadata":{"name":"kube-system", ...}}}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w2","type":"ADDED","object":{"kind":"Namespace","apiVersion":"v1","metadata":{"name":"default", ...}}}}
# ... Events for both watches are interleaved
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w1","type":"MODIFIED","object":{...}}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w2","type":"ADDED","object":{"kind":"Namespace","apiVersion":"v1","metadata":{"name":"kube-system", ...}}}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w1","type":"MODIFIED","object":{...}}}
# Stop watching
> {"requestId": "6", "stopWatch": {"watchId": "w1"}}
> {"requestId": "7", "stopWatch": {"watchId": "w2"}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","requestId":"7","watchStopped":{"watchId":"w2"}}
< {"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","requestId":"6","watchStopped":{"watchId":"w1"}}
The same API can be used with any http/1 client (POST request).
Operation 'stopWatch' is denied (whole request aborted) because it does not have any sense for HTTP requests.
Example:
curl \
--key /var/run/kubernetes/client-admin.key \
--cert /var/run/kubernetes/client-admin.crt \
--cacert /var/run/kubernetes/server-ca.crt \
-H "Content-Type: application/json" \
-X POST \
-d '{"requestId": "req1", "watchList": {"watchId": "w1", "resource": "pods"}}
{"requestId": "req2", "watchList": {"watchId": "w2", "resource": "events"}}
{"requestId": "req3", "list": {"resource": "nodes"}}
' https://localhost:6443/bulk
Response (streamed with transfer-encoding=chunked):
{"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","requestId":"req1","watchStarted":{"watchId":"w1"}}
{"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","requestId":"req2","watchStarted":{"watchId":"w2"}}
{"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w1","type":"ADDED","object":{...}}}
{"kind":"BulkResponse","apiVersion":"bulk.k8s.io/v1alpha1","watchEvent":{"watchId":"w2","type":"ADDED","object":{...}}}
# ....
Connection is closed by server when all requests are processed (there are no watches).
Otherwise client may just drop connection at any time.
Jenny Buckley
@jennybuckley commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkRequest contains a single request to bulk api. +type BulkRequest struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier used to match requests and responses. + // +optional + RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
any reason why this one isn't a pointer but in the response it is?
I think all the optional fields could benefit from being pointers
https://github.com/kubernetes/community/blame/master/contributors/devel/api-conventions.md#L593
Andrei Zhlobich
@anjensan commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkRequest contains a single request to bulk api. +type BulkRequest struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier used to match requests and responses. + // +optional + RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
I put '+optional' by mistake. 'RequestId' is not optional for requests (only for responses).
Andrei Zhlobich
@anjensan pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: anjensan
We suggest the following additional approver: wojtek-t
Assign the PR to them by writing /assign @wojtek-t in a comment when ready.
No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-device-plugin-gpu | 91e5110 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | 91e5110 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-kubemark-e2e-gce | 91e5110 | link | /test pull-kubernetes-kubemark-e2e-gce |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-verify | a888ac8 | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 91e5110 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-verify | a888ac8 | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 91e5110 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | 91e5110 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-kubemark-e2e-gce | 91e5110 | link | /test pull-kubernetes-kubemark-e2e-gce |
| pull-kubernetes-e2e-gce | 91e5110 | link | /test pull-kubernetes-e2e-gce |
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-verify | a888ac8 | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 91e5110 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | 91e5110 | link | /test pull-kubernetes-e2e-kops-aws |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-verify | a888ac8 | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 91e5110 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | 91e5110 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-kubemark-e2e-gce | 91e5110 | link | /test pull-kubernetes-kubemark-e2e-gce |
| pull-kubernetes-e2e-gce | 91e5110 | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-node-e2e | 91e5110 | link | /test pull-kubernetes-node-e2e |
k8s-ci-robot
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Wojciech Tyczynski
@wojtek-t commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/register.go:
> + "k8s.io/apimachinery/pkg/runtime/schema" +) + +// GroupName is the group name for this API. +const GroupName = "bulk.k8s.io" + +// SchemeGroupVersion is group version used to register these objects +var SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1alpha1"} + +// Resource takes an unqualified resource and returns a Group qualified GroupResource +func Resource(resource string) schema.GroupResource { + return SchemeGroupVersion.WithResource(resource).GroupResource() +} + +var ( + // TODO: move SchemeBuilder with zz_generated.deepcopy.go to k8s.io/api.
This comment seems to be no longer valid - this seems already to be in k8s.io/api
[How this works is that this staging/src/k8s.io// is periodically snapshotted and published as k8s.io/ repository.]
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +
+// BulkRequest contains a single request to bulk api.
+type BulkRequest struct {
+ metav1.TypeMeta `json:",inline"`
+
+ // Opaque identifier used to match requests and responses.
+ // +optional
+ RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
+
+ // Starts new watch.
+ // +optional
+ Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"`
+
+ // Starts new watch.
+ // +optional
+ WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"`
Why do we need separate high-level "Watch" and "WatchList"?
Looking into types below, I would prefer merging those two and making "Name" field simply an optional.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkRequest contains a single request to bulk api. +type BulkRequest struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier used to match requests and responses. + // +optional + RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
Can you please remove the "optional" then?
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + + // Starts new watch. + // +optional + Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"` + + // Starts new watch. + // +optional + WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"` + + // Stops existing watch. + // +optional + StopWatch *StopWatchOperation `json:"stopWatch,omitempty" protobuf:"bytes,4,opt,name=stopWatch"` + + // Gets list of items. + // +optional + List *ListOperation `json:"list,omitempty" protobuf:"bytes,5,opt,name=list"`
Similarly for List and Get here.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + // Contains single watch event.
+ // +optional
+ WatchEvent *BulkWatchEvent `json:"watchEvent,omitempty" protobuf:"bytes,5,opt,name=watchEvent"`
+
+ // Contains single object.
+ // +optional
+ GetResult *GetResult `json:"getResult,omitempty" protobuf:"bytes,6,opt,name=getResult"`
+
+ // Contains single object.
+ // +optional
+ ListResult *ListResult `json:"listResult,omitempty" protobuf:"bytes,7,opt,name=listResult"`
+}
+
+// GroupVersionResource identifies resource.
+type GroupVersionResource struct {
+
For consistency with other "types.go" files in different places, can you please remove empty lines right after type definitions?
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +type GroupVersionResource struct {
+
+ // Name of APIGroup, defaults to core API group.
+ // +optional
+ Group string `json:"group,omitempty" protobuf:"bytes,1,opt,name=group"`
+
+ // Version of APIGroup, defaults to preferred version.
+ // +optional
+ Version string `json:"version,omitempty" protobuf:"bytes,2,opt,name=version"`
+
+ // Resource identifier (pods, events etc). Note: this is not the kind.
+ Resource string `json:"resource" protobuf:"bytes,3,opt,name=resource"`
+
+ // Namespace
+ // +optional
+ Namespace string `json:"namespace,omitempty" protobuf:"bytes,4,opt,name=namespace"`
We shouldn't put Namespace in GroupVersionResource. This should be part of Selector.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> @@ -0,0 +1,216 @@ +/*
@smarterclayton @liggitt - can you please take a look into types definition.
All changes here may affect implementation, so I would like to ensure that we're on the same page about it.
Kubernetes Submit Queue
@anjensan PR needs rebase
Andrei Zhlobich
@anjensan commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/register.go:
> + "k8s.io/apimachinery/pkg/runtime/schema" +) + +// GroupName is the group name for this API. +const GroupName = "bulk.k8s.io" + +// SchemeGroupVersion is group version used to register these objects +var SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1alpha1"} + +// Resource takes an unqualified resource and returns a Group qualified GroupResource +func Resource(resource string) schema.GroupResource { + return SchemeGroupVersion.WithResource(resource).GroupResource() +} + +var ( + // TODO: move SchemeBuilder with zz_generated.deepcopy.go to k8s.io/api.
Such comment exists in all 'register.go' files (I just copied it).
It refers to the cleanup after removing of '/staging' (which is a temporal stage of moving parts of kubernetes into separate repos).
~/g/s/k/kubernetes (bulk-watch) $ grep -r 'TODO: move SchemeBuilder with zz_generated.deepcopy.go to k8s.io/api' | wc -l
52
Andrei Zhlobich
@anjensan commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkRequest contains a single request to bulk api. +type BulkRequest struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier used to match requests and responses. + // +optional + RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
Done
Andrei Zhlobich
> +
+// BulkRequest contains a single request to bulk api.
+type BulkRequest struct {
+ metav1.TypeMeta `json:",inline"`
+
+ // Opaque identifier used to match requests and responses.
+ // +optional
+ RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
+
+ // Starts new watch.
+ // +optional
+ Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"`
+
+ // Starts new watch.
+ // +optional
+ WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"`
Not all options are valid/meaningful for single-item watch.
For example current implementation of watch (not bulk) does not allow 'Name' and 'FieldSelector' at the same time.
Also LabelSelector/Limit/Contine fields doesn't not make sense for single-watch.
I'd like to make the new api "type-safe" as much of possible.
It is usually better not to add a field at all instead of runtime checks "such combination is not allowed".
In our case with two different messages user just can't provide unsupported combinations of fields (there is an exception for ListOptions.Watch field, it is just ignored).
PS: Actually at the beginning I used single 'verb' for both single- and list-watches.
After splitting the implementation also became a bit more structured and readable.
Andrei Zhlobich
@anjensan pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Andrei Zhlobich
> + + // Starts new watch. + // +optional + Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"` + + // Starts new watch. + // +optional + WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"` + + // Stops existing watch. + // +optional + StopWatch *StopWatchOperation `json:"stopWatch,omitempty" protobuf:"bytes,4,opt,name=stopWatch"` + + // Gets list of items. + // +optional + List *ListOperation `json:"list,omitempty" protobuf:"bytes,5,opt,name=list"`
The same reasons as for Watch/WatchList.
But for Get/List situation is even a bit worse.
We already use two different objects for options: GetOptions & ListOptions.
And I'd like to reuse the same types in bulk api.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +type GroupVersionResource struct {
+
+ // Name of APIGroup, defaults to core API group.
+ // +optional
+ Group string `json:"group,omitempty" protobuf:"bytes,1,opt,name=group"`
+
+ // Version of APIGroup, defaults to preferred version.
+ // +optional
+ Version string `json:"version,omitempty" protobuf:"bytes,2,opt,name=version"`
+
+ // Resource identifier (pods, events etc). Note: this is not the kind.
+ Resource string `json:"resource" protobuf:"bytes,3,opt,name=resource"`
+
+ // Namespace
+ // +optional
+ Namespace string `json:"namespace,omitempty" protobuf:"bytes,4,opt,name=namespace"`
Done.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
k8s-ci-robot
@anjensan: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-build | 3589746 | link | /test pull-kubernetes-bazel-build |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-build | 3589746 | link | /test pull-kubernetes-bazel-build |
| pull-kubernetes-bazel-test | 3589746 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-e2e-gce | 3589746 | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 3589746 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-bazel-build | 3589746 | link | /test pull-kubernetes-bazel-build |
| pull-kubernetes-bazel-test | 3589746 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-e2e-gce | 3589746 | link | /test pull-kubernetes-e2e-gce |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-build | 3589746 | link | /test pull-kubernetes-bazel-build |
| pull-kubernetes-bazel-test | 3589746 | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-e2e-gce | 3589746 | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 3589746 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | 3589746 | link | /test pull-kubernetes-e2e-kops-aws |
k8s-ci-robot
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
k8s-ci-robot
Kubernetes Submit Queue
@anjensan PR needs rebase
Wojciech Tyczynski
@wojtek-t commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +
+// BulkRequest contains a single request to bulk api.
+type BulkRequest struct {
+ metav1.TypeMeta `json:",inline"`
+
+ // Opaque identifier used to match requests and responses.
+ // +optional
+ RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
+
+ // Starts new watch.
+ // +optional
+ Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"`
+
+ // Starts new watch.
+ // +optional
+ WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"`
Actually, there isn't anything really protecting you from passing label/field selector for single-item watch (non-bulk).
And there are already fields in "ListOptions" that doesn't make sense for the watch case (limit or continue).
So I would really like to avoid splitting those two.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + + // Starts new watch. + // +optional + Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"` + + // Starts new watch. + // +optional + WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"` + + // Stops existing watch. + // +optional + StopWatch *StopWatchOperation `json:"stopWatch,omitempty" protobuf:"bytes,4,opt,name=stopWatch"` + + // Gets list of items. + // +optional + List *ListOperation `json:"list,omitempty" protobuf:"bytes,5,opt,name=list"`
Yes - that makes more sense in this case.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +type GroupVersionResource struct {
+
+ // Name of APIGroup, defaults to core API group.
+ // +optional
+ Group string `json:"group,omitempty" protobuf:"bytes,1,opt,name=group"`
+
+ // Version of APIGroup, defaults to preferred version.
+ // +optional
+ Version string `json:"version,omitempty" protobuf:"bytes,2,opt,name=version"`
+
+ // Resource identifier (pods, events etc). Note: this is not the kind.
+ Resource string `json:"resource" protobuf:"bytes,3,opt,name=resource"`
+
+ // Namespace
+ // +optional
+ Namespace string `json:"namespace,omitempty" protobuf:"bytes,4,opt,name=namespace"`
Would you be able to merge last two commits with this one?
We would like to first get an agreement on the API (and hopefully merge the API sooner than the actual implementation), so it would be easier to follow with all the changes to this file in a single place.
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: anjensan
We suggest the following additional approver: wojtek-t
Assign the PR to them by writing /assign @wojtek-t in a comment when ready.
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-unit | 5a0ba0e | link | /test pull-kubernetes-unit |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
Clayton Coleman
@smarterclayton commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +
+// BulkRequest contains a single request to bulk api.
+type BulkRequest struct {
+ metav1.TypeMeta `json:",inline"`
+
+ // Opaque identifier used to match requests and responses.
+ // +optional
+ RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
+
+ // Starts new watch.
+ // +optional
+ Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"`
+
+ // Starts new watch.
+ // +optional
+ WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"`
All watch is implemented as WatchList. The singular watch is just a convenience. This API should reflect regular WatchList, not the convenience function.
Clayton Coleman
> + + // Starts new watch. + // +optional + Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"` + + // Starts new watch. + // +optional + WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"` + + // Stops existing watch. + // +optional + StopWatch *StopWatchOperation `json:"stopWatch,omitempty" protobuf:"bytes,4,opt,name=stopWatch"` + + // Gets list of items. + // +optional + List *ListOperation `json:"list,omitempty" protobuf:"bytes,5,opt,name=list"`
Get is LIST metadata.name=NAME. I don't see any reason to have Get by itself here.
Clayton Coleman
> + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkResponse contains a single response from bulk api. +type BulkResponse struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier propagated from the request. + // Null for messages issued by server (WatchEvent, WatchStopped triggered by server) + // +optional + RequestID *string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"` + + // Contains error details when request was failed. + // +optional + Failure *metav1.Status `json:"failure,omitempty" protobuf:"bytes,2,opt,name=failure"`
We named this Result in admission web hooks. We should be consistent with that pattern unless we have a reason.
Clayton Coleman
> +
+// BulkRequest contains a single request to bulk api.
+type BulkRequest struct {
+ metav1.TypeMeta `json:",inline"`
+
+ // Opaque identifier used to match requests and responses.
+ // +optional
+ RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
+
+ // Starts new watch.
+ // +optional
+ Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"`
+
+ // Starts new watch.
+ // +optional
+ WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"`
I'm not seeing from this why you need anything other than ListOptions. Everything you are doing is covered by either field selectors or the existing options.
Jordan Liggitt
@liggitt commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +
+// BulkRequest contains a single request to bulk api.
+type BulkRequest struct {
+ metav1.TypeMeta `json:",inline"`
+
+ // Opaque identifier used to match requests and responses.
+ // +optional
+ RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
+
+ // Starts new watch.
+ // +optional
+ Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"`
+
+ // Starts new watch.
+ // +optional
+ WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"`
I'm not seeing from this why you need anything other than ListOptions. Everything you are doing is covered by either field selectors or the existing options.
We want to make sure this is able to authorize watches of individual items by name, to allow kubelets to watch individual secrets. If you use field selectors for that, that means special treatment of the metadata.name selector in the authorization check. Not sure if that’s good or bad, just calling it out
Wojciech Tyczynski
@wojtek-t commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +
+// BulkRequest contains a single request to bulk api.
+type BulkRequest struct {
+ metav1.TypeMeta `json:",inline"`
+
+ // Opaque identifier used to match requests and responses.
+ // +optional
+ RequestID string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"`
+
+ // Starts new watch.
+ // +optional
+ Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"`
+
+ // Starts new watch.
+ // +optional
+ WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"`
@smarterclayton - I think you need to be able to specify group-version-kind. So we need something on top of ListOptions.
About @liggitt `s point - I think it a very good point to keep in mind.
I think I would still prefer to not introduce two separate Watch and WatchList operations (we already have some logic in code that is treating "metadata.name" field specially, e.g.:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/selection_predicate.go#L127
), but let's make conscious decision here.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + + // Starts new watch. + // +optional + Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"` + + // Starts new watch. + // +optional + WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"` + + // Stops existing watch. + // +optional + StopWatch *StopWatchOperation `json:"stopWatch,omitempty" protobuf:"bytes,4,opt,name=stopWatch"` + + // Gets list of items. + // +optional + List *ListOperation `json:"list,omitempty" protobuf:"bytes,5,opt,name=list"`
@smarterclayton - agree. But we treat the differently in main api (I mean GET and LIST are different calls with different set of options). Shouldn't we reflect it here too then?
Andrei Zhlobich
@anjensan commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkResponse contains a single response from bulk api. +type BulkResponse struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier propagated from the request. + // Null for messages issued by server (WatchEvent, WatchStopped triggered by server) + // +optional + RequestID *string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"` + + // Contains error details when request was failed. + // +optional + Failure *metav1.Status `json:"failure,omitempty" protobuf:"bytes,2,opt,name=failure"`
This field acts as 'type-of-response' switch (fields Failure/WatchStopped/WatchStarted/... are mutually exclusive).
So it is some sort of "event-type" (corresponds to "previous request was failed").
IMO event/field name should clearly indicate unexpected error.
Also similar field is named 'ResponseStatus' in audit api
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/apis/audit/types.go#L110
It looks much better for our case. Alternatively we can create submessage like:
type OperationFailed struct { // ResponseFailed?
Result *metav1.Status
}
And use OperationFailed instead of metav1.Status
type BulkResponse {
// ...
OperationFailed *OperationFailed
// ...
}
Andrei Zhlobich
> + + // Starts new watch. + // +optional + Watch *WatchOperation `json:"watch,omitempty" protobuf:"bytes,2,opt,name=watch"` + + // Starts new watch. + // +optional + WatchList *WatchListOperation `json:"watchList,omitempty" protobuf:"bytes,3,opt,name=watchList"` + + // Stops existing watch. + // +optional + StopWatch *StopWatchOperation `json:"stopWatch,omitempty" protobuf:"bytes,4,opt,name=stopWatch"` + + // Gets list of items. + // +optional + List *ListOperation `json:"list,omitempty" protobuf:"bytes,5,opt,name=list"`
Also existing client libraries use different signatures for get/list operations.
https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/core/v1/pod.go#L41
Daniel Smith
@lavalamp commented on this pull request.
I didn't review everything but I did go over the API, I'd like to explore alternatives to the system of giant unions.
In cmd/kube-apiserver/app/options/options_test.go:
> @@ -194,6 +194,7 @@ func TestAddFlags(t *testing.T) {
EnableSwaggerUI: true,
EnableProfiling: true,
EnableContentionProfiling: true,
+ EnableBulk: false,
Shouldn't this be a feature flag?
In cmd/kube-apiserver/app/server.go:
> @@ -99,6 +101,11 @@ import (
const etcdRetryLimit = 60
const etcdRetryInterval = 1 * time.Second
+func init() {
+ // FIXME(anjensan): Find better place / install bulk api into separate registry...
+1 please fix :)
In plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go:
> @@ -177,6 +177,10 @@ func ClusterRoles() []rbac.ClusterRole {
"/swagger.json", "/swagger-2.0.0.pb-v1",
"/api", "/api/*",
"/apis", "/apis/*",
+ "/bulk", "/bulk/*",
I'm pretty sure bulk should not be a top-level path. Have you thought of any alternatives?
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkRequest contains a single request to bulk api. +type BulkRequest struct {
ChannelRequest? Some number of these are sent after a long running request (channel) is opened, right?
(Maybe we want a bulk POST in the future. The current name seems overly broad.)
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + StopWatch *StopWatchOperation `json:"stopWatch,omitempty" protobuf:"bytes,4,opt,name=stopWatch"` + + // Gets list of items. + // +optional + List *ListOperation `json:"list,omitempty" protobuf:"bytes,5,opt,name=list"` + + // Gets single item. + // +optional + Get *GetOperation `json:"get,omitempty" protobuf:"bytes,6,opt,name=get"` +} + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+// +genclient:nonNamespaced
+
+// BulkResponse contains a single response from bulk api.
+type BulkResponse struct {
Are "responses" always 1:1 with requests?
How about ClientMessage and ServerMessage as names if not?
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkResponse contains a single response from bulk api. +type BulkResponse struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier propagated from the request. + // Null for messages issued by server (WatchEvent, WatchStopped triggered by server) + // +optional + RequestID *string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"` + + // Contains error details when request was failed. + // +optional + Failure *metav1.Status `json:"failure,omitempty" protobuf:"bytes,2,opt,name=failure"`
Why the union rather than just having the possibility of sending multiple different messages? E.g. the ID field doesn't make sense for everything, at least per the comment on it.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkRequest contains a single request to bulk api. +type BulkRequest struct {
I think Bulk is a fine name for the group, but the resource / operation maybe should be "multiplexed" or something like that? When I think of "bulk" I think of a request body with N chunks of work for the server to do.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> + GroupVersionResource `json:",inline" protobuf:"bytes,1,name=resource"`
+
+ // Namespace
+ // +optional
+ Namespace string `json:"namespace,omitempty" protobuf:"bytes,4,opt,name=namespace"`
+
+ // Query options used to filter watched resources.
+ // Field 'Watch' is ignored by bulk api.
+ // +optional
+ Options *metav1.ListOptions `json:"options" protobuf:"bytes,2,name=options"`
+}
+
+// BulkWatchEvent represents a single event to a watched resource.
+type BulkWatchEvent struct {
+ // Identifier of the watch.
+ WatchID string `json:"watchId" protobuf:"bytes,1,name=watchId"`
Do we need both WatchID and RequestID?
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +package v1alpha1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkRequest contains a single request to bulk api. +type BulkRequest struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier used to match requests and responses. + RequestID string `json:"requestId" protobuf:"bytes,1,name=requestId"`
I think the semantics of this field would be significantly better if we made this object not a union and instead had e.g. StartWatchRequest, StartWatchListRequest, StopWatchRequest, ListRequest, etc., messages.
Because I would like to document "RequestID must be unique" but that's not true if you're sending a StopWatch request that references a past Watch request.
In staging/src/k8s.io/apiserver/pkg/apis/bulk/OWNERS:
> @@ -0,0 +1,17 @@ +reviewers:
Please pare this list down-- many of these people won't know anything about this API. (it's easy enough to add them back!)
Andrei Zhlobich
@anjensan commented on this pull request.
In staging/src/k8s.io/api/bulk/v1alpha1/types.go:
> +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkRequest contains a single request to bulk api. +type BulkRequest struct {
Maybe we want a bulk POST in the future. The current name seems overly broad.
Actually bulk POST is already implemented :)
It is just ~130 additional lines with current architecture (see bulk/http.go), so it doesn't make a lot of sense to not implement it from start.
Andrei Zhlobich
> + StopWatch *StopWatchOperation `json:"stopWatch,omitempty" protobuf:"bytes,4,opt,name=stopWatch"` + + // Gets list of items. + // +optional + List *ListOperation `json:"list,omitempty" protobuf:"bytes,5,opt,name=list"` + + // Gets single item. + // +optional + Get *GetOperation `json:"get,omitempty" protobuf:"bytes,6,opt,name=get"` +} + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkResponse contains a single response from bulk api. +type BulkResponse struct {
Are "responses" always 1:1 with requests?
No. For each request it is exactly 1 response (either success or failure) + some additional responses are not mapped to requests (watch events).
How about ClientMessage and ServerMessage as names if not?
Sounds good to me.
Andrei Zhlobich
> + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkResponse contains a single response from bulk api. +type BulkResponse struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier propagated from the request. + // Null for messages issued by server (WatchEvent, WatchStopped triggered by server) + // +optional + RequestID *string `json:"requestId,omitempty" protobuf:"bytes,1,opt,name=requestId"` + + // Contains error details when request was failed. + // +optional + Failure *metav1.Status `json:"failure,omitempty" protobuf:"bytes,2,opt,name=failure"`
Without union it is harder to serialize/deserialize messages. Some dynamic parsing (like json.RawMessage) might be required do determine type of object. Using of protobufs is also complicated. So
- is it needed to create new serialization/deserialization logic, which might cause some divergences with traditional RESP apis (hidden bugs);
- any tricks around RawMessage introduce additional overhead.
Andrei Zhlobich
> +package v1alpha1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +genclient:nonNamespaced + +// BulkRequest contains a single request to bulk api. +type BulkRequest struct { + metav1.TypeMeta `json:",inline"` + + // Opaque identifier used to match requests and responses. + RequestID string `json:"requestId" protobuf:"bytes,1,name=requestId"`
Because I would like to document "RequestID must be unique" but that's not true if you're sending a StopWatch request that references a past Watch request.
ReqeustID still should be unique in this case. Referring is performed by 'WatchID' instead of RequestID.
I think the semantics of this field would be significantly better if we made this object not a union and instead had e.g. StartWatchRequest, StartWatchListRequest, StopWatchRequest, ListRequest, etc., messages.
I'm don't fully understand what will change with 'RequestID' after switching to separate top-level request/response messages. IMO semantic of RequestID will not change at all.
Andrei Zhlobich
> + GroupVersionResource `json:",inline" protobuf:"bytes,1,name=resource"`
+
+ // Namespace
+ // +optional
+ Namespace string `json:"namespace,omitempty" protobuf:"bytes,4,opt,name=namespace"`
+
+ // Query options used to filter watched resources.
+ // Field 'Watch' is ignored by bulk api.
+ // +optional
+ Options *metav1.ListOptions `json:"options" protobuf:"bytes,2,name=options"`
+}
+
+// BulkWatchEvent represents a single event to a watched resource.
+type BulkWatchEvent struct {
+ // Identifier of the watch.
+ WatchID string `json:"watchId" protobuf:"bytes,1,name=watchId"`
Yes, we need both.
WatchID might be used by client to map "watch options" & watch events.
RequestID is used to map async requests & responses (it is possible to send a new request before server answered to the previous one).
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: anjensan
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
Assign the PR to them by writing /assign @lavalamp in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Needs approval from an approver in each of these files:Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
k8s-ci-robot
@anjensan: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-device-plugin-gpu | 2158982 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-device-plugin-gpu | 2158982 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-kubemark-e2e-gce | 2158982 | link | /test pull-kubernetes-kubemark-e2e-gce |
| pull-kubernetes-e2e-kops-aws | 2158982 | link | /test pull-kubernetes-e2e-kops-aws |
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-device-plugin-gpu | 2158982 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-kubemark-e2e-gce | 2158982 | link | /test pull-kubernetes-kubemark-e2e-gce |
| pull-kubernetes-e2e-kops-aws | 2158982 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gce | 2158982 | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-bazel-build | 2158982 | link | /test pull-kubernetes-bazel-build |
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-e2e-gce-device-plugin-gpu | 2158982 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-kubemark-e2e-gce | 2158982 | link | /test pull-kubernetes-kubemark-e2e-gce |
| pull-kubernetes-e2e-kops-aws | 2158982 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gce | 2158982 | link | /test pull-kubernetes-e2e-gce |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-device-plugin-gpu | 2158982 | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-kubemark-e2e-gce | 2158982 | link | /test pull-kubernetes-kubemark-e2e-gce |
| pull-kubernetes-e2e-kops-aws | 2158982 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gce | 2158982 | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-node-e2e | 2158982 | link | /test pull-kubernetes-node-e2e |
k8s-ci-robot
@anjensan: PR needs rebase.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
—
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-unit | 5627fe8 | link | /test pull-kubernetes-unit |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: anjensan
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
Assign the PR to them by writing /assign @lavalamp in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Needs approval from an approver in each of these files:- api/OWNERS
- cmd/kube-apiserver/OWNERS
- hack/OWNERS
- pkg/OWNERS
- plugin/pkg/auth/OWNERS
- staging/OWNERS
- test/OWNERS
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
—
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-unit | dcc71c3 | link | /test pull-kubernetes-unit |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
Tim Hockin
This change is
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce | 6d2aff9 | link | /test pull-kubernetes-e2e-gce |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
Andrei Zhlobich
@anjensan commented on this pull request.
In cmd/kube-apiserver/app/server.go:
> @@ -99,6 +101,11 @@ import (
const etcdRetryLimit = 60
const etcdRetryInterval = 1 * time.Second
+func init() {
+ // FIXME(anjensan): Find better place / install bulk api into separate registry...
Unfortunately I didn't found a better place for it :)
It is impossible to put such line into apiserver/pkg/apis/bulk/install/install.go due to cycle dependency (legacyscheme lives in /pkg/kubernetes & apiserver can't use packages from).
Do you have any ideas / suggestions?
Jordan Liggitt
@liggitt commented on this pull request.
In cmd/kube-apiserver/app/server.go:
> @@ -99,6 +101,11 @@ import (
const etcdRetryLimit = 60
const etcdRetryInterval = 1 * time.Second
+func init() {
+ // FIXME(anjensan): Find better place / install bulk api into separate registry...
this is a logically separate component, just like the custom resource server and the kube aggregator. those have their own APIs, live in distinct packages (staging/src/k8s.io/kube-aggregator and staging/src/k8s.io/apiextensions-apiserver), and are combined in https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-apiserver/app/server.go#L145
Andrei Zhlobich
@anjensan commented on this pull request.
In cmd/kube-apiserver/app/server.go:
> @@ -99,6 +101,11 @@ import (
const etcdRetryLimit = 60
const etcdRetryInterval = 1 * time.Second
+func init() {
+ // FIXME(anjensan): Find better place / install bulk api into separate registry...
I don't think it should be a separate component, it looks more like additional transportation/serialization layer (like HTTP, HTTPS, SPDY...)
Jordan Liggitt
@liggitt commented on this pull request.
In plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go:
> @@ -186,6 +186,10 @@ func ClusterRoles() []rbac.ClusterRole {
"/openapi", "/openapi/*",
"/api", "/api/*",
"/apis", "/apis/*",
+ "/bulk", "/bulk/*",
why is this a new top-level context root? I expected these operations to live within an API group
Jordan Liggitt
@liggitt commented on this pull request.
In cmd/kube-apiserver/app/server.go:
> @@ -425,7 +432,7 @@ func BuildGenericConfig(s *options.ServerRunOptions, proxyTransport *http.Transp
genericConfig.SwaggerConfig = genericapiserver.DefaultSwaggerConfig()
genericConfig.EnableMetrics = true
genericConfig.LongRunningFunc = filters.BasicLongRunningRequestCheck(
- sets.NewString("watch", "proxy"),
+ sets.NewString("watch", "proxy", "bulk"),
I would not expect bulk to be a verb
Jordan Liggitt
> @@ -99,6 +101,11 @@ import (
const etcdRetryLimit = 60
const etcdRetryInterval = 1 * time.Second
+func init() {
+ // FIXME(anjensan): Find better place / install bulk api into separate registry...
the distinct API group and expectation that API servers other than kube-apiserver could use this mechanism to allow bulk operations on their own resources indicates to me it is a separable component.
it looks like you tried to accomplish that by adding to k8s.io/apiserver, but it will be cleaner to follow the pattern of the custom resource server, containing the API and special behaviors into their own component, then install that into the kube-apiserver.
@deads2k can provide guidance here.
David Eads
@deads2k can provide guidance here.
To make this easier to reason about and maintain, we should build it as a separate logical unit like we did for api aggregation and CRDs. That structure enforces very strong dependency discipline and prevents entanglement.
Since you're able to create delegated API servers, it doesn't prevent us from including it in the kube-apiserver, but the benefits of separation and independent testability are significant.
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: anjensan
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
Assign the PR to them by writing /assign @lavalamp in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Needs approval from an approver in each of these files:Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
—
Andrei Zhlobich
@anjensan commented on this pull request.
In cmd/kube-apiserver/app/server.go:
> @@ -99,6 +101,11 @@ import (
const etcdRetryLimit = 60
const etcdRetryInterval = 1 * time.Second
+func init() {
+ // FIXME(anjensan): Find better place / install bulk api into separate registry...
Unfortunately I didn't found a better place for it :)
I've fixed original FIXME by moving bulk.{Server,Client}Message objects into a separate registry.
Actually everything was already ready for it :) --
embedded objects (inside WatchEvent/GetResult/ListResult) are still serialized using per-group serializers (taken from genericapiserver.APIGroupInfo).
the distinct API group
IMO semantically it shouldn't be a separate group. It is closer to v1.WatchEvent. And it contains 0 resources (both ServerMessage/ClientMessage are not resources).
But it is a good idea to move it into a separate group just because it is highly experimental and it easier to evolve such api (alpha -> beta and so).
expectation that API servers other than kube-apiserver could use this mechanism to allow bulk operations
Ideally external API servers need to do nothing in order to enable bulk operations.
Exactly like they don't need to do anything to provide regular 'watch' (http-based).
I've just performed some basic manual testing - it is now possible to watch/list CustomResourceDefinitions through bulk api without any modifications in 'apiextensions-server'.
Are they served from 'apiextensions-server'?
containing the API and special behaviors into their own component
The main complication here is that bulk-api should not introduce any "special" behaviors.
It just adds another transport protocol to work with existing apigroups.
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-integration | 1b8f96a | link | /test pull-kubernetes-integration |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
Jordan Liggitt
@liggitt commented on this pull request.
In cmd/kube-apiserver/app/server.go:
> @@ -99,6 +101,11 @@ import (
const etcdRetryLimit = 60
const etcdRetryInterval = 1 * time.Second
+func init() {
+ // FIXME(anjensan): Find better place / install bulk api into separate registry...
By definition, it is a new API that does not belong in any other group (because via it you can interact with multiple other groups). That means new endpoints, which don't belong to any existing API group.
Ideally external API servers need to do nothing in order to enable bulk operations.
If something else (like the aggregator) is handling the bulk APIs, sure, but this should be structured in a way that a stand-alone server could serve them as well.
And it contains 0 resources
The top level types submitted to the API are resources.
Andrei Zhlobich
Just to clarify.
Right now apiserver chain initialization structure looks like (pseudocode)
server1 := createAPIServer1(... some config etc ...)
server1.GenericAPIServer.InstallAPIGroup(groupA) // serializer, schema, storage config etc
server1.GenericAPIServer.InstallAPIGroup(groupB)
server2 := createAPIServer2Delegate( server1, ... some config etc ... )
server2.GenericAPIServer.InstallAPIGroup(groupC)
server3 := createAPIServer2Delegate( server2, ... some config etc ... )
server3.GenericAPIServer.InstallAPIGroup(groupD)
Of course calls to 'InstallAPIGroups' usually are hidden somewhere deeper (inside createAPIServerX etc). You are suggesting to extract all bulk-related logic into a separate api server. But it still need access to 'groupInfo' (as it reuses serializer/storage etc). So initialization will look like
server1 := createAPIServer1(... some config etc ...)
server1.GenericAPIServer.InstallAPIGroup(groupA) // serializer, schema, storage config etc
server1.GenericAPIServer.InstallAPIGroup(groupB)
server1b := createAPIServer1(server1, ... the same config ...)
server1b.GenericAPIServer.InstallAPIGroupBulk(groupA) // serializer, schema, storage config etc
server1b.GenericAPIServer.InstallAPIGroupBulk(groupB)
server2 := createAPIServer2Delegate( server1b, ... some config etc ... )
server2.GenericAPIServer.InstallAPIGroup(groupC)
server2b := createAPIServer2Delegate( server2, ... the same config ... )
server2b.GenericAPIServer.InstallAPIGroup(groupC)
server3 := createAPIServer2Delegate( server2b, ... some config etc ... )
server3.GenericAPIServer.InstallAPIGroup(groupD)
server3b := createAPIServer2Delegate( server3, ... the same config ... )
server3b.GenericAPIServer.InstallAPIGroupBulk(groupD)
So we'll need to double a number of api-serves in the chain + register all api-groups into two apiservers (one for http-rest and another for bulk). And of course configs for all pairs of 'apiserver' & 'bulk-apiserver' should be similar because any divergences in behavior of 'bulk' & regular http-rest may cause issues. Is it correct?
Jordan Liggitt
No, the bulk server will route to the existing servers via an opaque client (in this case, likely the loopback client for the apiserver). The bulk server will not be tightly coupled to the resources it is exposing. It should work equally well for kube API types, custom resources, and remote extensions API server resources.
Andrei Zhlobich
@anjensan commented on this pull request.
In cmd/kube-apiserver/app/server.go:
> @@ -99,6 +101,11 @@ import (
const etcdRetryLimit = 60
const etcdRetryInterval = 1 * time.Second
+func init() {
+ // FIXME(anjensan): Find better place / install bulk api into separate registry...
if something else (like the aggregator) is handling the bulk APIs,
Aggregator already have special handling for bulk. It is part of this PR actually.
Main reason for this -- aggregator should multiplex several websockets (as websocket clients usually can open only 1 connection due to browser policies etc).
stand-alone server could serve them as well.
Do you mean "a separate server based on genericapiserver"?
The top level types submitted to the API are resources.
Hm, maybe I was understanding/using the term "resources" a bit incorrectly.
I was though it is something like "entity" which have a separate REST path etc.
For bulk API both bulk.ClientMessage and bulk.ServerMessage are not entities. They should behave exactly like 'GetOptions' / 'WatchEvent'. Maybe I'm registering them incorrectly.
Andrei Zhlobich
No, the bulk server will route to the existing servers via an opaque client
So it ruins initial purpose of such API :) -- don't create a lot of http connections / requests when you need to watch several resources.
Andrei Zhlobich
@anjensan commented on this pull request.
In plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go:
> @@ -186,6 +186,10 @@ func ClusterRoles() []rbac.ClusterRole {
"/openapi", "/openapi/*",
"/api", "/api/*",
"/apis", "/apis/*",
+ "/bulk", "/bulk/*",
Bulk is quite "special" api group. It doesn't contain any entities.
It doesn't support any existing verbs (get/list/watch etc).
We will start with introducing a new dedicated API group:
/apis/bulk.k8s.io/
I don't think it is good idea to serve wssocket on 'apis/bulk.k8s.io'.
Maybe we can use '/apis/bulk.k8s.io/connect' (or something similar) where 'connect' is a fake resource (list/get/watch/etc are not supported). But I'm not sure that it will not break any existing clients/logic. Also It should be handled differently in aggregator (as it just proxies all requests to another apiserver, which is not enough for bulk). So some 'if path == "bulk.k8s.io" need to be added... Looks a bit hacky and unmaintainable.
Anyway I don't like to introduce any new top-level path but don't know a better place for it.
So I open for any suggestions :)
k8s-ci-robot
@anjensan: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-integration | 8236569 | link | /test pull-kubernetes-integration |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
k8s-ci-robot
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: anjensan
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
Assign the PR to them by writing /assign @lavalamp in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Needs approval from an approver in each of these files:Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
—
Andrei Zhlobich
/assign @lavalamp
Andrei Zhlobich
To summarize comments:
Watch+WatchList VS Watch
It was suggested to merge 'Watch' and 'WatchList'.
Should additional field 'Name' (resource name) be added into WatchOperation?
Or client needs manually specify 'selector.metadata.name = ..' (some extra code should be added into authorization check)?
Get+List VS just List
Should we drop 'Get' operation completely or leave as it is?
The same question regarding 'Name' - optional 'Name' field or 'selector.metadata.name' ?
BigUnion VS separate top-level messages
IMO there are cons/pros for both variants.
With BigUnion server/client code is a bit cleaner (chan bulk.ServerMessage instead of chan interface{}, easier to serialize etc)
Learning of api might be a bit easier (it is possible to figure out full set of operations by just looking into bulk.ClientMessage).
Alternative aproach with separate top-level messages make api a bit prettier and cleaner.
So, what do think?
Url for bulk api
Should we expose api on 'apis/bulk.k8s.io/v1alpah1'?
Or 'apis/bulk.k8s/io/v1alpha1/wsocket' or similar?
WDYT?
Bultin bulk staff into apiserver VS separate handler in the chain
@deads2k @liggitt
One of reasons for bulk api to exists is intention to reduce number of parallel connections to the apiserver.
For example client may need to watch 100 items (secrets etc) at the same time and it is quite
costly to maintain 100 connections for that. So I'm not sure that 'opaque client' will work.
Do you mean something like https://github.com/novnc/websockify ?
Daniel Smith
Haven't had a chance to review this yet, I'm sorry. It is on my list, but my list is really long. I do notice:
For example client may need to watch 100 items (secrets etc) at the same time and it is quite costly to maintain 100 connections for that.
I think this isn't true any more given that we use HTTP/2 by default?
Andrei Zhlobich
I think this isn't true any more given that we use HTTP/2 by default?
Hm, this sounds cool! But in such case BulkAPI is quite less useful :)
At least implementation proposed by @liggitt (where 'opaque client ' == 'client-go' or any other http2-based client, please correct me if I misunderstood you).
I'm not sure how we can improve scalability (main intention from original proposal) by putting additional agent into communication chain -- kubelets communicate with bulkapi (websockets) & bulkapi communicates with apiservers by using existing protocol (http2-based-rest). Direct chain "kubelets communicate with apiservers (http2-based-rest)" will be faster/more-scalable in any case. OR bulkapi should use some different approach to connection with apiservers (in-process communication, grpc, etc.)
k8s-ci-robot
@anjensan: PR needs rebase.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
—
fejta-bot
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale
fejta-bot
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten