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

Use a forked version of glog in kubernetes #70264

Closed
7 of 8 tasks
dims opened this issue Oct 26, 2018 · 26 comments · Fixed by #70889
Closed
7 of 8 tasks

Use a forked version of glog in kubernetes #70264

dims opened this issue Oct 26, 2018 · 26 comments · Fixed by #70889
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation.

Comments

@dims
Copy link
Member

dims commented Oct 26, 2018

Based on the discussion in SIG-Arch on Oct 25 and previous experiment in #69333, we should start by forking glog, cleanup the code to suit our use cases and switch over to it. Also see issue #61006

Tasks:

  • Request a new repository for the forked code in https://github.com/dims/klog in the kubernetes org(?)
  • Update k8s.io/klog README.md to explain it is a permanent fork.
  • update github.com/kubernetes/repo-infra/kazel/kazel.go to use klog
  • update k8s.io/gengo/ to use klog
  • update k8s.io/kube-openapi/ to use klog
  • update github.com/google/cadvisor to use klog
  • vendor all the repos above into k/k and switch k/k from glog to klog
  • add verify scripts to prevent adding any new vendor dependencies that use glog

cc @thockin @tallclair @erictune @justinsb @lavalamp

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 26, 2018
@dims
Copy link
Member Author

dims commented Oct 26, 2018

/sig architecture

@k8s-ci-robot k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 26, 2018
@dims
Copy link
Member Author

dims commented Oct 26, 2018

/assign @thockin
/assign @tallclair

@erictune
Copy link
Member

Thank you for doing this, @dims!

Also:

  • Update k8s.io/klog README.md to explain it is a permanent fork.

@liggitt
Copy link
Member

liggitt commented Oct 26, 2018

I think we would also need to add verify scripts to prevent adding any new vendor dependencies that use glog. I'm actually very surprised we don't have more already:

$ grep -R 'glog"' vendor | egrep -v 'k8s.io|cadvisor'
vendor/github.com/golang/glog/BUILD:    importpath = "github.com/golang/glog",
vendor/github.com/kubernetes/repo-infra/kazel/kazel.go:	"github.com/golang/glog"

@dims
Copy link
Member Author

dims commented Oct 26, 2018

+1 @erictune @liggitt updating task list above

@deads2k
Copy link
Contributor

deads2k commented Oct 26, 2018

I think we would also need to add verify scripts to prevent adding any new vendor dependencies that use glog. I'm actually very surprised we don't have more already:

I think that's because this project uses godep, not something like glide which pulls in entire trees. github.com/coreos/etcd vendors github.com/grpc-ecosystem/grpc-gateway which vendors glog.

@liggitt
Copy link
Member

liggitt commented Oct 26, 2018

we would also need to add verify scripts to prevent adding any new vendor dependencies that use glog

given the prevalence of glog usage in the go ecosystem, we also have to consider the impact of k8s.io/* components using a logging library that cannot be built into a binary that also includes glog. If a downstream consumer imports k8s.io/apimachinery or k8s.io/client-go (and therefore "klog"), and also has another dependency that uses glog, this change will break them without much recourse.

@deads2k
Copy link
Contributor

deads2k commented Oct 26, 2018

Based on the discussion in SIG-Arch on Oct 25 and previous experiment in #69333, we should start by forking glog, cleanup the code to suit our use cases and switch over to it. Also see issue #61006

@dims I've had a look at your fork. Is there a list of specific objectives beyond flags and output? I think I'd be looking for something more pluggable overall if we commit to this pain.

@dims
Copy link
Member Author

dims commented Oct 26, 2018

@liggitt to allow klog and glog to co-exist

  • We have to drop the flags registration from "init" and require an explicit Init for klog
  • end users can use klog's setOutput and redirect the logs from klog to glog

@dims
Copy link
Member Author

dims commented Oct 26, 2018

@deads2k, there are a lot of discussions in many of the previous issues about things that can be done. I am deliberately trying to start simple here and unblock folks who want to do more things.

For example, i would like to:

-- Dims

@liggitt
Copy link
Member

liggitt commented Oct 26, 2018

@liggitt to allow klog and glog to co-exist

  • We have to drop the flags registration from "init" and require an explicit Init for klog
  • end users can use klog's setOutput and redirect the logs from klog to glog

I think allowing co-existence is a requirement if klog is referenced by our published repos.

@dims
Copy link
Member Author

dims commented Oct 26, 2018

@liggitt agree. we should document what someone has to do if they are using glog today and want to import our stuff.

@deads2k
Copy link
Contributor

deads2k commented Oct 26, 2018

@liggitt agree. we should document what someone has to do if they are using glog today and want to import our stuff.

It's not just about code directly owned by the vendorer. I vendor kube libraries and I vendor libraries from other authors that I do not own. Anything that requires me to change code to import and run with kube libraries can make that impossible.

@deads2k, there are a lot of discussions in many of the previous issues about things that can be done. I am deliberately trying to start simple here and unblock folks who want to do more things.

@dims do you have a specific roadmap in mind? This feels like a switch from A to B, where B is more or less equivalent and any future direction change will face the same roadblocks as before.

@liggitt
Copy link
Member

liggitt commented Oct 26, 2018

Anything that requires me to change code to import and run with kube libraries can make that impossible.

Right. klog should allow co-existence by default with no action taken by importers. The types of things required to do this are good practices anyway (don't register global flags in init blocks, etc).

@dims
Copy link
Member Author

dims commented Oct 26, 2018

@deads2k regarding same roadblocks this time, we have a place where we can make changes :) klog repo! we did not have the luxury of modifying glog before :)

@tallclair
Copy link
Member

tallclair commented Oct 26, 2018

copying here for visibility (from kubernetes/org#195 (comment)):

Are we still intending for [klog] to be a transitional step towards our logging end goal? If so, can we call that out in the description & readme, and discourage others from using this? My concern is that as soon as we put out a "kubernetes-log" repo, other projects will start cargo-culting it...
Along similar lines, I wonder if we should stick with "glog" for the name? It further emphasizes that this is a fork of glog, and not a new kubernetes-endorced log library. It also means fewer lines need to be touched in the migration.

@BenTheElder
Copy link
Member

I think we would also need to add verify scripts to prevent adding any new vendor dependencies that use glog. I'm actually very surprised we don't have more already:

Noting that one of the examples:

vendor/github.com/kubernetes/repo-infra/kazel/kazel.go: "github.com/golang/glog"

Is actually a dev-time dependency rather than being built into any k8s components. I don't think we need to worry too much about that (though we might as well migrate it too) cc @ixdy

/cc

@dims
Copy link
Member Author

dims commented Oct 26, 2018

@tallclair ack.

One more data point for folks here. thanks @aledbf for showing me how Istio does this. based on his pointers here's a repo - https://github.com/dims/glog-minimal that redirects glog calls to klog

This can work with some usecases like https://github.com/istio/istio/blob/master/Gopkg.toml#L61 (override the source using dep)

@ixdy
Copy link
Member

ixdy commented Oct 26, 2018

yes, migrating kazel is fine with me.

@dims
Copy link
Member Author

dims commented Oct 26, 2018

We have 2 ways of co-existing with glog. the glog-minimal repo above or see the example in klog. So i believe we can move on to the steps listed above.

@thockin @tallclair i am looking to you both to approve the org request as well :)

kubernetes/org#195

Yes, please decide on the name as well. (klog vs glog)

@tallclair
Copy link
Member

One more data point for folks here. thanks @aledbf for showing me how Istio does this. based on his pointers here's a repo - https://github.com/dims/glog-minimal that redirects glog calls to klog

That's awesome that istio has already done this - that's exactly the route I was picturing us going.

@thockin
Copy link
Member

thockin commented Oct 29, 2018

I'm not sure I see how this fixes conflicts with imports on "real" glog?

If I have an app that uses github.com/golang/glog pervasively, and I decide to use client-go, I will have glog and klog loaded, and the logs will not be merged.

What am I missing?

@dims
Copy link
Member Author

dims commented Oct 29, 2018

@thockin if you are using github.com/golang/glog pervasively and want to use client-go, then you have 2 choices.

Choice #1 - Keep github.com/golang/glog in charge of actual file operations using example/pattern in:

Choice #2 - I don't care much about github.com/golang/glog` in charge of writing to disk, but i don't want to change all my existing code, what is my option?

@thockin
Copy link
Member

thockin commented Oct 29, 2018 via email

@dims
Copy link
Member Author

dims commented Oct 31, 2018

/sig instrumentation

@k8s-ci-robot k8s-ci-robot added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Oct 31, 2018
@dims
Copy link
Member Author

dims commented Oct 31, 2018

@kubernetes/sig-instrumentation-feature-requests @kubernetes/sig-architecture-feature-requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants