package log runner as part of Kubernetes releases #106123
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
pohly
force-pushed
the
log-go-runner-windows
branch
from
e446343
to
89c36a2
Compare
k8s-ci-robot
added
release-note
|
/test pull-kubernetes-e2e-windows-gce |
cmd/kube-runner/go-runner.go
Outdated
| @@ -0,0 +1,123 @@ | |||
| /* | |||
| Copyright 2020 The Kubernetes Authors. | |||
There was a problem hiding this comment.
This is the original go-runner.go from @dims. I kept the file name, but perhaps I should better rename also it to kube-runner.go?
|
We should really have this de-duped vs https://github.com/kubernetes/release/tree/master/images/build/go-runner I don't think it makes sense to have two versions of this, maybe we can just publish the one in kubernetes/release somewhere and optionally also include that in our release tarballs? See also: kubernetes/release#2301 cc @serathius |
Agreed. But before worrying about how to do that, I wanted to first check that:
"Publish the one in kubernetes/release" - do you mean source code or binaries? My gut feeling is that sharing source code via go.mod dependencies might be simpler, but I haven't tried it. |
|
Also, everybody's favorite discussion: what should be the name of the tool, given that |
A binary, right now I think only the distroless based image is published but we could publish a binary as well (and potentially re-publish it with kubernetes releases) One thing we have to be careful of: These things are an API in the images they wind up in where users may explicitly set the entrypoint, so at the very least if we pick a new name for either of these we need to add a symlink in the images. I'm not sure what's best here. I wish we'd kept more of this in-repo and avoided the duplicate naming to begin with 🙃 |
I'm currently leaning the other way: let's publish the source code as part of k/k and then pick that up in k/release via Downloading a precompiled binary sounds like it will be more work. Here's a more complete proposal:
|
|
Not having a |
|
I think this worked as intended. https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/106123/pull-kubernetes-e2e-windows-gce/1455925818994724864/artifacts/e2e-7d7d9eefc0-95646-windows-node-group-f4fn/serial-1.log shows that kubelet and kube-proxy get started via kube-runner. |
kube-log-runner was formerly known as go-runner when it was originally introduced in kubernetes@393e095 It was moved to kubernetes/release/images/build/go-runner later but is now needed again in Kubernetes itself as replacement for the deprecated --log-file klog feature: when bringing up a Windows node, kube-proxy.exe and kubelet.exe must be wrapped with the helper binary to redirect output. It got renamed to avoid a naming conflict with test/conformance/image/go-runner and because the name was too vague. Other downstream Kubernetes users may have a similar need, therefore it makes sense to provide a prebuilt binary also in the release archives.
pohly
force-pushed
the
log-go-runner-windows
branch
from
89c36a2
to
d2a8a81
Compare
|
/test pull-kubernetes-e2e-windows-gce |
k8s-ci-robot
added
sig/api-machinery
k8s-ci-robot
added
triage/accepted
|
I think we should get agreement from more folks first, so we don't keep going in circles on this one. |
|
Yes, let's keep this open. @dims will have time to comment on Monday. FWIW, the PR currently implements my proposal above (kube-log-runner as name, hosted in component-base/logs). |
|
/milestone v1.23 |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@vishh: may I have your approval for the |
|
/hold Let's make sure we have sufficient feedback (@dims, @BenTheElder?) before merging. |
k8s-ci-robot
added
the
do-not-merge/hold
|
/assign @vishh |
|
/retest yes, +1 to ask @BenTheElder to peek as well. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, pohly, serathius 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 |
k8s-ci-robot
added
the
approved
|
@pohly: The following test failed, say
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. |
|
/kind feature |
k8s-ci-robot
added
kind/feature
|
Looks like @BenTheElder is out and we should get this merged before the code freeze, so: /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What type of PR is this?
/kind feature
What this PR does / why we need it:
kube-log-runner was formerly known as go-runner when it was originally introduced
in #90804.
It was moved to kubernetes/release/images/build/go-runner later but is now
needed again in Kubernetes itself as replacement for the deprecated --log-file
klog feature: when bringing up a Windows node, kube-proxy.exe and kubelet.exe must be wrapped
with the helper binary to redirect output.
It got renamed to avoid a naming conflict with
test/conformance/image/go-runner.
Other downstream Kubernetes users may have a similar need, therefore it makes
sense to provide a pre-built binary.
Which issue(s) this PR fixes:
Fixes #106101
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: