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

implement "--log_file" and "--log_dir" for klog #9521

Closed
medyagh opened this issue Oct 21, 2020 · 8 comments · Fixed by #9592
Closed

implement "--log_file" and "--log_dir" for klog #9521

medyagh opened this issue Oct 21, 2020 · 8 comments · Fixed by #9592
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@medyagh
Copy link
Member

medyagh commented Oct 21, 2020

I hardcoded this in our code to force log file to be to a specific file,

	if err := goflag.Set("log_file", "medya_1.txt"); err != nil {

and makes the log file to append to medya_1.txt we just need to parse this from the flags

however if I pass the same flag to the minikube binary it will not do anything (without that line above)

minikube status --log_file=medya2.txt

same with one dash

minikube status -log_file=medya2.txt
@medyagh medyagh added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 21, 2020
@medyagh medyagh changed the title fix klog not respecting log_file flag implement log_file Oct 21, 2020
@medyagh medyagh changed the title implement log_file implement "log_file" for klog Oct 21, 2020
@medyagh medyagh added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Oct 21, 2020
@medyagh
Copy link
Member Author

medyagh commented Oct 22, 2020

I see we had a workaround package that we created to fix Glog erroring

	// initflag must be imported before any other minikube pkg.
	// Fix for https://github.com/kubernetes/minikube/issues/4866

	"k8s.io/klog/v2"
	_ "k8s.io/minikube/pkg/initflag"
https://github.com/medyagh/minikube/blob/717b738d6d785b21604f9145dc3432680034900a/pkg/minikube/extract/extract.go#L34

but I see after the klog refactor we are importing klog before initflag package (that could be an issue maybe?)

I tried deleting the initflag package workarround and that didnt have any effect.

@medyagh medyagh added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Oct 22, 2020
@medyagh medyagh changed the title implement "log_file" for klog implement "--log_file" and "--log_dir" for klog Oct 22, 2020
@medyagh
Copy link
Member Author

medyagh commented Oct 22, 2020

I just did a standalone example Cobra+Klog and I noticed running this code does NOT produce any temp file logs AT ALL !!!

find $TMPDIR -mtime -1 -type f -name "*INFO*" -ls 2>/dev/null

that makes me think, is the temp file really created by klog ???

here is the code for the standalone example , same as (this llink)[https://flowerinthenight.com/blog/2019/02/05/golang-cobra-klog] except I made it v2 to match minikube

package main

import (
	goflag "flag"

	"github.com/spf13/cobra"
	flag "github.com/spf13/pflag"
	"k8s.io/klog/v2"
)

var (
	str = "hello world"

	rootCmd = &cobra.Command{
		Use:   "echo",
		Short: "use klog with cobra",
		Long:  "Use klog together with cobra.",
	}
)

func init() {
	rootCmd.Flags().SortFlags = false
	rootCmd.AddCommand(
		RunCmd(),
	)

	klog.InitFlags(nil)
	goflag.Parse()
	flag.CommandLine.AddGoFlagSet(goflag.CommandLine)
}

func RunCmd() *cobra.Command {
	runcmd := &cobra.Command{
		Use:   "run",
		Short: "run command",
		Long:  "Run command.",
		Run: func(cmd *cobra.Command, args []string) {
			klog.Infof("echo=%v", str)
		},
	}

	runcmd.Flags().SortFlags = false
	runcmd.Flags().StringVar(&str, "str", str, "string to print")
	return runcmd
}

func main() {
	if err := rootCmd.Execute(); err != nil {
		klog.Fatalf("root cmd execute failed, err=%v", err)
	}
}


@prezha
Copy link
Contributor

prezha commented Oct 23, 2020

/assign

@medyagh
Copy link
Member Author

medyagh commented Oct 23, 2020

@prezha thanks for taking this, please let me know if you go stuck. and please hit me up on slack !

btw I woud like to invite you to minikube office hours this monday ! we have Minikube Crazy Idea event. (to come up with crazy ideas for the next 2 years)

@prezha
Copy link
Contributor

prezha commented Oct 23, 2020

@medyagh no probs, i will

also, thank you for inviting me: i'd love to join minikube office hours on monday and the minikube crazy idea event - sounds like fun ;)

@medyagh
Copy link
Member Author

medyagh commented Oct 27, 2020

@prezha
I belive this must be because we use kubectls' templates to group the cobra command as seen here

we might need to do what openshift is donig here

@prezha
Copy link
Contributor

prezha commented Oct 27, 2020

thank you @medyagh, i'll have a look at that

@prezha
Copy link
Contributor

prezha commented Oct 31, 2020

@medyagh i think that the issue is with go flag and the fact that we are using subcommands (that prevent go flag from parsing args); that can be solved with pflag - details are in the pr #9592
please have a look and share your thoughts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants