Test pods logs collected - Automate BZ1838973 #134

psimovec · 2020-07-07T09:44:25Z

This is my first contribution in golang, any suggestions are welcome!

automates https://bugzilla.redhat.com/show_bug.cgi?id=1838973

Changes to previous code:
upgraded checkPodsLogs to accept regex instead of a string / added possibility to check only new logs... I don't know if the solution I have done is alright, I just did not want to change old function's parameters..

Added:
ExecCmd - execute bash command on a pod
TestPodLogsCollected test case

(In recent PR #124 , waiting time for logs was reduced from 30 mins to 5 mins... But I wait for a fresh archive, and it takes 6 minutes. Is it okay to set it to 10 minutes?) solved

(I will squash commits after review if everything is ok)

openshift-ci-robot · 2020-07-07T09:44:47Z

Hi @psimovec. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

lina-is-here · 2020-07-10T07:44:39Z

test/integration/main_test.go

@@ -79,6 +84,16 @@ func clusterOperatorInsights() *configv1.ClusterOperator {
 	return operator
 }

+
+func clusterOperatorMonitoring() *configv1.ClusterOperator {


I'd suggest to merge it with clusterOperatorInsights and create one function that can check any operator

I have extracted function that can check any operator, it looks cleaner, but kept the functions, so the strings of operator names don't have to be used in code, but keeping the functions might be useless (for now I at least don't touch other tests)

Extracted the function, didn't delete clusterOperatorInsights yet, it will be done in follow-up cleanup PR

lina-is-here · 2020-07-10T07:50:04Z

test/integration/main_test.go

@@ -169,19 +184,24 @@ func restartInsightsOperator(t *testing.T) {
 	t.Log(errPod)
 }

-func checkPodsLogs(t *testing.T, kubeClient *kubernetes.Clientset, message string) {
+func _checkPodsLogs(t *testing.T, kubeClient *kubernetes.Clientset, message string, newLogs bool) {


I'm not sure how underscore function name is used in go, @martinkunc ?

You are right, go devs are a bit stricter about naming. Please use camelCase for private and PascalCase for public methods. Underscores should be only in file names, not in variables. See here: https://golang.org/doc/effective_go.html#mixed-caps

By this naming, it looks like most of the main_test.go functions should also be renamed - most functions are in camelCase but are used as public ones.
Edit: I will do this in follow-up PR with changing panic functions to t.Fatal

Ok. And regarding the public functions. Public in Go means that they are accessible from other packages, which is also making a contract with other packages that they will keep working, which is why we tend not to use them if we don't have to.
In case of tests, I think we are in same packge, so we don't need to expose functions for someone else, except for Test... functions, executed by test runner.

lina-is-here · 2020-07-10T07:52:24Z

test/integration/main_test.go

 	newPods, err := kubeClient.CoreV1().Pods("openshift-insights").List(metav1.ListOptions{})
 	if err != nil {
 		t.Fatal(err.Error())
 	}
-
+	tajm := metav1.NewTime(time.Now())


tajm - is it supposed to be time? :)

oh :D the last variable that made my life happier, forgot to rename it. It was time, but it's already taken, unfortunately :D

martinkunc · 2020-07-10T08:01:14Z

/ok-to-test

martinkunc · 2020-07-10T08:10:13Z

test/integration/main_test.go

+	// get info about insights cluster operator
+	operator, err := configClient.ClusterOperators().Get("monitoring", metav1.GetOptions{})
+	if err != nil {
+		panic(err.Error())


I think generally its better not to use panic in tests. would it be possible to add t *testing.T and use t.Fatal(err) instead ? The reason is to distinguish valid test "failure" from panic when something really unexepected happens.

This will be also done in follow-up cleanup PR, so I don't touch other test functions in this PR

martinkunc · 2020-07-10T08:29:51Z

test/integration/main_test.go

@@ -236,6 +256,100 @@ func forceUpdateSecret(ns string, secretName string, secret *v1.Secret) error {
 	return nil
 }

+func checkPodsLogs(t *testing.T, kubeClient *kubernetes.Clientset, message string) {


Go doesn't have optional parameters, but we are using Functional Options pattern, see here: https://stackoverflow.com/a/26326418
I guess it would be usefull in this case.

nice pattern! I cheated it a little bit with ...bool for one optional parameter only, I think it is way simpler than using options struct with one bool value, which gets set up by function. If you don't like this, I can change it to Functional Options pattern

martinkunc · 2020-07-10T11:58:15Z

test/integration/main_test.go

+	// wait for fresh report
+	checkNewPodsLogs(t, kubeClient, "Writing \\d+ records to")
+	insightsPod := findPod(t, kubeClient, "openshift-insights", "insights-operator")
+	hasLatestArchiveLogs := "tar tf $(ls -dtr /var/lib/insights-operator/* | tail -1)|grep -c \"\\.log$\""


Very nice, but complicated :) Good work.
I am thinking, what if we would try to be more specific with matching just for log our our degraded operator ? Also if you use golang backtick multiline string literal, you woudlnt need to escape, something like:

hasLatestArchiveLogs := ` tar tf $(ls -dtr /var/lib/insights-operator/* | tail -1)|grep -c "^config/pod/cluster-monitoring-operator/logs/.*\.log$" `

Great to know about multi-line string, thanks!
And the specific matching makes more sense.

martinkunc · 2020-07-10T12:07:50Z

test/integration/main_test.go

+	return nil
+}
+
+func PodsLogsExist(t *testing.T, kubeClient *kubernetes.Clientset) bool {


At first I was not exacly sure what this method does just by its name. Pod logs is quite specific, but its not related actually. What about calling it something like ArchiveContainsPodLogs, or anything else :)
Coudln't you also please take out code to degrate in separate function, maybe degradeMonitoring, also this would be both neede to be reverted in the end. For that I would call it alrady from TestPodLogsCollected.
You could use return defer function, something like:

func action() func() { fmt.Println("doing action") return func() { fmt.Println("reverting action") } } func main() { defer action()() fmt.Println("main") }

martinkunc · 2020-07-10T12:13:19Z

Ahh, also it looks e2e tests failed on the build machine. I am guessing that you have used a package, which is not yet in vendor folder in github. When you bring a new package, you need to commit the content of vendor, go.mod and go.sum as well. Please be Please run

make vendor

to keep it clean.

martinkunc · 2020-07-15T06:59:00Z

/test e2e-aws

martinkunc · 2020-07-15T06:59:11Z

/lgtm

added logs check check only fresh logs little refactor the test now reverts its changes new packages, content of vendor force update when changing report time interval to speed it up delete pod to update settings optional parameter deleted useless comment

martinkunc · 2020-07-15T08:09:20Z

/lgtm
/approve

openshift-ci-robot · 2020-07-15T08:09:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: martinkunc, psimovec

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [martinkunc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-07-15T08:36:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 7, 2020

openshift-ci-robot requested review from derekwaynecarr and smarterclayton July 7, 2020 09:44

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 7, 2020

psimovec force-pushed the test_pods_logs_collected branch from a898af6 to 483d01c Compare July 9, 2020 14:29

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2020

psimovec changed the title ~~[WIP] Test pods logs collected - Automate BZ1838973~~ [RFR] Test pods logs collected - Automate BZ1838973 Jul 9, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2020

psimovec changed the title ~~[RFR] Test pods logs collected - Automate BZ1838973~~ Test pods logs collected - Automate BZ1838973 Jul 9, 2020

lina-is-here reviewed Jul 10, 2020

View reviewed changes

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 10, 2020

martinkunc reviewed Jul 10, 2020

View reviewed changes

psimovec changed the title ~~Test pods logs collected - Automate BZ1838973~~ [WIP] Test pods logs collected - Automate BZ1838973 Jul 14, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2020

psimovec changed the title ~~[WIP] Test pods logs collected - Automate BZ1838973~~ Test pods logs collected - Automate BZ1838973 Jul 14, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2020

martinkunc approved these changes Jul 15, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2020

openshift-ci-robot assigned martinkunc Jul 15, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2020

added test case

99a428c

added logs check check only fresh logs little refactor the test now reverts its changes new packages, content of vendor force update when changing report time interval to speed it up delete pod to update settings optional parameter deleted useless comment

psimovec force-pushed the test_pods_logs_collected branch from a2d9e51 to 99a428c Compare July 15, 2020 07:41

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2020

openshift-merge-robot merged commit 1c80079 into openshift:master Jul 15, 2020

psimovec mentioned this pull request Nov 19, 2020

[release-4.5] Backport e2e tests #252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test pods logs collected - Automate BZ1838973 #134

Test pods logs collected - Automate BZ1838973 #134

psimovec commented Jul 7, 2020 •

edited

openshift-ci-robot commented Jul 7, 2020

lina-is-here Jul 10, 2020

psimovec Jul 13, 2020 •

edited

lina-is-here Jul 10, 2020

martinkunc Jul 10, 2020

psimovec Jul 13, 2020 •

edited

martinkunc Jul 15, 2020

lina-is-here Jul 10, 2020

psimovec Jul 13, 2020

martinkunc commented Jul 10, 2020

martinkunc Jul 10, 2020

psimovec Jul 13, 2020

martinkunc Jul 10, 2020

psimovec Jul 14, 2020

martinkunc Jul 10, 2020

psimovec Jul 13, 2020

martinkunc Jul 10, 2020

martinkunc commented Jul 10, 2020

martinkunc commented Jul 15, 2020

martinkunc commented Jul 15, 2020

martinkunc commented Jul 15, 2020

openshift-ci-robot commented Jul 15, 2020

openshift-bot commented Jul 15, 2020

Test pods logs collected - Automate BZ1838973 #134

Test pods logs collected - Automate BZ1838973 #134

Conversation

psimovec commented Jul 7, 2020 • edited

openshift-ci-robot commented Jul 7, 2020

Choose a reason for hiding this comment

psimovec Jul 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psimovec Jul 13, 2020 • edited

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

martinkunc commented Jul 10, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinkunc commented Jul 10, 2020

martinkunc commented Jul 15, 2020

martinkunc commented Jul 15, 2020

martinkunc commented Jul 15, 2020

openshift-ci-robot commented Jul 15, 2020

openshift-bot commented Jul 15, 2020

psimovec commented Jul 7, 2020 •

edited

psimovec Jul 13, 2020 •

edited

psimovec Jul 13, 2020 •

edited