Skip to content

Add retries for docker-compose cmd in integration tests#32772

Closed
kssenii wants to merge 2 commits intoClickHouse:masterfrom
kssenii:add-retries
Closed

Add retries for docker-compose cmd in integration tests#32772
kssenii wants to merge 2 commits intoClickHouse:masterfrom
kssenii:add-retries

Conversation

@kssenii
Copy link
Copy Markdown
Member

@kssenii kssenii commented Dec 14, 2021

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Should resolve errors like

Exception: Command ['docker-compose', '--env-file', '/ClickHouse/tests/integration/test_blob_storage_zero_copy_replication/_instances_0/.env', '--project-name', 'roottestblobstoragezerocopyreplication', '--file', '/compose/docker_compose_azurite.yml', '--verbose', 'up', '-d'] return non-zero code 1
...
               compose.service.pull: Pulling azurite1 (mcr.microsoft.com/azure-storage/azurite:)...
E               compose.cli.verbose_proxy.proxy_callable: docker pull <- ('mcr.microsoft.com/azure-storage/azurite', tag='latest', stream=True, platform=None)
E               compose.cli.errors.log_api_error: Get "https://mcr.microsoft.com/v2/": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Dec 14, 2021
@alesapin
Copy link
Copy Markdown
Member

alesapin commented Dec 15, 2021

Ok.

azat
azat reviewed Dec 15, 2021
View reviewed changes
tests/integration/helpers/cluster.py
if self.with_mysql_client and self.base_mysql_client_cmd:
logging.debug('Setup MySQL Client')
subprocess_check_call(self.base_mysql_client_cmd + common_opts)
subprocess_check_call(self.base_mysql_client_cmd + common_opts, num_tries=DOCKER_COMPOSE_CMD_NUM_TRIES)
Copy link
Copy Markdown
Member

@azat azat Dec 15, 2021

Choose a reason for hiding this comment

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

Interesting, there was already pull with retries, but this had been removed in 10b7037, in the merge commit, maybe @qoega can comment on this?

But AFACIS it will not pull images with separate docker-compose.yml files, i.e. mysql and similar.

tests/integration/helpers/cluster.py
if i == num_tries - 1:
raise ex
else:
logging.info("Got execption while executing command: %s", ex)
Copy link
Copy Markdown
Member

@azat azat Dec 15, 2021

Choose a reason for hiding this comment

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

There is already retry_exception(), and I think adding delay between retries may help a little.

Copy link
Copy Markdown
Member Author

@kssenii kssenii Dec 15, 2021

Choose a reason for hiding this comment

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

Though it does not show proper exception:

ClickHouse/tests/integration/helpers/cluster.py

Line 107 in e5c538f

raise StopIteration('Function did not finished successfully')

@Algunenano Algunenano mentioned this pull request Dec 15, 2021
Merged
@azat azat mentioned this pull request Dec 16, 2021
Merged
@alesapin
Copy link
Copy Markdown
Member

alesapin commented Dec 16, 2021

Didn't help?

@kssenii
Copy link
Copy Markdown
Member Author

kssenii commented Dec 16, 2021 •
edited
Loading

Not sure, I added retries not for all such docker command runs but only for those which run with subsprocess_check_all, those which are started with check_and_run I did not add, they are the minority I thought.. (about 2-3 of them remained)

@qoega
Copy link
Copy Markdown
Member

qoega commented Dec 16, 2021

I vote for our docker proxies. If compose/hub fails it is a reason to check why..

@kssenii kssenii closed this Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat left review comments

@alesapin alesapin alesapin approved these changes

Assignees

No one assigned

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kssenii @alesapin @qoega @azat @robot-clickhouse