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

PodLogsFollower #104

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

otaviof
Copy link
Member

@otaviof otaviof commented Mar 25, 2022

Changes

  • Refactoring the PodFollower into PodLogsFollower, a component focused on the pod lifecycle, capable of both streaming live logs and read logs from an existing pod
  • shp buildrun logs waits a few seconds for the pod, instead of error out immediately
  • Refactoring BuildRun inspections into a single component
  • Introducing FakeParams to simplify unit-testing
  • Refactoring Params to avoid cyclic-dependencies
  • Minor documentation updates

Fixes #103

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

Release Notes

Able to both stream and read all logs from a existing BuildRun, therefore the --follow/-F flag has been removed from "shp buildrun logs"

@otaviof otaviof added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Mar 25, 2022
@otaviof otaviof force-pushed the pod-logs-follower branch 7 times, most recently from 15a0c76 to 30ab1c9 Compare March 27, 2022 16:54
@otaviof
Copy link
Member Author

otaviof commented Mar 28, 2022

/assign @gabemontero @HeavyWombat

Copy link
Member

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

OK the changes are substantial enough @otaviof that I may just have to check out the branch and trace some end to end flows to get my bearings. The git file diffs, even with the different commits, are a bit hard for me to follow. That said, if my hunches are right, I like what you have done.

Before I go down that path, a couple of high level questions that might avoid more churn on my part:

  1. am I correct in thinking that -F was removed because you simply "follow by default" and the logic figures out if the pod is finished before the watch apparatus is fully set up. If so, highlighting that in the release note and PR description would be very helpful.
  2. per Can't tail buildruns. (buildrun create, buildrun logs) #80 (comment) can you construct the command sequence now used to follow logs after running shp buildrun create -n build --buildref-name foo .... (the answer here might confirm what I just said in (1)).

thanks

if len(pods.Items) == 0 {
fmt.Fprintf(ioStreams.ErrOut, "no builder pod found for BuildRun %q\n", c.name)
return false, nil
c.podLogsFollower, err = follower.NewPodLogsFollowerFromParams(ctx, p, pw, ioStreams)
Copy link
Member

Choose a reason for hiding this comment

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

ok so I see some form of log following still occurring

Now from what I recall Params are set from the cli flags. so I'm still not sure how this reconciles with removing -F

Copy link
Member Author

Choose a reason for hiding this comment

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

Please consider my latest comment. Thanks for pointing that out!

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 31, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from gabemontero after the PR has been reviewed.

The full list of commands accepted by this bot can be found 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

- Refactoring the former `PodFollower` to become `PodLogsFollower`,
  focused on the pod lifecycle, also able to both stream live logs or
  read all logs from a existing pod
- Refactoring `BuildRun` inspections into a single component
- Introducing `FakeParams` to simplify unit-testing and refactoring
  `Params` to avoid cycle dependencies
@otaviof otaviof changed the title PodLogsFollower and Params Refactoring PodLogsFollower Mar 31, 2022
@otaviof
Copy link
Member Author

otaviof commented Apr 1, 2022

Before I go down that path, a couple of high level questions that might avoid more churn on my part:

1. am I correct in thinking that `-F` was removed because you simply "follow by default" and the logic figures out if the pod is finished before the watch apparatus is fully set up.  If so, highlighting that in the release note and PR description would be very helpful.

I need to go back on this @gabemontero. Testing the behavior on main branch, it shows a different outcome during on using the --follow flag.

I traced back the GetPodLogs() usage, and consolidated it the new PodLogsFollower, with the same behavior we see today on main branch.

Please consider the latest changes.

Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Nice, this cleans up some things like the global follow instance.

Comment on lines +46 to +47
case br != nil && !br.HasStarted():
fmt.Fprintf(ioStreams.ErrOut, "BuildRun %q has been marked as failed.\n", name)
Copy link
Member

Choose a reason for hiding this comment

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

Pseude code:

Suggested change
case br != nil && !br.HasStarted():
fmt.Fprintf(ioStreams.ErrOut, "BuildRun %q has been marked as failed.\n", name)
case br != nil && !br.HasStarted():
fmt.Fprintf(ioStreams.ErrOut, "BuildRun %q has failed to start because of %q.\n", name, br.Status.Conditions[type=Succeeded].Message)


p.log("Pod %q in %q state, starting up log tail\n", pod.GetName(), corev1.PodRunning)

time.Sleep(3 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

I assume we wait here so that the user can read the message that was just printed? Is worth a comment, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, I would like to remove the sleep here. One can always scroll up to see the beginning. An artificial wait feels not right as it may slow down a response/result I would like to see.

logTail *tail.Tail // helper to tail container logs
logTailStarted map[string]bool // containers with logTail started

onlyOnce bool // retrieve the pod logs only once
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 Apr 2, 2022

Choose a reason for hiding this comment

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

This irritated me a little bit. If I understand it correctly, then this PodLogsFollower is capable to retrieve logs with and without following which is controlled by this flag. If that's the case, what about

  • renaming PodLogsFollower to PodLogsRetriever
  • renaming onlyOnce to follow

Alternatively, we could also just change onlyOnce to noFollow as a simple solution (but then the struct name would be confusing).


```sh
make build test-e2e
make test-e2e
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: trailing space

Suggested change
make test-e2e
make test-e2e

func (c *DeleteCommand) Complete(params *params.Params, io *genericclioptions.IOStreams, args []string) error {
func (c *DeleteCommand) Complete(p params.Interface, io *genericclioptions.IOStreams, args []string) error {
if len(args) != 1 {
return errors.New("build name is not informed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure here, could be that I am missing something. My thought was just if it is != to one, it could be that the user provided multiple build names thinking this could be supported. Therefore, the error message would be or could be misleading. We could consider using the Cobra setting to define the expected number of arguments.

br.GetName(),
)}
_, err = r.follower.Start(listOpts)
buildNameLabel := fmt.Sprintf("%s=%s", buildv1alpha1.LabelBuild, r.buildName)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could do it now, or keep it for later, but with embedded BuildSpecs (aka one-off builds) we will have TaskRuns with only the buildrun label, not the build label since there is no build.

// buildNameAnnotation label to identify the Build name.
buildNameAnnotation = "build.shipwright.io/name"
// buildRunNameAnnotation label to identify the BuildRun name.
buildRunNameAnnotation = "buildrun.shipwright.io/name"
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch


p.log("Pod %q in %q state, starting up log tail\n", pod.GetName(), corev1.PodRunning)

time.Sleep(3 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, I would like to remove the sleep here. One can always scroll up to see the beginning. An artificial wait feels not right as it may slow down a response/result I would like to see.

})

pw.Start(metav1.ListOptions{})
g.Expect(called).To(BeTrue())
g.Expect(called > 0).To(BeTrue())
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Gomega

Suggested change
g.Expect(called > 0).To(BeTrue())
g.Expect(called).To(BeNumerically(">", 0))

@SaschaSchwarze0 SaschaSchwarze0 added this to the release-v0.10.0 milestone Apr 4, 2022
@SaschaSchwarze0
Copy link
Member

/hold

Making sure this does not go into v0.10 release.

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 4, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 9, 2022

@otaviof: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky Unit Test Results
4 participants