Add version flag to LH binaries #1332

vthapar · 2023-08-01T07:25:58Z

Adds --version flag to Lighthouse Agent and --subm-version flag to Lighthouse Coredns. CoreDNS already has --version which prints CoreDNS version, so we can't use the same flag for Lighthouse version.

Refer submariner-io/enhancements#204

Fixes #1333

submariner-bot · 2023-08-01T07:26:01Z

🤖 Created branch: z_pr1332/vthapar/version-flag
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

submariner-bot · 2023-08-01T14:44:06Z

🤖 Closed branches: [z_pr1332/vthapar/version-flag]

submariner-bot · 2023-08-01T14:44:08Z

🤖 Created branch: z_pr1332/vthapar/version-flag
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

tpantelis · 2023-08-01T14:59:37Z

coredns/main.go

-var version = "not-compiled-properly"
+var (
+	version     = "not-compiled-properly"
+	submVersion = false


Suggested change

submVersion = false

showVersion = false

tpantelis · 2023-08-01T15:01:08Z

pkg/agent/main.go

+	masterURL   string
+	kubeConfig  string
+	help        = false
+	submVersion = false


Suggested change

submVersion = false

showVersion = false

tpantelis · 2023-08-01T15:01:55Z

coredns/main.go

+	if submVersion {
+		fmt.Printf("submariner-lighthouse-coredns version: %s", version)
+		return
+	}


We also log the version in setupLighthouse. How about we just print the version here (and remove the log in setupLighthouse) and exit if the version was requested? Same with the agent.

Suggested change

if submVersion {

fmt.Printf("submariner-lighthouse-coredns version: %s", version)

return

}

fmt.Printf("submariner-lighthouse-coredns version: %s", version)

if showVersion {

return

}

Yeah, I thought about printing and not using logger, but what if someone is using an external logger? printf will not show up in that, right? I like idea of using common print for both coz it ensures version message will be same.

How would someone use/setup an external logger? But regardless, wouldn't fmt.Printf go to the pod log no matter what? We'll scrape from that.

Tried it out, fmt.Printf didn't show up in logs, only if we use logger. So will continue with current approach. Changed submVersion to showVersion.

That’s odd. So fmt.Printf shows up when you run it with --version but not when run normally? What’s the diff? Aren’t you scraping the logs in the same manner?

I looked at klog code and it writes to stderr by default, ie the logtostderr flag defaults to true. In fact there’s no option to write to stdout. admiral's zerolog also writes to stderr. I think that's b/c kubectl scrapes the log from stderr. So if we do fmt.Fprintf(os.Stderr, …), it should show up in the pod logs.

@tpantelis Okay, print to stderr worked. But two issues with it:

Doesn't work for coredns For some reason any output to stdout or stderr before plugin directive is loaded results in CrashLoopBackoff on coredns. So we can't print before plugin is loaded and if we do that --version will not work coz coredns is already running so it will fail to bring it up again.

Using print to stderr shows up in logs but without timestamp or line of code information:

submariner-lighthouse-agent version: devel-e7cead11fe84 2023-08-03T05:42:07.414Z INF ..e/pkg/agent/main.go:105 main Arguments: [/usr/local/bin/lighthouse-a gent -alsologtostderr -v=2]

Am okay either ways but do prefer to use logger for log file so that version information shows up with timestamp et al in the logs. Thoughts?

Doesn't work for coredns For some reason any output to stdout or stderr before plugin directive is loaded results in CrashLoopBackoff on coredns.

That's weird. Do you get a stack trace? We can log it where we do now.

Am okay either ways but do prefer to use logger for log file so that version information shows up with timestamp et al in the logs. Thoughts?

I think it's fine w/o timestamp. It's not really needed since it's the first log entry and the pertinent info is the version. But if you want it then set up the logger beforehand.

That's weird. Do you get a stack trace? We can log it where we do now.

No stack trace. Only error about missing dns directive.

tpantelis

I think to be consistent we should use fmt.Fprintf in setupLighthouse as well.

vthapar · 2023-08-03T16:07:36Z

I think to be consistent we should use fmt.Fprintf in setupLighthouse as well.

There are quite a few log statements printed from CoreDNS before setupLighthouse. It works for agent coz it is the first statement so no timestamp is fine. For setupLighthouse I prefer having the timestamp as it gives indication of when lighthouse was loaded.

Also, I think I tried it that way and got some issues, but it was probably coz I was doing printf in main.go

submariner-bot · 2023-08-03T16:59:50Z

🤖 Closed branches: [z_pr1332/vthapar/version-flag]

submariner-bot · 2023-08-03T16:59:53Z

🤖 Created branch: z_pr1332/vthapar/version-flag
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

vthapar · 2023-08-04T04:58:22Z

Managed to make it work without needing a logger in setupLighthouse.

As discussed on slack, will use Fprintf(os.Stderr...) in main.go, remove log entry from setupLighthouse and metrics.go:init() so that version is first line in logs.

submariner-bot · 2023-08-04T05:02:36Z

🤖 Closed branches: [z_pr1332/vthapar/version-flag]

submariner-bot · 2023-08-04T05:02:39Z

🤖 Created branch: z_pr1332/vthapar/version-flag
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

tpantelis · 2023-08-07T14:12:38Z

coredns/main.go

 	dnsserver.Directives = directives
 }

 func main() {
+	flag.Parse()
+	admversion.Print(names.LighthouseCoreDNSComponent, version)
+	fmt.Fprintf(os.Stderr, "submariner-lighthouse-coredns version: %s\n", version)


Suggested change

fmt.Fprintf(os.Stderr, "submariner-lighthouse-coredns version: %s\n", version)

Adds `--version` flag to Lighthouse Agent and `--subm-version` flag to Lighthouse Coredns. CoreDNS already has `--version` which prints CoreDNS version, so we can't use the same flag for Lighthouse version. Removed a log entry from metrics.go which didn't add much value so we can have version as first entry in log file. Refer submariner-io/enhancements#204 Fixes submariner-io#1333 Signed-off-by: Vishal Thapar <[email protected]>

submariner-bot · 2023-08-07T14:55:17Z

🤖 Closed branches: [z_pr1332/vthapar/version-flag]

submariner-bot · 2023-08-07T14:55:20Z

🤖 Created branch: z_pr1332/vthapar/version-flag
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

submariner-bot · 2023-08-08T04:39:51Z

🤖 Closed branches: [z_pr1332/vthapar/version-flag]

vthapar requested review from aswinsuryan, Oats87, skitt, sridhargaddam and tpantelis as code owners August 1, 2023 07:25

vthapar force-pushed the version-flag branch from 1dcbf1a to a911697 Compare August 1, 2023 08:01

tpantelis closed this Aug 1, 2023

tpantelis reopened this Aug 1, 2023

tpantelis requested changes Aug 1, 2023

View reviewed changes

vthapar force-pushed the version-flag branch from 574f721 to d75ec96 Compare August 2, 2023 05:11

vthapar mentioned this pull request Aug 2, 2023

show versions should extract version information from pod logs submariner-io/subctl#798

Closed

vthapar requested a review from tpantelis August 2, 2023 05:50

vthapar force-pushed the version-flag branch 4 times, most recently from 12355d7 to d262725 Compare August 3, 2023 14:46

tpantelis reviewed Aug 3, 2023

View reviewed changes

tpantelis approved these changes Aug 3, 2023

View reviewed changes

tpantelis closed this Aug 3, 2023

tpantelis reopened this Aug 3, 2023

vthapar force-pushed the version-flag branch from d262725 to d3c4f85 Compare August 4, 2023 05:01

tpantelis approved these changes Aug 4, 2023

View reviewed changes

submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Aug 4, 2023

tpantelis closed this Aug 4, 2023

tpantelis reopened this Aug 4, 2023

vthapar force-pushed the version-flag branch from d3c4f85 to 27afbe3 Compare August 7, 2023 14:09

vthapar requested a review from tpantelis August 7, 2023 14:10

tpantelis requested changes Aug 7, 2023

View reviewed changes

vthapar force-pushed the version-flag branch from 27afbe3 to 9d8b975 Compare August 7, 2023 14:20

vthapar force-pushed the version-flag branch from 9d8b975 to 1143973 Compare August 7, 2023 14:26

vthapar requested a review from tpantelis August 7, 2023 14:37

tpantelis approved these changes Aug 7, 2023

View reviewed changes

tpantelis closed this Aug 7, 2023

tpantelis reopened this Aug 7, 2023

tpantelis enabled auto-merge (rebase) August 7, 2023 14:55

sridhargaddam approved these changes Aug 8, 2023

View reviewed changes

tpantelis merged commit 7bbd6e4 into submariner-io:devel Aug 8, 2023
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add version flag to LH binaries #1332

Add version flag to LH binaries #1332

vthapar commented Aug 1, 2023 •

edited

Loading

submariner-bot commented Aug 1, 2023

submariner-bot commented Aug 1, 2023

submariner-bot commented Aug 1, 2023

tpantelis Aug 1, 2023

tpantelis Aug 1, 2023

tpantelis Aug 1, 2023 •

edited

Loading

vthapar Aug 1, 2023 •

edited

Loading

tpantelis Aug 1, 2023 •

edited

Loading

vthapar Aug 2, 2023

tpantelis Aug 2, 2023 •

edited

Loading

tpantelis Aug 2, 2023

vthapar Aug 3, 2023

tpantelis Aug 3, 2023

vthapar Aug 3, 2023

tpantelis left a comment

vthapar commented Aug 3, 2023

submariner-bot commented Aug 3, 2023

submariner-bot commented Aug 3, 2023

vthapar commented Aug 4, 2023

submariner-bot commented Aug 4, 2023

submariner-bot commented Aug 4, 2023

tpantelis Aug 7, 2023

submariner-bot commented Aug 7, 2023

submariner-bot commented Aug 7, 2023

submariner-bot commented Aug 8, 2023

Add version flag to LH binaries #1332

Add version flag to LH binaries #1332

Conversation

vthapar commented Aug 1, 2023 • edited Loading

submariner-bot commented Aug 1, 2023

submariner-bot commented Aug 1, 2023

submariner-bot commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpantelis Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

vthapar Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

tpantelis Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpantelis Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpantelis left a comment

Choose a reason for hiding this comment

vthapar commented Aug 3, 2023

submariner-bot commented Aug 3, 2023

submariner-bot commented Aug 3, 2023

vthapar commented Aug 4, 2023

submariner-bot commented Aug 4, 2023

submariner-bot commented Aug 4, 2023

Choose a reason for hiding this comment

submariner-bot commented Aug 7, 2023

submariner-bot commented Aug 7, 2023

submariner-bot commented Aug 8, 2023

vthapar commented Aug 1, 2023 •

edited

Loading

tpantelis Aug 1, 2023 •

edited

Loading

vthapar Aug 1, 2023 •

edited

Loading

tpantelis Aug 1, 2023 •

edited

Loading

tpantelis Aug 2, 2023 •

edited

Loading