Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Forking glog into third_party/ directory #69333

Closed

Conversation

dims
Copy link
Member

@dims dims commented Oct 2, 2018

Change-Id: I9756b73c806f8e29edabae3999c06d4083a6527b

What this PR does / why we need it:

  • Move the glog directory to third_party/forked
  • patch up godep-save.sh to make sure links are created
  • patch up hack/update-bazel.sh so BUILD is correct
  • patch up update-godep-licenses.sh so LICENSES get generated correctly
  • patch hack/verify-pkg-names.sho to skip the forked glog directory

Forking glog is a first step to essentially get rid of it, please see discussion in #61006 and elsewhere on why glog id problematic

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 2, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 2, 2018
@dims dims force-pushed the try-forking-glog-into-third-party-directory branch 2 times, most recently from bdf8d15 to bcad164 Compare October 2, 2018 18:27
@dims
Copy link
Member Author

dims commented Oct 2, 2018

/test pull-kubernetes-bazel-test

@dims dims force-pushed the try-forking-glog-into-third-party-directory branch from bcad164 to a3693a4 Compare October 2, 2018 20:50
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 2, 2018
@dims
Copy link
Member Author

dims commented Oct 2, 2018

related to #61006

@neolit123
Copy link
Member

would be quite nice if this works! 👍

@dims
Copy link
Member Author

dims commented Oct 3, 2018

@neolit123 well, all the tests are running fine, couple of small things in verify that can be dealt with. For sure, i'd like someone like @obeattie, @alexellis or others who were actively advocating the fork in #61006 to pick up the torch from this experiment and run with it.

@tallclair
Copy link
Member

@dims I'd like to be able to take advantage of the changes we make in non-k/k repos (in particular, to fix all the container images that require a shell just to redirect the logs). Will it be possible to vendor the fork from here, or should we consider putting it elsewhere (perhaps it's own repo?)

@dims
Copy link
Member Author

dims commented Oct 3, 2018

@tallclair my thought was to iterate here in k/k first, then put the fork in its own repo and get the projects in our dependencies to switch over.

for example, we need to take care of problems with say "log_dir" causing issues when there are multiple copies of the code.

@dims dims force-pushed the try-forking-glog-into-third-party-directory branch from a3693a4 to eae5faf Compare October 14, 2018 16:04
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 14, 2018
@dims dims changed the title [WIP] Try forking glog into third_party/ directory Forking glog into third_party/ directory Oct 14, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2018
@dims dims force-pushed the try-forking-glog-into-third-party-directory branch from eae5faf to 93b3a74 Compare October 15, 2018 00:30
@k8s-ci-robot k8s-ci-robot added area/apiserver area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Oct 19, 2018
- fork code into third_party/forked/glog/ directory
- patch up hack/godep-save.sh to make sure links are created
- patch hack/verify-pkg-names.sho to skip the forked glog directory
- link vendor/github.com/golang/glog/glog.go and glog_file.go from the
  forked directory

Change-Id: Ife852c6e8f868ba157a4d6c3dc7c66c1bfac91e7
- Move all flag initialization to a single init() block
- add a new option `log-file`
- Avoid creating directories when `log-file` is specified

Change-Id: I04ef706af9260902aacf43a38ed75ba06df1449d
Change-Id: Id5a97c61d5f611959a284e1fa0ca79091495dd02
Change-Id: I95f1bbdeaf9c993c704a7984a4ad4770436cc8fe
Change-Id: I010ebb01483bfb53b532646047d959ce6c7d0579
Change-Id: Iecf92407333123d6427476edcd093b9df99fc3e8
@dims dims force-pushed the try-forking-glog-into-third-party-directory branch 5 times, most recently from 9b71ee1 to 502a9ca Compare October 25, 2018 14:17
@dims
Copy link
Member Author

dims commented Oct 25, 2018

/test pull-kubernetes-integration

@dims
Copy link
Member Author

dims commented Oct 25, 2018

/test pull-kubernetes-e2e-kubeadm-gce

@dims dims force-pushed the try-forking-glog-into-third-party-directory branch from 502a9ca to 98b08a7 Compare October 25, 2018 16:23
else
kube::log::status "creating symbolic link to forked $file"
rm -f "vendor/github.com/golang/glog/$file" || true
ln -s "../../../../third_party/forked/glog/$file" "vendor/github.com/golang/glog/$file"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't we just link the directory, like we do for staging stuff

@@ -81,6 +81,17 @@ for repo in $(ls staging/src/k8s.io); do
fi
done

for file in "README" "LICENSE" "glog.go" "glog_file.go"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs comments

@@ -26,7 +26,7 @@ source "${KUBE_ROOT}/hack/lib/init.sh"
kube::golang::verify_go_version

cd "${KUBE_ROOT}"
if git --no-pager grep -E $'^(import |\t)[a-z]+[A-Z_][a-zA-Z]* "[^"]+"$' -- '**/*.go' ':(exclude)vendor/*' ':(exclude)**.*.pb.go'; then
if git --no-pager grep -E $'^(import |\t)[a-z]+[A-Z_][a-zA-Z]* "[^"]+"$' -- '**/*.go' ':(exclude)vendor/*' ':(exclude)**.*.pb.go' ':(exclude)third_party/forked/glog/*'; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs comments

limitations under the License.
*/

package glog // import "k8s.io/third_party/forked/glog"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really the canonical import path we want? I don't understand.

if *logDir != "" {
logDirs = append(logDirs, *logDir)
// if log-file is not specified, only then try to use logDir
if logging.logFile == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing this as a switch would maybe make it easier to read

logDirs = append(logDirs, *logDir)
// if log-file is not specified, only then try to use logDir
if logging.logFile == "" {
// if log-dir is not specified, then use temp directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty deep change - it changes "log to all dirs" into "log to one dir", if I read correctly. Why bother to keep logDirs as a plural at this point?

@@ -677,6 +677,42 @@ func (l *loggingT) printWithFileLine(s severity, file string, line int, alsoToSt
l.output(s, buf, file, line, alsoToStderr)
}

type redirectBuffer struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments

@@ -27,19 +28,20 @@ import (

var logFlushFreq = pflag.Duration("log-flush-frequency", 5*time.Second, "Maximum number of seconds between log flushes")

// GlogWriter serves as a bridge between the standard log package and the glog package.
type GlogWriter struct{}
// ErrorLogWriter serves as a bridge between the standard log package and the glog package.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better or worse to set --log-file=/dev/stderr instead of this? It seems less invasive and maybe we could drop the SetOutput stuff ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In doing it this way, do you lose teh formatted prefix (timestamp, etc) on each line? Or if not, and you send to another logger, do you log two timestamps?

@@ -28,17 +30,18 @@ import (

var logFlushFreq = pflag.Duration("log-flush-frequency", 5*time.Second, "Maximum number of seconds between log flushes")

// ErrorLogWriter serves as a bridge between the standard log package and the glog package.
type ErrorLogWriter struct{}
// SysLogWriter serves as a bridge between the standard log package and the glog package.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a change we want to keep or just a proof?

@thockin
Copy link
Member

thockin commented Oct 25, 2018

So, IIUC, the discussion comes down to which path to choose:

  1. Do nothing, leave it all alone.

  2. Replace glog's output with $other_output (stderr, logrus, zap, etc); use glog as the API for now; possibly convert to a better API $later
    pro: works now
    pro: delivers better perf/behavior ASAP with minimal churn
    con: does not address log API / structured vs printf / spam
    con: does not at address proliferation of deps on glog
    meh: ugly deps/build hacks, but not unprecedented
    meh: need to fork glog, but it's frozen anyway so low risk

  3. Replace glog with a better API; convert to a better implementation $later
    pro: forces us to re-evaluate logging output
    pro: can set a trend for other go projects
    con: longer timeline
    con: high churn, thousands of call-sites to touch
    meh: better logging API is (maybe) less likely to be abused, but probably not

@DirectXMan12 @tallclair what else am I missing?

@dims
Copy link
Member Author

dims commented Oct 25, 2018

@thockin i'd like to open the door for #3 for long term and #2 for the short-term. We cannot do #3 without doing #2. We can talk more on the call

@thockin
Copy link
Member

thockin commented Oct 25, 2018 via email

Change-Id: I80e40b20298c73f3eefe8b743ad46871af24a73b
@dims dims force-pushed the try-forking-glog-into-third-party-directory branch from 98b08a7 to a03f52f Compare October 25, 2018 17:51
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dims
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: thockin

If they are not already assigned, you can assign the PR to them by writing /assign @thockin 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
Copy link
Contributor

k8s-ci-robot commented Oct 25, 2018

@dims: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-verify a03f52f link /test pull-kubernetes-verify

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.

@dims dims changed the title Forking glog into third_party/ directory [WIP] Forking glog into third_party/ directory Oct 26, 2018
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2018
@k8s-ci-robot
Copy link
Contributor

@dims: 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 k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2018
@dims
Copy link
Member Author

dims commented Nov 2, 2018

we now have k8s.io/klog so let's use that and drop the stuff here

@dims dims closed this Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apiserver area/code-generation area/kubeadm area/kubectl area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants