Make logstream watch the pods as they change. #1496
Conversation
- watch the pods and track what we already watch - start watchers for the new pods
knative-prow-robot
added
size/L
test/logstream/kubelogs.go
Outdated
| } | ||
| k.kc = kc | ||
|
|
||
| // List the pods in the given namespace. | ||
| pl, err := kc.Kube.CoreV1().Pods(k.namespace).List(metav1.ListOptions{}) | ||
| if err != nil { | ||
| t.Error("Error listing pods", "error", err) |
There was a problem hiding this comment.
Did you want to return here, too?
In general, I wonder whether returning an error here would be better than calling t.Error, but I don't feel strongly.
test/logstream/kubelogs.go
Outdated
| pod.Name, container.Name, scanner.Err()) | ||
| }) | ||
| } | ||
| pod := pod |
There was a problem hiding this comment.
This line looks strange. Do we need to copy the pod here, vs making a copy in startForPod?
There was a problem hiding this comment.
more or less standard go practice for for loops that spawn goroutines (had to add it actually based on debugging, I thought the passing to function would work too).
test/logstream/kubelogs.go
Outdated
| // equivalent to --all-containers. | ||
| for _, container := range pod.Spec.Containers { | ||
| // Required for capture below. | ||
| pod, container := pod, container |
There was a problem hiding this comment.
Why do we need to capture pod though?
There was a problem hiding this comment.
Right, but it's not a loop-variable 🤔
There was a problem hiding this comment.
escapes in go goroutine?
test/logstream/kubelogs.go
Outdated
| func (k *kubelogs) watchPods(t test.TLegacy, eg *errgroup.Group) { | ||
| wi, err := k.kc.Kube.CoreV1().Pods(k.namespace).Watch(metav1.ListOptions{}) | ||
| if err != nil { | ||
| t.Logf("Logstream knative pod watch failed, logs might be missing: %v", err) |
There was a problem hiding this comment.
Should we surface this error similar to the List error in init?
There was a problem hiding this comment.
I debated that... I am not sure failing a test, because of logging failure is a valid concern?
There was a problem hiding this comment.
But I'll go along with what we have now.
|
/retest I sent #1497 to fix the workqueue_test flakiness. |
test/logstream/kubelogs.go
Outdated
| case watch.Added, watch.Modified: | ||
| if k.watchedPods.Has(p.Name) { | ||
| t.Log("Already watching pod", p.Name) | ||
| continue | ||
| } | ||
| // If pod is ready then start the logs. | ||
| if p.Status.Phase == corev1.PodRunning && p.DeletionTimestamp == nil { | ||
| for _, cond := range p.Status.Conditions { | ||
| if cond.Type == corev1.PodReady && cond.Status == corev1.ConditionTrue { | ||
| t.Log("The new knative pod is ready", p.Name) |
There was a problem hiding this comment.
I think this would flow/read better with a bit more encapsulation and redrawing the line between these functions. Something like:
if watched.Has(p.Name) {
continue
}
if !isReady(pod) {
continue
}
watched.Insert(pod.Name)
k.startForPod(pod)
This also lets you make watchedPods a local variable instead of a field.
There was a problem hiding this comment.
No this does not work because we also start pods in init. But I think along the lines you left below — that I can remove the loop in init.
test/logstream/kubelogs.go
Outdated
| }) | ||
| } | ||
| pod := pod | ||
| k.startForPod(&eg, &pod) |
There was a problem hiding this comment.
Do we still need this loop, or can it be subsumed by watchPods?
There was a problem hiding this comment.
yeah, I think so.
|
Fixed the build and updated according to reviews. |
|
/close |
|
/retest |
|
Why my new commits don't show? |
|
@vagababov: Closed this PR. In response to this:
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. |
|
/reopen |
|
@vagababov: Reopened this PR. In response to this:
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. |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, vagababov 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 |
knative-prow-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
/close |
|
@vagababov: Closed this PR. In response to this:
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. |
|
/reopen |
|
@vagababov: Reopened this PR. In response to this:
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. |
For #1488
/assign @markusthoemmes mattmoor