-
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
Implement OpenShift pull, tag, and inspectImage #3956
Implement OpenShift pull, tag, and inspectImage #3956
Conversation
Can one of the admins verify this patch? |
ci-build |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1819/ |
.getItems(); | ||
|
||
if (pods.size() < 1) { | ||
throw new DockerException(String.format("Pod with deployment name %s not found", |
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.
@amisevsk might be more relevant to throw OpenShiftException now ?
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.
One reason I didn't do this is that OpenShiftException is unchecked, and so it wouldn't be obvious that it could be thrown when using the methods. DockerException is an IOException.
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.
With the latest commit, I've changed OpenShiftException to a subclass of IOException (like DockerException) and used it instead of DockerException.
@@ -23,17 +23,17 @@ | |||
/** | |||
* Provides API for managing Kubernetes {@link EnvVar} | |||
*/ | |||
public class KubernetesEnvVar { | |||
public final class KubernetesEnvVar { |
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.
@amisevsk for all final util classes I would also add private constructor
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.
Fixed
@amisevsk you need to rebase your branch (there is a conflict) |
a75cbf3
to
307d5b3
Compare
@amisevsk works just fine in most cases. However, I can reproduce pretty often the following error: |
@ibuziuk sounds like a likely issue; I'll look into it. |
@ibuziuk I've been able to reproduce the issue occasionally, but yeah, it doesn't happen when debugging or if the image has already been pulled. It's happening because OpenShift is taking longer than 5 seconds to get metadata about the docker repository. I've increased the max wait time to 10 seconds, which will hopefully fix the issue. However, since it doesn't always occur, I can't be sure. If it does occur again, could you manually check that the data is pulled (Builds -> Images -> <image-name> in OpenShift console)? That would confirm that it's just a timeout issue. OpenShift doesn't seem to offer a way to monitor the progress here, so we are kind of stuck waiting. The (now 20 second) maximum wait time between pull and tag shouldn't have much of an impact in practical use, since the image stream tags are created quickly once the image has been pulled. |
.inNamespace(openShiftCheProjectName) | ||
.withName(imageStreamName) | ||
.get(); | ||
String registryAddress = imageStream.getStatus() |
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.
I would test if imageStream is not null before using it
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.
Fixed.
String imageStreamTagPullSpec = imageStreamTag.getMetadata().getName(); | ||
|
||
// Next we need to get the address of the registry where the ImageStreamTag is stored | ||
String imageStreamName = imageStreamTagPullSpec.split(":")[0]; |
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.
It is safer to check if variable imageStreamTagPullSpec
contains ":" before calling split()
. You should do the same for all calls to the split()
method.
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.
Agreed. I didn't do it before because OpenShift should complain that an ImageStreamTag name does not contain ":", but I'm not sure. Fixed.
@@ -408,6 +434,194 @@ public Network inspectNetwork(InspectNetworkParams params) throws IOException { | |||
.withEnableIPv6(false); | |||
} | |||
|
|||
@Override | |||
public void pull(final PullParams params, final ProgressMonitor progressMonitor) throws IOException { | |||
// This method does not cause the relevant image to actually be pulled to the local |
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.
You may use this comment for pull()
javadoc
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.
Good idea -- done.
ci-build |
Implement OpenShiftConnector pull, tag, and inspectImage through ImageStreams. Makes changes to how containers are created. Currently snapshots do not work, as commit and push methods are not implemented. Additionally, pulling a stack from a private repository is not supported. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
39299c0
to
3bad564
Compare
I've squashed and rebased onto master. Additionally, I've added a minor fix for tests broken by commit 41541e0. |
Implement OpenShiftConnector
pull
,tag
, andinspectImage
throughImageStreams. Makes changes to how containers are created.
Currently snapshots do not work, as
commit
andpush
methods are notimplemented (these are being worked on by @ibuziuk currently). Additionally,
pulling a stack from a private repository is not yet implemented.
The changes necessitate reworking
OpenShiftConnector#createContainer()
to accomodate using ImageStreams instead of the underlying Docker implementation.
What does this PR do?
Adds implementations of
pull
,tag
andinspectImage
to OpenShiftConnector.Additionally, this PR refactors the helper methods used in OpenShiftConnector to be
static, as they store no state and serve as utility methods. This makes OpenShiftConnector
somewhat cleaner.
Changelog
Implement OpenShiftConnector
pull
,tag
, andinspectImage
throughImageStreams. Makes changes to how containers are created.
Docs Pull Request
https://github.com/eclipse/che-docs/issues/76