Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.9k
Relocate modules#607
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
Uh oh!
There was an error while loading. Please reload this page.
Relocate modules #607
Changes from all commits
749c7566bd213aae5bdc1896339fa4037f8b2351d60dff7e59f1ca7b8bf2d1cb737b9824cc398c298d84ca080d0a479bd3ca4f31878129d660142785e94cd870eb9d632c541b0495e73a4cf0a544b81f4701f7d32cc4633c5df2f77367615e03ce09bc5ebbfb0e4477781656b307977574a753fd69726dFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # Each line is a file pattern followed by one or more owners. | ||
| # These owners will be the default reviewers for everything in | ||
| # the repo. | ||
| * @rnorth @bsideup @kiview | ||
| # The last matching pattern takes the most | ||
| # precedence. | ||
| # Contributed modules can have different reviewers | ||
| modules/mssqlserver/ @StefanHufschmidt @rnorth @bsideup @kiview | ||
| modules/vault/ @mikeoswald @rnorth @bsideup @kiview |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -6,6 +6,7 @@ | ||
| import com.github.dockerjava.api.model.Image; | ||
| import com.github.dockerjava.core.command.PullImageResultCallback; | ||
| import lombok.NonNull; | ||
| import lombok.ToString; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.profiler.Profiler; | ||
| import org.testcontainers.DockerClientFactory; | ||
| @@ -14,30 +15,32 @@ | ||
| import org.testcontainers.utility.DockerLoggerFactory; | ||
| import org.testcontainers.utility.LazyFuture; | ||
| import java.util.Collections; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
| @ToString | ||
| public class RemoteDockerImage extends LazyFuture<String> { | ||
| public static final Set<String> AVAILABLE_IMAGE_NAME_CACHE = new HashSet<>(); | ||
| public static final Set<DockerImageName> AVAILABLE_IMAGE_NAME_CACHE = new HashSet<>(); | ||
| private final String dockerImageName; | ||
| private DockerImageName imageName; | ||
| public RemoteDockerImage(String dockerImageName) { | ||
| DockerImageName.validate(dockerImageName); | ||
| this.dockerImageName = dockerImageName; | ||
| imageName = new DockerImageName(dockerImageName); | ||
| } | ||
| public RemoteDockerImage(@NonNull String repository, @NonNull String tag) { | ||
| this.dockerImageName = repository + ":" + tag; | ||
| imageName = new DockerImageName(repository, tag); | ||
| } | ||
| @Override | ||
| protected final String resolve() { | ||
| Profiler profiler = new Profiler("Rule creation - prefetch image"); | ||
| Logger logger = DockerLoggerFactory.getLogger(dockerImageName); | ||
| Logger logger = DockerLoggerFactory.getLogger(imageName.toString()); | ||
| profiler.setLogger(logger); | ||
| Profiler nested = profiler.startNested("Obtaining client"); | ||
| @@ -48,58 +51,64 @@ protected final String resolve() { | ||
| profiler.start("Check local images"); | ||
| int attempts = 0; | ||
| DockerClientException lastException = null; | ||
| while (true) { | ||
| // Does our cache already know the image? | ||
| if (AVAILABLE_IMAGE_NAME_CACHE.contains(dockerImageName)) { | ||
| logger.trace("{} is already in image name cache", dockerImageName); | ||
| if (AVAILABLE_IMAGE_NAME_CACHE.contains(imageName)) { | ||
| logger.trace("{} is already in image name cache", imageName); | ||
| break; | ||
| } | ||
| // Update the cache | ||
| ListImagesCmd listImagesCmd = dockerClient.listImagesCmd(); | ||
| if (Boolean.parseBoolean(System.getProperty("useFilter"))) { | ||
| listImagesCmd = listImagesCmd.withImageNameFilter(dockerImageName); | ||
| listImagesCmd = listImagesCmd.withImageNameFilter(imageName.toString()); | ||
| } | ||
| List<Image> updatedImages = listImagesCmd.exec(); | ||
| for (Image image : updatedImages) { | ||
| if (image.getRepoTags() != null) { | ||
| Collections.addAll(AVAILABLE_IMAGE_NAME_CACHE, image.getRepoTags()); | ||
| } | ||
| } | ||
| updatedImages.stream() | ||
| .map(Image::getRepoTags) | ||
| .filter(Objects::nonNull) | ||
| .flatMap(Stream::of) | ||
| .map(DockerImageName::new) | ||
| .collect(Collectors.toCollection(() -> AVAILABLE_IMAGE_NAME_CACHE)); | ||
| // And now? | ||
| if (AVAILABLE_IMAGE_NAME_CACHE.contains(dockerImageName)) { | ||
| logger.trace("{} is in image name cache following listing of images", dockerImageName); | ||
| if (AVAILABLE_IMAGE_NAME_CACHE.contains(imageName)) { | ||
| logger.trace("{} is in image name cache following listing of images", imageName); | ||
| break; | ||
| } | ||
| // Log only on first attempt | ||
| if (attempts == 0) { | ||
| logger.info("Pulling docker image: {}. Please be patient; this may take some time but only needs to be done once.", dockerImageName); | ||
| logger.info("Pulling docker image: {}. Please be patient; this may take some time but only needs to be done once.", imageName); | ||
| profiler.start("Pull image"); | ||
| } | ||
| if (attempts++ >= 3) { | ||
| logger.error("Retry limit reached while trying to pull image: " + dockerImageName + ". Please check output of `docker pull " + dockerImageName + "`"); | ||
| throw new ContainerFetchException("Retry limit reached while trying to pull image: " + dockerImageName); | ||
| logger.error("Retry limit reached while trying to pull image: {}. Please check output of `docker pull {}`", imageName, imageName); | ||
| throw new ContainerFetchException("Retry limit reached while trying to pull image: " + imageName, lastException); | ||
| } | ||
| // The image is not available locally - pull it | ||
| try { | ||
| dockerClient.pullImageCmd(dockerImageName).exec(new PullImageResultCallback()).awaitCompletion(); | ||
| } catch (InterruptedException e) { | ||
| throw new ContainerFetchException("Failed to fetch container image for " + dockerImageName, e); | ||
| final PullImageResultCallback callback = new PullImageResultCallback(); | ||
| dockerClient | ||
| .pullImageCmd(imageName.getUnversionedPart()) | ||
| .withTag(imageName.getVersionPart()) | ||
| .exec(callback); | ||
| callback.awaitSuccess(); | ||
| AVAILABLE_IMAGE_NAME_CACHE.add(imageName); | ||
| break; | ||
| } catch (DockerClientException e) { | ||
| lastException = e; | ||
| } | ||
| // Do not break here, but step into the next iteration, where it will be verified with listImagesCmd(). | ||
| // see https://github.com/docker/docker/issues/10708 | ||
MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This issue is resolved as of Jan 2016 so I don't think we need to worry about it any more. Accordingly, I've tidied the code a bit. Crucially the control flow now breaks out early if the image has been successfully pulled (line 96), instead of re-fetching the image listing. This is actually important in letting us pull by sha256 hash: the listing doesn't include the same hash as the repository, so we can't use the listing to check for its presence (possibly something we can fix upstream in docker-java for better performance) Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we also add a test for pulling with sha256 hash? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. While I was at it I tidied the image name handling code and added more tests relating to image names/image pulling. | ||
| } | ||
| return dockerImageName; | ||
| return imageName.toString(); | ||
| } catch (DockerClientException e) { | ||
| throw new ContainerFetchException("Failed to get Docker client for " + dockerImageName, e); | ||
| throw new ContainerFetchException("Failed to get Docker client for " + imageName, e); | ||
| } finally { | ||
| profiler.stop().log(); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
@bsideup I've split the CI build further so we now have four concurrent build jobs. I'm just wondering if I've missed anything: you'd excluded certain check tasks, whereas I've gone on an inclusive basis. This looks OK to me in local builds (i.e. it's compiling but not running tests on the core dependency, which seems correct).