-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Provide configuration property to be able to set docker image build timeout #9259
Conversation
@@ -1341,6 +1332,34 @@ public void shouldThrowIOExceptionWhenBuildImageButNoSuccessMessageInResponse() | |||
verify(dockerResponse).getInputStream(); | |||
} | |||
|
|||
@Test( | |||
expectedExceptions = DockerException.class, | |||
expectedExceptionsMessageRegExp = "Docker image build exceed timeout.*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it better to rewrite it in the following way "Docker image build exceed timeout .* seconds." ?
@@ -900,7 +907,7 @@ private String buildImage( | |||
"Docker image build failed. Image id not found in build output.", 500); | |||
}); | |||
|
|||
return imageIdFuture.get(); | |||
return imageIdFuture.get(imageBuildTimeoutSec, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we interrupt Thread which builds images? Will be building really interrupted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:O good point, @akorneta are you sure that docker build process will be interrupted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sleshchenko Do you know a better approach?
AFAIK from recent versions of Docker, it is a way to perform interruption of the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garagatyi No, I don't. I just ask if it will solve the original issue 😺
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK Docker CLI does the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sleshchenko , @riuvshin so if you carefully look at the implementation of docker image build you will see that in the case when exceptions occur in main(waiting) thread there no any additional work for interruption of building thread the explanation why is placed here.
@@ -267,6 +267,9 @@ che.infra.docker.bootstrapper.server_check_period_sec=3 | |||
# in parallel on workspace startups. | |||
che.infra.docker.max_pull_threads=10 | |||
|
|||
# Time(in seconds) that limits the docker build process. | |||
# The default value is 8 minutes, after which the build will be considered as failed. | |||
che.infra.docker.build_timeout_sec=480 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much time does it take to build dockerfile of Che master in some good case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in master, we do not have any stacks that contain workspace config with dockerfile recipe type but form my custom workspace config docker image build does not take more than 3 minutes. The build process depends on many factors so it is hard to estimate the default value, any suggestions are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Can you take a look at my inlined comment?
@@ -900,7 +907,7 @@ private String buildImage( | |||
"Docker image build failed. Image id not found in build output.", 500); | |||
}); | |||
|
|||
return imageIdFuture.get(); | |||
return imageIdFuture.get(imageBuildTimeoutSec, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an ability to disable timeout? It might be useful for some users.
e661f7e
to
18382b1
Compare
ci-test |
ci-test build report: |
ci-test |
18382b1
to
d28aa67
Compare
ci-test build report: |
d28aa67
to
dcabc47
Compare
dcabc47
to
6c813ef
Compare
What does this PR do?
Provides configuration property which allows setting a limitation for docker image build timeout.
property:
che.infra.docker.build_timeout_sec
default value(experimental):
480 seconds
What issues does this PR fix or reference?
#9130