-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Kubedns logging #36013
Kubedns logging #36013
Conversation
Jenkins verification failed for commit 39e5d426c3d01344c221fa97e9eb409cb92a52f0. Full PR test history. The magic incantation to run this job again is |
39e5d42
to
b15344c
Compare
I took a quick look. Assigned to @MrHohn. Can you post a snippet of logs here? |
v(2)
v(3)
v(4)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Add comments basically for nits.
server.SetDefaults(skydnsConfig) | ||
s := server.New(d.kd, skydnsConfig) | ||
if err := metrics.Metrics(); err != nil { | ||
glog.Fatalf("skydns: %s", err) | ||
glog.Fatalf("Skydns metrics error: %s", err) | ||
} else if metrics.Port != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to give a warning if metrics.Port is not set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok -- added info log
@@ -230,7 +243,9 @@ func assertIsService(obj interface{}) (*kapi.Service, bool) { | |||
|
|||
func (kd *KubeDNS) newService(obj interface{}) { | |||
if service, ok := assertIsService(obj); ok { | |||
glog.V(4).Infof("Add/Updated for service %v", service.Name) | |||
glog.V(2).Infof("newService %v", service.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would New service: %v
better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -63,7 +63,8 @@ func ReverseArray(arr []string) []string { | |||
func GetSkyMsg(ip string, port int) (*msg.Service, string) { | |||
msg := NewServiceRecord(ip, port) | |||
hash := HashServiceRecord(msg) | |||
glog.V(2).Infof("DNS Record:%s, hash:%s", fmt.Sprintf("%v", msg), hash) | |||
glog.V(5).Infof("Created new DNS record: %s, hash:%s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function construct a new record but not actually create one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to "constructed"
"github.com/skynetservices/skydns/msg" | ||
) | ||
|
||
func TestTreeCache(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also test SetSubCache() here?
Saw the TODO says we should combine it with SetEntry() in future. May be wait til then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a test
|
||
retval := []skymsg.Service{} | ||
for _, val := range records { | ||
retval = append(retval, *val) | ||
} | ||
|
||
glog.V(2).Infof("records:%v, retval:%v, path:%v", records, retval, path) | ||
glog.V(4).Infof("getRecordsForPath retval=%+v, path=%v", retval, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it also make sense to make it identical to the function name as it is V4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is the same?
@@ -676,34 +709,39 @@ func (kd *KubeDNS) getPodIP(path []string) (string, error) { | |||
// We can add support for wildcard queries later, if needed. | |||
func (kd *KubeDNS) isFederationQuery(path []string) bool { | |||
if len(path) != 4+len(kd.domainPath) { | |||
glog.V(2).Infof("not a federation query: len(%q) != 4+len(%q)", path, kd.domainPath) | |||
glog.V(4).Infof("not a federation query: len(%q) != 4+len(%q)", path, kd.domainPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: first letter capital
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
func TestTreeCache(t *testing.T) { | ||
tc := NewTreeCache() | ||
|
||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why put this in a block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limit variable scoping
t.Error("should not affect p3.p1.") | ||
} | ||
if tc.DeletePath("p1", "p2") { | ||
t.Fatal() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be add a bit error message when fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
{"key3", []string{"p1", "p3"}}, | ||
} { | ||
if _, ok := tc.GetEntry(testCase.k, testCase.p...); ok { | ||
t.Error() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
--v=2 is low noise (record changes), can be default --v=3 will shows per request logging Note: due to the code path with which we integrate with skydns, we don't see non-PILLAR_DOMAIN requests, so these will never be logged.
b15344c
to
d9557d4
Compare
/lgtm |
Jenkins GCI GCE e2e failed for commit d9557d4. Full PR test history. The magic incantation to run this job again is |
gci gce e2e test this |
@k8s-bot gci gce e2e test this |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
fixes
#29053
may resolve #29054, but depends on what the specific ask is
This change is