Skip to content

Use different containerd sock address for integration test client#9415

Merged
mikebrow merged 1 commit intocontainerd:mainfrom
kiashok:fixIntegrationClientAddr
Apr 8, 2024
Merged

Use different containerd sock address for integration test client#9415
mikebrow merged 1 commit intocontainerd:mainfrom
kiashok:fixIntegrationClientAddr

Conversation

@kiashok
Copy link
Copy Markdown
Contributor

@kiashok kiashok commented Nov 22, 2023

If the containerd/containerd/defaults.DefaultAddress is used , it causes failures if containerd is already running on the default address and the tests are run without the 'no-daemon' flag.
It would be good to use a different containerd sock address for integration tests

@kiashok kiashok requested review from dcantah, dmcgowan and kzys November 22, 2023 22:43
@dcantah
Copy link
Copy Markdown
Member

dcantah commented Nov 22, 2023 •
edited
Loading

The address is configurable via the flag, can't we just pass the flag when we run our tests? (or locally when a user is running the tests they can provide the flag)

@kiashok
Copy link
Copy Markdown
Contributor Author

kiashok commented Nov 23, 2023 •
edited
Loading

The address is configurable via the flag, can't we just pass the flag when we run our tests? (or locally when a user is running the tests they can provide the flag)

Hmm isn't the -address flag for the tests mostly to know which sock address to connect to? It would be nice to have parity in how the test daemon is started when the '--no-daemon' flag is not specified. That is start on a different non-default sock address for the tests. like npipe:\containerd-containerd-test
Was this change intended to make it to 1.6 as well - f318947 ? Is there any reason we want to have it only in main and 1.7?

@kiashok
Copy link
Copy Markdown
Contributor Author

kiashok commented Nov 23, 2023

The address is configurable via the flag, can't we just pass the flag when we run our tests? (or locally when a user is running the tests they can provide the flag)

Hmm isn't the -address flag for the tests mostly to know which sock address to connect to? It would be nice to have parity in how the test daemon is started when the '--no-daemon' flag is not specified. That is start on a different non-default sock address for the tests. like npipe:\containerd-containerd-test Was this change intended to make it to 1.6 as well - f318947 ? Is there any reason we want to have it only in main and 1.7?

Also, tests like the following try to get the ttrpc address from the "address" set in TestMain() which is a unique test only sock address in 1.6 which makes more sense:

@kiashok
Copy link
Copy Markdown
Contributor Author

kiashok commented Nov 23, 2023

If the test address is now same as the default address for the platform and if this is already open, tests would fail while trying to open same pipe

@kiashok kiashok force-pushed the fixIntegrationClientAddr branch 2 times, most recently from 40054df to 18a48d8 Compare November 23, 2023 04:15
@dcantah
Copy link
Copy Markdown
Member

dcantah commented Nov 23, 2023

Hmm isn't the -address flag for the tests mostly to know which sock address to connect to?

It may be (I'm reviewing on mobile so didn't do great due diligence 😅). The description made it seem like thats the addr it'd listen on if they were launching the daemon

@ruiwen-zhao ruiwen-zhao mentioned this pull request Nov 28, 2023
Merged
@ruiwen-zhao
Copy link
Copy Markdown
Member

ruiwen-zhao commented Nov 29, 2023

/retest

@kiashok kiashok force-pushed the fixIntegrationClientAddr branch from 18a48d8 to c21bc30 Compare November 29, 2023 20:35
@kiashok kiashok changed the title Use different containerd sock address in tests Use different containerd sock address for integration test client Mar 6, 2024
@kiashok kiashok force-pushed the fixIntegrationClientAddr branch 2 times, most recently from e4212a6 to 5f1e9a6 Compare March 6, 2024 23:23
@k8s-ci-robot k8s-ci-robot added size/S and removed size/M labels Mar 6, 2024
@kiashok kiashok force-pushed the fixIntegrationClientAddr branch from 5f1e9a6 to c51ce11 Compare March 6, 2024 23:25
@kiashok kiashok force-pushed the fixIntegrationClientAddr branch from c51ce11 to fe9f2b5 Compare March 21, 2024 04:08
@kiashok kiashok force-pushed the fixIntegrationClientAddr branch 2 times, most recently from 7fb6e0a to 6c4ad26 Compare March 21, 2024 04:58
@kiashok kiashok force-pushed the fixIntegrationClientAddr branch from 6c4ad26 to 00a703e Compare March 21, 2024 05:06
@kiashok
Copy link
Copy Markdown
Contributor Author

kiashok commented Mar 21, 2024

@fuweid @cpuguy83 @mikebrow @MikeZappa87 could you please take a look when you have some time please? Thanks!

@kiashok
Copy link
Copy Markdown
Contributor Author

kiashok commented Mar 25, 2024

@fuweid @cpuguy83 @mikebrow @MikeZappa87 could you please take a look at this PR?

fuweid
fuweid reviewed Mar 26, 2024
View reviewed changes
Comment thread integration/client/client_windows_test.go
@kiashok kiashok force-pushed the fixIntegrationClientAddr branch from 00a703e to fd81b19 Compare March 27, 2024 23:25
@kiashok
Copy link
Copy Markdown
Contributor Author

kiashok commented Mar 28, 2024

/retest

@kiashok kiashok force-pushed the fixIntegrationClientAddr branch from fd81b19 to e7b8a18 Compare March 28, 2024 02:00
@kiashok kiashok force-pushed the fixIntegrationClientAddr branch from e7b8a18 to 5b6ae0f Compare March 28, 2024 02:04
fuweid
fuweid approved these changes Mar 29, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@kiashok kiashok requested review from MikeZappa87 and cpuguy83 April 1, 2024 17:06
mikebrow
mikebrow approved these changes Apr 8, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@mikebrow mikebrow added this pull request to the merge queue Apr 8, 2024
Merged via the queue into containerd:main with commit 406e9e8 Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikebrow mikebrow mikebrow approved these changes

@fuweid fuweid fuweid approved these changes

Assignees

No one assigned

Labels

size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kiashok @dcantah @ruiwen-zhao @mikebrow @fuweid @k8s-ci-robot