Skip to content

test(systest): avoid host volume mount in minio test#8569

Merged
mangalaman93 merged 1 commit intomainfrom
aman/minio
Jan 5, 2023
Merged

test(systest): avoid host volume mount in minio test#8569
mangalaman93 merged 1 commit intomainfrom
aman/minio

Conversation

@mangalaman93
Copy link
Copy Markdown
Contributor

@mangalaman93 mangalaman93 commented Jan 4, 2023 •
edited
Loading

instead of a host mount, we do everything inside the container now. This avoids creating any files on the host and the delete error on during cleanup on the CI do not happen.

@github-actions github-actions Bot added the area/testing Testing related issues label Jan 4, 2023
all-seeing-code
all-seeing-code reviewed Jan 4, 2023
View reviewed changes
Comment thread testutil/minio.go
all-seeing-code
all-seeing-code reviewed Jan 4, 2023
View reviewed changes
Comment thread systest/bulk_live/live/docker-compose.yml
@all-seeing-code all-seeing-code self-requested a review January 4, 2023 19:16
all-seeing-code
all-seeing-code approved these changes Jan 4, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@all-seeing-code all-seeing-code left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please take a look at some comments above.

skrdgraph
skrdgraph approved these changes Jan 4, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@skrdgraph skrdgraph left a comment

Choose a reason for hiding this comment

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

@aman-bansal - this looks good to merge. Thanks!

The above comments are nitpicks .. and can be addressed when we move to compose tool later.

@skrdgraph
Copy link
Copy Markdown
Contributor

skrdgraph commented Jan 4, 2023

I have re-triggered a CI run .. once you have all passing - pls merge this off.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 4, 2023 •
edited
Loading

Coverage Status

Coverage: 64.308% (-0.1%) from 64.409% when pulling 26d1f15 on aman/minio into 32de2e4 on main.

@all-seeing-code
Copy link
Copy Markdown
Contributor

all-seeing-code commented Jan 4, 2023

Test failure seems relevant to the change. @mangalaman93 highlighting it for you.

Caught output:
=== CONT  TestBulkCases/bulk_test_cases
    bulk_live_fixture.go:84: 
        	Error Trace:	bulk_live_fixture.go:84
        	            				bulk_live_cases.go:151
        	            				bulk_live_cases.go:52
        	Error:      	Received unexpected error:
        	            	Get "http://localhost:9001/data/?location=": dial tcp 127.0.0.1:9001: connect: connection refused
        	Test:       	TestBulkCases/bulk_test_cases
--- FAIL: TestBulkCases (7.87s)
    --- FAIL: TestBulkCases/bulk_test_cases (7.87s)
        --- PASS: TestBulkCases/bulk_test_cases/Pan_and_Jackson (0.10s)
        --- PASS: TestBulkCases/bulk_test_cases/Pan_only (0.00s)
        --- PASS: TestBulkCases/bulk_test_cases/Jackson_only (0.00s)
       ```

@skrdgraph
Copy link
Copy Markdown
Contributor

skrdgraph commented Jan 5, 2023 •
edited
Loading

I think this should be hitting minio:9001 instead of localhost:9001

Received unexpected error:
[1069](https://github.com/dgraph-io/dgraph/actions/runs/3840357871/jobs/6543986295#step:11:1070)
        	            	Get "http://localhost:9001/data/?location=": dial tcp 127.0.0.1:9001: connect: connection refused

@skrdgraph
Copy link
Copy Markdown
Contributor

skrdgraph commented Jan 5, 2023

related PR #8576 (comment)

@mangalaman93
test(live): avoid host volume mount in minio test
26d1f15 
The host mount would have files created as root causing the
cleanup to fail on the CI.
@mangalaman93
Copy link
Copy Markdown
Contributor Author

mangalaman93 commented Jan 5, 2023

I have no idea how these tests have been passing. It could be because of differences in the docker version or something. I have fixed the tests now.

@skrdgraph
Copy link
Copy Markdown
Contributor

skrdgraph commented Jan 5, 2023

I have no idea how these tests have been passing. It could be because of differences in the docker version or something. I have fixed the tests now.

there was a bunch of docker related issues which we have also fixed in the t.go file. Mostly because the old team was using cmd exec from go-code ... and those cli parsers have changed overtime with newer docker builds. I made those changes with @dshekhar95 initially while setting up CI.

It could be something related to host-networking as well... sorry to have rabbit-holed you into this.

@mangalaman93
Copy link
Copy Markdown
Contributor Author

mangalaman93 commented Jan 5, 2023

yeah, it is a bit cleaner now and I will spend some time after slash main alignment on improving our test infra.

@mangalaman93 mangalaman93 merged commit 900a8b3 into main Jan 5, 2023
@mangalaman93 mangalaman93 deleted the aman/minio branch January 5, 2023 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akon-dey akon-dey Awaiting requested review from akon-dey

@darkn3rd darkn3rd Awaiting requested review from darkn3rd

@meghalims meghalims Awaiting requested review from meghalims

@matthewmcneely matthewmcneely Awaiting requested review from matthewmcneely

@billprovince billprovince Awaiting requested review from billprovince

2 more reviewers

@skrdgraph skrdgraph skrdgraph approved these changes

@all-seeing-code all-seeing-code all-seeing-code approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/testing Testing related issues

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mangalaman93 @skrdgraph @coveralls @all-seeing-code