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

ci: add fluentd basic e2e tests #71

Merged
merged 1 commit into from
Sep 19, 2023
Merged

ci: add fluentd basic e2e tests #71

merged 1 commit into from
Sep 19, 2023

Conversation

ningziwen
Copy link
Member

@ningziwen ningziwen commented Sep 16, 2023

  • breaking out the 'log generation' piece out of the aws log test file into the main test file as func sendTestLogByContainerd so that it can be reused for every driver (which seems like a good idea)
  • add a fluentd test that does validation
  • adds fluentd to the GitHub action (note this launches a docker containerd with the fluentd server)
  • adds fluentd test to makefile

@ningziwen ningziwen force-pushed the fluentd branch 6 times, most recently from 9c41202 to 7ea617e Compare September 16, 2023 23:52
@ningziwen ningziwen changed the title ci: add fluentd e2e tests ci: add fluentd basic e2e tests Sep 16, 2023
@ningziwen ningziwen marked this pull request as ready for review September 16, 2023 23:52
@ningziwen ningziwen requested a review from ginglis13 September 16, 2023 23:57
Makefile Show resolved Hide resolved
@sbuckfelder
Copy link

So looking at this PR. From what I can gather the majority of the work is as follows:

  • breaking out the 'log generation' piece out of the aws log test file into the main test file as func sendTestLogByContainerd so that it can be reused for every driver (which seems like a good idea)
  • add a fluentd test that does validation
  • adds fluentd to the GitHub action (note this launches a docker containerd with the fluentd server)
  • adds fluentd test to makefile

@ningziwen
Copy link
Member Author

So looking at this PR. From what I can gather the majority of the work is as follows:

  • breaking out the 'log generation' piece out of the aws log test file into the main test file as func sendTestLogByContainerd so that it can be reused for every driver (which seems like a good idea)
  • add a fluentd test that does validation
  • adds fluentd to the GitHub action (note this launches a docker containerd with the fluentd server)
  • adds fluentd test to makefile

@sbuckfelder Thanks. Added to the description.

@ningziwen ningziwen force-pushed the fluentd branch 3 times, most recently from 2bc7f0e to 755ec8d Compare September 18, 2023 21:10
e2e/main_test.go Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
e2e/fluentd_test.go Show resolved Hide resolved
@ningziwen ningziwen merged commit 1e30706 into main Sep 19, 2023
11 checks passed
@ningziwen ningziwen deleted the fluentd branch September 19, 2023 01:15
gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
gomega.Expect(code).Should(gomega.Equal(uint32(0)))
func validateTestLogsInAwslogs(client *cloudwatchlogs.Client, logGroupName string, logStreamName string, testLog string) {
cwOutput, err := client.GetLogEvents(context.TODO(), &cloudwatchlogs.GetLogEventsInput{
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider passing in a Ginkgo SpecContext so that this function can time out when the test times out. Same for cleanupAwslogs.

Probably not 100% necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking into the documentation. https://onsi.github.io/ginkgo/#spec-subjects-it

SpecContext is added immediately after ginkgo.It() such as

ginkgo.It("should send logs to fluentd log driver", func(ctx SpecContext) {}

It is usually used for long-running code so may not need for our case.

return err
}
if strings.HasPrefix(info.Name(), "data.") && strings.HasSuffix(info.Name(), ".log") && info.Name() != "data.log" {
fileName = path
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you could add a return after this assignment if you only expect one file to match the if statement's condition

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any difference of whether to return early if only one matched file is always expected?

// 2023-09-16T22:54:11+00:00 123456789012 {"source":"stdout","log":"test-e2e-log","container_id":"123456789012","container_name":"test-container"}
// 2023-09-16T22:54:30+00:00 123456789012 {"container_id":"123456789012","container_name":"test-container","source":"stdout","log":"test-e2e-log"}
// 2023-09-16T22:56:17+00:00 123456789012 {"container_id":"123456789012","container_name":"test-container","source":"stdout","log":"test-e2e-log"}
// The following steps retrieves the "log" field of the third string parsed by tab of the last line to validate the tests sent.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear as to why we only care about the last line's log when all 3 have the "log": "test-e2e-log" data. Could just be because I'm not familiar with this. Could you either add more explanation to the comment or just respond to this comment for my own knowledge?

Copy link
Member Author

Choose a reason for hiding this comment

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

The log events are following time sequence. Each e2e test case only sends one log event. For now there is only one case so it doesn't matter. In the future, if there are multiple test cases, assuming they are running in serial, getting the last line could match the log the current test case just sent.

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.

4 participants