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

added flag --since-time for Logs Collectors #287

Merged
merged 17 commits into from Oct 29, 2020

Conversation

manavellamnimble
Copy link
Contributor

@markpundsack @divolgin Hi, this is the first approach for a flag to determine a point of time, from which the logs collector should start collecting logs. It overrides all the logs collectors, and keeps the maxLine parameter from the original specs. Let me know what you think of it. Fix #277 An example:

    $ kubectl support-bundle --since-time="2020-10-19T12:36:23Z" file_or_url 

@manavellamnimble manavellamnimble changed the title added flag --since-time added flag --since-time for Logs Collectors Oct 19, 2020
@markpundsack
Copy link
Contributor

Great! I'd like to see it support since as well, since relative time is easier to manipulate than UTC time. Does since-time support local time?

@manavellamnimble
Copy link
Contributor Author

@markpundsack Would you like to add the --since flag or do you want the --since-time flag to accept hours, --since-time="720h" for example. I think if you use the 'Z' at the end of the date-time, like in "2020-10-19T12:36:23Z", the local time is used.

@markpundsack
Copy link
Contributor

@manavellamnimble Both have value.

I think if you use the 'Z' at the end of the date-time, like in "2020-10-19T12:36:23Z", the local time is used.

Hmm... I think 'Z' means "zero offset" which means UTC.

@markpundsack
Copy link
Contributor

Btw, we don’t need to block merging this; since handling can be a separate PR.

@markpundsack
Copy link
Contributor

@jgruica @GraysonNull We should consider adding this to the Kots web UI as well.

@manavellamnimble
Copy link
Contributor Author

@markpundsack Hi! I added the --since flag, so both can be shipped (and documented) together.

@markpundsack
Copy link
Contributor

Awesome, thank you!

@markpundsack
Copy link
Contributor

@salahalsaleh Can you review this?

cmd/preflight/cli/run.go Outdated Show resolved Hide resolved
cmd/preflight/cli/run.go Outdated Show resolved Hide resolved
pkg/collect/logs.go Outdated Show resolved Hide resolved
manavellamnimble and others added 4 commits October 21, 2020 13:59
Co-authored-by: Salah Aldeen Al Saleh <salahalsaleh1993@gmail.com>
Co-authored-by: Salah Aldeen Al Saleh <salahalsaleh1993@gmail.com>
cmd/preflight/cli/root.go Outdated Show resolved Hide resolved
cmd/preflight/cli/root.go Outdated Show resolved Hide resolved
@@ -129,6 +129,29 @@ func runPreflights(v *viper.Viper, arg string) error {
KubernetesRestConfig: restConfig,
}

if v.GetString("since-time") != "" {
if v.GetString("since") != "" {
progressChan <- errors.Errorf("Only one of since-time / since may be used. The flag since-time will be used.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that how Kubectl handles it? Or does it abort because of invalid args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I had my doubts about this too. K8s aborts with the message error: parsing time. I followed the error handling for the limits in the logs collector (communicating it through the progress channel), but it would make sense to abort because if someone used the flag and it can't be parsed maybe there would be no point in running through all the collectors and analyzers

cmd/troubleshoot/cli/root.go Outdated Show resolved Hide resolved
cmd/troubleshoot/cli/root.go Outdated Show resolved Hide resolved
manavellamnimble and others added 5 commits October 22, 2020 09:26
Co-authored-by: Mark Pundsack <markpundsack@users.noreply.github.com>
Co-authored-by: Mark Pundsack <markpundsack@users.noreply.github.com>
Co-authored-by: Mark Pundsack <markpundsack@users.noreply.github.com>
Co-authored-by: Mark Pundsack <markpundsack@users.noreply.github.com>
@manavellamnimble
Copy link
Contributor Author

@markpundsack based on what we discussed, I've redesign the solution. Before running the collectors, the flags are parsed to time variable to check if it is all right, and throws an error if there is something wrong. All of this happens in a separate function to make it more organized. This modification also requires just one extra field in log.limits (not two) and requires less work to implement in the log collector. @salahalsaleh Can you please check it?

@markpundsack
Copy link
Contributor

Thanks for the changes.

BTW, do we not have tests on this project?

@divolgin divolgin merged commit 014a716 into replicatedhq:master Oct 29, 2020
@manavellamnimble manavellamnimble deleted the addLogFlags branch November 4, 2020 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support command line (and web UI) way of adjusting time period for collecting logs
4 participants