Adapt logs command to Loki #213

Kristian-ZH · 2020-06-26T07:04:13Z

What this PR does / why we need it:
Adapt logs command to Loki

This PR depends on this one: gardener/gardener#2515 and should wait until it is merged

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
@DockToFuture

Release note:

`--loki` flag is added in the `gardenctl logs` command
`--elasticsearch` flag is removed from the `gardenctl logs` command.

vpnachev · 2020-07-31T14:32:53Z

/reviewed ok-to-test

vpnachev · 2020-07-31T14:36:26Z

gardener/gardener#2515 has been merged.
In the best case, I think that gardenctl should support both flags for some time and remove --elasticsearch later. This is needed to support landscape running on different versions of Gardener. However, I have no strong opinion about it.

neo-liang-sap · 2020-08-02T09:45:59Z

Hi @Kristian-ZH , i see this PR is failing at https://concourse.ci.gardener.cloud/builds/67311
in your local build env you can run gardenctl/.ci/check to reproduce this error.
Could you please fix this error in your PR?
Thanks
-Neo

Running golangci-lint...
pkg/cmd/logs.go:37:2: `emptyString` is unused (deadcode)
	emptyString           = ""
	^
pkg/cmd/logs.go:38:2: `unauthorized` is unused (deadcode)
	unauthorized          = "Unauthorized"
	^
pkg/cmd/logs.go:219:6: `getCredentials` is unused (deadcode)
func getCredentials(namespace string) (*v1.Secret, error) {
     ^
pkg/cmd/logs.go:261:3: ineffectual assignment to `lokiQuery` (ineffassign)
		lokiQuery += fmt.Sprintf("&&query={container_name=~\"%s.*\"", container)
		^

neo-liang-sap · 2020-08-10T02:12:48Z

Update: per Slack conversation with @Kristian-ZH , he will fix this validation error soon

neo-liang-sap · 2020-08-10T07:06:25Z

@dansible @DockToFuture do you have any comments on this PR? if not i will go ahead merge this PR, thanks!

Kristian-ZH · 2020-08-10T07:07:58Z

@dansible @DockToFuture do you have any comments on this PR? if not i will go ahead merge this PR, thanks!

I want also to discuss with @vpnachev if we need to add support for --elasticsearch also :)

neo-liang-sap · 2020-08-10T07:09:23Z

I want also to discuss with @vpnachev if we need to add support for --elasticsearch also :)

sure, take your time, thanks for the PR!

Kristian-ZH · 2020-08-10T08:52:23Z

I added support for --elasticsearch flag too

Kristian-ZH · 2020-08-10T08:53:25Z

@vlvasilev Can you please check the PR and make a review

vlvasilev · 2020-08-12T08:28:57Z

pkg/cmd/miscellaneous.go

+	}
+	pieces := strings.Split(apiAdress, ".")
+
+	return "garden-" + pieces[2]


why piece 2? Is this apiAddress always be in the 2 position?

I do not know other option how to take the project namespace from the garden cluster

If you know the project name, you can retrieve the namespace out of its spec, e.g. .spec.namespace.
Also, not all project namespaces are prefixed with garden-, and some have a trailing hash, so I advise you to get the namespace name from the project.

hmm, what about when the user is targeting shoot directly.
e.g. g target garden -> seed -> shoot. In this way I do not know the project name

The URL of the API is not a better solution, it will even not work when the DNS is disabled and the public address of the kube-apiserver service is used directly.

The kubeconfig file contains the technical name of the shoot as cluster and context name, e.g. shoot-[-]<project-name-[-]<shoot-name>. You have the shoot name in the current target, so you can easily find the project name from the technical name.

vlvasilev · 2020-08-12T16:24:31Z

/lgtm

vpnachev · 2020-08-13T05:58:56Z

pkg/cmd/miscellaneous.go

+	}
+	pieces := strings.Split(apiAdress, ".")
+
+	return "garden-" + pieces[2]


The kubeconfig file contains the technical name of the shoot as cluster and context name, e.g. shoot-[-]<project-name-[-]<shoot-name>. You have the shoot name in the current target, so you can easily find the project name from the technical name.

Kristian-ZH · 2020-08-14T14:35:57Z

Sorry for the delay but we are fixing issues with the logging stack.

Hope next week will finish this PR
(Will refactor the function GetProjectName, because does not work correctly)

neo-liang-sap · 2020-08-14T15:12:56Z

Sorry for the delay but we are fixing issues with the logging stack.
Hope next week will finish this PR

@Kristian-ZH , thanks! absolutely not urgent, take your time! i just regularly (as one of code owner) check the existing PR to make sure it's not blocked or forgotten ;)
thanks again for working on this PR!
-Neo

Kristian-ZH · 2020-08-17T09:12:16Z

@vpnachev @neo-liang-sap Please check the latest changes

neo-liang-sap · 2020-08-17T09:26:39Z

/lgtm

vpnachev

Some suggestions for improvements.

pkg/cmd/logs.go

pkg/cmd/miscellaneous.go

vpnachev

/lgtm

Kristian-ZH requested review from DockToFuture and ialidzhikov as code owners June 26, 2020 07:04

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 26, 2020

gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 26, 2020

gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 31, 2020

Kristian-ZH force-pushed the loki branch from 27c8664 to 2073a55 Compare August 10, 2020 06:32

Kristian-ZH requested a review from a team as a code owner August 10, 2020 06:32

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 10, 2020

neo-liang-sap approved these changes Aug 10, 2020

View reviewed changes

Kristian-ZH force-pushed the loki branch from 2073a55 to ae38e39 Compare August 10, 2020 08:51

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 10, 2020

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 10, 2020

Kristian-ZH force-pushed the loki branch from ae38e39 to 52020c8 Compare August 10, 2020 08:55

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 10, 2020

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 10, 2020

Kristian-ZH force-pushed the loki branch from 52020c8 to 4ff7541 Compare August 10, 2020 14:31

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 10, 2020

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 10, 2020

Kristian-ZH force-pushed the loki branch from 4ff7541 to 6d1ce46 Compare August 11, 2020 08:20

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 11, 2020

vlvasilev reviewed Aug 12, 2020

View reviewed changes

gardener-robot added the reviewed/lgtm Has approval for merging label Aug 12, 2020

vpnachev suggested changes Aug 13, 2020

View reviewed changes

Kristian-ZH force-pushed the loki branch from 6d1ce46 to 113a213 Compare August 17, 2020 09:10

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 17, 2020

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 17, 2020

Kristian-ZH force-pushed the loki branch from 113a213 to 23e4965 Compare August 17, 2020 09:20

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 17, 2020

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 17, 2020

vpnachev reviewed Aug 17, 2020

View reviewed changes

Adapt logs command to Loki

92af02b

Kristian-ZH force-pushed the loki branch from 23e4965 to 92af02b Compare August 17, 2020 12:18

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 17, 2020

Kristian-ZH requested a review from vpnachev August 17, 2020 12:18

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 17, 2020

vpnachev approved these changes Aug 17, 2020

View reviewed changes

neo-liang-sap merged commit ec77b52 into gardener-attic:master Aug 17, 2020

Kristian-ZH mentioned this pull request Sep 29, 2020

Logging stack using Loki gardener/logging#72

Closed

43 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt logs command to Loki #213

Adapt logs command to Loki #213

Kristian-ZH commented Jun 26, 2020

vpnachev commented Jul 31, 2020

vpnachev commented Jul 31, 2020

neo-liang-sap commented Aug 2, 2020

neo-liang-sap commented Aug 10, 2020

neo-liang-sap commented Aug 10, 2020

Kristian-ZH commented Aug 10, 2020

neo-liang-sap commented Aug 10, 2020 •

edited

Loading

Kristian-ZH commented Aug 10, 2020

Kristian-ZH commented Aug 10, 2020

vlvasilev Aug 12, 2020

Kristian-ZH Aug 12, 2020

vpnachev Aug 12, 2020 •

edited

Loading

Kristian-ZH Aug 13, 2020

vpnachev Aug 13, 2020

vpnachev Aug 13, 2020

vlvasilev commented Aug 12, 2020

vpnachev Aug 13, 2020

Kristian-ZH commented Aug 14, 2020 •

edited

Loading

neo-liang-sap commented Aug 14, 2020

Kristian-ZH commented Aug 17, 2020

neo-liang-sap commented Aug 17, 2020

vpnachev left a comment

vpnachev left a comment

Adapt logs command to Loki #213

Adapt logs command to Loki #213

Conversation

Kristian-ZH commented Jun 26, 2020

vpnachev commented Jul 31, 2020

vpnachev commented Jul 31, 2020

neo-liang-sap commented Aug 2, 2020

neo-liang-sap commented Aug 10, 2020

neo-liang-sap commented Aug 10, 2020

Kristian-ZH commented Aug 10, 2020

neo-liang-sap commented Aug 10, 2020 • edited Loading

Kristian-ZH commented Aug 10, 2020

Kristian-ZH commented Aug 10, 2020

vlvasilev Aug 12, 2020

Choose a reason for hiding this comment

Kristian-ZH Aug 12, 2020

Choose a reason for hiding this comment

vpnachev Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Kristian-ZH Aug 13, 2020

Choose a reason for hiding this comment

vpnachev Aug 13, 2020

Choose a reason for hiding this comment

vpnachev Aug 13, 2020

Choose a reason for hiding this comment

vlvasilev commented Aug 12, 2020

vpnachev Aug 13, 2020

Choose a reason for hiding this comment

Kristian-ZH commented Aug 14, 2020 • edited Loading

neo-liang-sap commented Aug 14, 2020

Kristian-ZH commented Aug 17, 2020

neo-liang-sap commented Aug 17, 2020

vpnachev left a comment

Choose a reason for hiding this comment

vpnachev left a comment

Choose a reason for hiding this comment

neo-liang-sap commented Aug 10, 2020 •

edited

Loading

vpnachev Aug 12, 2020 •

edited

Loading

Kristian-ZH commented Aug 14, 2020 •

edited

Loading