Fixes #82 Add a client for the pod read-log endpoint #83

dbolkensteyn · 2018-08-15T19:23:17Z

This PR adds the ability to read the logs of a pod. Fixes #82

k8s-ci-robot · 2018-08-15T19:23:29Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

dbolkensteyn · 2018-08-15T19:42:30Z

node-client/src/web-socket-handler.ts

@@ -87,7 +88,7 @@ export class WebSocketHandler {
                            textHandler(message.utf8Data);
                        }
                    } else if (message.type === 'binary') {
-                        if (binaryHandler) {
+                        if (binaryHandler && message.binaryData.length > 0) {


On my Kubernetes cluster, the first message received is an empty one.

dbolkensteyn · 2018-08-15T19:44:29Z

node-client/src/log.ts

+        const promise = this.handler.connect(path, null, (streamNum: number, buff: Buffer) => {
+            const charBuff = Buffer.allocUnsafe(1)
+            charBuff.writeInt8(streamNum, 0)
+            stream.write(Buffer.concat([charBuff, buff]));


Little bit ugly trick, but this allows to re-use the code from WebSocketHandler without having to it in a way that could break the other consumers of it (Attach, Exec).

A single write() call is issued on the stream because that tends to correspond to a line in the logs (although this is probably not guaranteed).

Sorry can you explain why this is needed? This is probably a bug in WebSocketHandler that should be fixed instead of doing this here...

(and it's probably my bug :)

dbolkensteyn · 2018-08-16T16:03:17Z

There is a flaky error while reading the logs:

internal/buffer.js:35
    throw new ERR_OUT_OF_RANGE('value', `>= ${min} and <= ${max}`, value);
    ^

RangeError [ERR_OUT_OF_RANGE]: The value of "value" is out of range. It must be >= 0 and <= 4294967295. Received -2003205171
    at checkInt (internal/buffer.js:35:11)
    at writeU_Int32BE (internal/buffer.js:625:3)
    at Buffer.writeUInt32BE (internal/buffer.js:638:10)
    at WebSocketFrame.toBuffer (/Users/dinesh/Desktop/projects/javascript/node-client/node_modules/websocket/lib/WebSocketFrame.js:257:24)

This is already fixed upstream in this commit theturtle32/WebSocket-Node@12ed73d

All we need is to update to the latest release 1.0.26, which indeed solves this problem.

dbolkensteyn · 2018-08-16T16:52:01Z

node-client/package.json

@@ -38,7 +38,7 @@
    "request": "^2.72.0",
    "shelljs": "^0.7.8 ",
    "underscore": "^1.8.3",
-    "websocket": "^1.0.25"
+    "websocket": "^1.0.26"


The diff on the auto-generated package-lock.json is pretty huge, unsure what to do with that, so I'm excluding it for the time being from this PR

brendandburns · 2018-08-21T04:04:39Z

node-client/src/log.ts

+    }
+
+    // TODO add support for limitBytes, previous, sinceSeconds and tailLines
+    public log(namespace: string, podName: string, containerName: string,


indentation is weird here, can you run this through tslint?

brendandburns · 2018-08-21T04:10:09Z

node-client/src/log.ts

+    public 'handler': WebSocketHandler;
+
+    public constructor(config: KubeConfig) {
+        this.handler = new WebSocketHandler(config);


Does the log endpoint even use WebSockets? I thought it was a standard hanging GET, that's what we do in Java:

https://github.com/kubernetes-client/java/blob/master/util/src/main/java/io/kubernetes/client/PodLogs.java#L65

brendandburns · 2018-08-21T04:10:54Z

Thanks for the PR!

Do we really need WebSockets here? I think in other clients this is implemented as a straight GET...

Thanks!

brendandburns · 2018-08-28T04:39:13Z

@dbolkensteyn friendly ping on this, I'd love to get it merged, but I have some thoughts.

Thanks!

dbolkensteyn · 2018-08-28T15:52:41Z

And I thank you for the valuable feedback @brendandburns - I'll come back to this during the week.

brendandburns · 2018-09-10T02:58:21Z

Friendly ping on this if you still have time I'd love to get this merged.

Thanks!

brendandburns · 2018-10-09T15:41:22Z

@dbolkensteyn Friendly ping on this.

Thanks!

brendandburns · 2019-02-07T06:54:02Z

Final ping, otherwise I'm going to close and build a different implementation of this functionality.

Thanks!

dbolkensteyn · 2019-03-19T23:29:09Z

Sorry for the late follow-up @brendandburns , I've just forced-pushed a new implementation that incorporates all of your feedback and the one from #214 which actually implements the same feature (but hard-codes follow to true)

dbolkensteyn · 2019-03-20T00:17:25Z

src/log.ts

+            }
+        }).on('response', (response) => {
+            if (response.statusCode === 200) {
+                req.pipe(stream);


unlike #214 this will only start to pipe the "logs" when a 200 response is returned (else Kubernetes error jsons might get treated as logs)

@dbolkensteyn How to stop piping? I am successfully able to open the logs (follow option) but what is the right way to terminate underlying request?

dbolkensteyn · 2019-03-20T00:17:47Z

src/log.ts

+    public log(
+        namespace: string,
+        podName: string,
+        containerName: string,


containerName is mandatory

dbolkensteyn · 2019-03-20T00:18:06Z

src/log.ts

+    /**
+     * Follow the log stream of the pod. Defaults to false.
+     */
+    follow?: boolean;


follow is taken as a parameter and not hard-coded

dbolkensteyn · 2019-03-20T00:18:43Z

src/log.ts

+
+import { KubeConfig } from './config';
+
+export interface LogOptions {


This is taken over from #214 as it seems as a nice way to avoid overly-parameterized methods

brendandburns · 2019-03-27T22:06:35Z

/lgtm
/approve

k8s-ci-robot · 2019-03-27T22:06:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, dbolkensteyn

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:

~~OWNERS~~ [brendandburns]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abierbaum · 2020-10-27T22:07:42Z

Does anyone have an example of using this code? I tried to use it to stream logs but have not gotten it to work.

izhilenkov · 2020-11-12T03:11:44Z

@abierbaum You could use something like LineStream from byline (which exactly Transform) as a line stream for quick example

import { KubeConfig, Log } from '@kubernetes/client-node';
import { LineStream } from 'byline';

const kc = new KubeConfig();
kc.loadFromDefault();

const log = new Log(kc);
const logStream = new LineStream();

logStream.on('data', data => console.log(data.toString()));

log.log(namespace, pod, container, logStream, (err) => console.log(err), { follow: true });

kingdevnl · 2020-12-29T15:29:17Z

How could i close the log?

Iv'e tried to call logStream.destroy() but the program still keeps running.

Celthi · 2021-03-25T13:21:20Z

Hi guys, I use this class with follow=true but my file only has part of the log instead of all the log from the pod?
Is there anything I need to pay attention to use this class?

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 15, 2018

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 15, 2018

dbolkensteyn force-pushed the read-log branch from 78281b8 to 38d0e7e Compare August 15, 2018 19:28

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 15, 2018

dbolkensteyn commented Aug 15, 2018

View reviewed changes

dbolkensteyn force-pushed the read-log branch 3 times, most recently from 47d5225 to 4e8676c Compare August 15, 2018 20:32

dbolkensteyn commented Aug 16, 2018

View reviewed changes

brendandburns reviewed Aug 21, 2018

View reviewed changes

dbolkensteyn force-pushed the read-log branch 2 times, most recently from 428bff4 to a649a15 Compare March 19, 2019 23:24

dbolkensteyn force-pushed the read-log branch from a649a15 to b00737d Compare March 19, 2019 23:49

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 19, 2019

dbolkensteyn force-pushed the read-log branch from b00737d to a091fb6 Compare March 20, 2019 00:09

Fixes kubernetes-client#82 Add a client for the pod read-log endpoint

1d18f90

dbolkensteyn force-pushed the read-log branch from a091fb6 to 1d18f90 Compare March 20, 2019 00:10

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 20, 2019

dbolkensteyn commented Mar 20, 2019

View reviewed changes

k8s-ci-robot assigned brendandburns Mar 27, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2019

brendandburns mentioned this pull request Mar 27, 2019

Added FollowLogs class #214

Closed

k8s-ci-robot merged commit 4a5b84d into kubernetes-client:master Mar 27, 2019

popcornylu mentioned this pull request Aug 3, 2020

readNamespacedPodLog with follow=true #110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #82 Add a client for the pod read-log endpoint #83

Fixes #82 Add a client for the pod read-log endpoint #83

dbolkensteyn commented Aug 15, 2018 •

edited

Loading

k8s-ci-robot commented Aug 15, 2018

dbolkensteyn Aug 15, 2018

dbolkensteyn Aug 15, 2018

brendandburns Aug 21, 2018

dbolkensteyn commented Aug 16, 2018 •

edited

Loading

dbolkensteyn Aug 16, 2018

brendandburns Aug 21, 2018

brendandburns Aug 21, 2018

brendandburns commented Aug 21, 2018

brendandburns commented Aug 28, 2018

dbolkensteyn commented Aug 28, 2018

brendandburns commented Sep 10, 2018

brendandburns commented Oct 9, 2018

brendandburns commented Feb 7, 2019

dbolkensteyn commented Mar 19, 2019 •

edited

Loading

dbolkensteyn Mar 20, 2019

tettusud Jun 27, 2020

dbolkensteyn Mar 20, 2019 •

edited

Loading

dbolkensteyn Mar 20, 2019

dbolkensteyn Mar 20, 2019

brendandburns commented Mar 27, 2019

k8s-ci-robot commented Mar 27, 2019

abierbaum commented Oct 27, 2020

izhilenkov commented Nov 12, 2020

kingdevnl commented Dec 29, 2020

Celthi commented Mar 25, 2021


		import { KubeConfig } from './config';

		export interface LogOptions {

Fixes #82 Add a client for the pod read-log endpoint #83

Fixes #82 Add a client for the pod read-log endpoint #83

Conversation

dbolkensteyn commented Aug 15, 2018 • edited Loading

k8s-ci-robot commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolkensteyn commented Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Aug 21, 2018

brendandburns commented Aug 28, 2018

dbolkensteyn commented Aug 28, 2018

brendandburns commented Sep 10, 2018

brendandburns commented Oct 9, 2018

brendandburns commented Feb 7, 2019

dbolkensteyn commented Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolkensteyn Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Mar 27, 2019

k8s-ci-robot commented Mar 27, 2019

abierbaum commented Oct 27, 2020

izhilenkov commented Nov 12, 2020

kingdevnl commented Dec 29, 2020

Celthi commented Mar 25, 2021

dbolkensteyn commented Aug 15, 2018 •

edited

Loading

dbolkensteyn commented Aug 16, 2018 •

edited

Loading

dbolkensteyn commented Mar 19, 2019 •

edited

Loading

dbolkensteyn Mar 20, 2019 •

edited

Loading