Skip to content
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

dockerExecute with dockerVolumeBind is now broken #4705

Closed
vtintillier opened this issue Dec 4, 2023 · 12 comments
Closed

dockerExecute with dockerVolumeBind is now broken #4705

vtintillier opened this issue Dec 4, 2023 · 12 comments

Comments

@vtintillier
Copy link
Member

Hello

since #4673 was delivered, our pipeline is broken. We have this in our scripted pipeline:

    def dotCf = createDir('.cf')

    dockerExecute(script: script, dockerImage: 'public.int.repositories.cloud.sap/ppiper/cf-cli:v14', dockerVolumeBind: "${dotCf}:/.cf") {

This now fails with

java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
	at org.codehaus.groovy.runtime.dgmimpl.arrays.ObjectArrayGetAtMetaMethod.invoke(ObjectArrayGetAtMetaMethod.java:41)
	at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:325)
	at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1225)
	at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1034)
	at org.codehaus.groovy.runtime.callsite.PojoMetaClassSite.call(PojoMetaClassSite.java:46)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:47)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:116)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:128)
	at dockerExecute.containerMountPathFromVolumeBind(dockerExecute.groovy:390)

Thank you for your help.

@anilkeshav27
Copy link
Member

@c0d1ngm0nk3y , this seems to have broken pipelines that run in jenkins instance where volume binds can be done without any restrictions

@anilkeshav27
Copy link
Member

@vtintillier , do you run on your custom jenkins instance ?

@vtintillier
Copy link
Member Author

We run on a JaaS instance

@anilkeshav27
Copy link
Member

anilkeshav27 commented Dec 4, 2023

We run on a JaaS instance

afaik JaaS instance did not allow volume binds and this was not considered downstream when Piper executed on dockerExecuteOnKuberenetes . till now the parameter was there but it really did not translate anything in the JaaS pod. did this volume bind work earlier ? ,

@vtintillier
Copy link
Member Author

We've always used this volume bind. Let me try what happens if we remove it.

@phil9909
Copy link
Contributor

phil9909 commented Dec 4, 2023

The root of the ArrayIndexOutOfBoundsException seems to be, that we expect dockerVolumeBind to be an Array of Strings, but @vtintillier passes a String (something like "/jenkinsdata/[...]:/.cf"). We intended to take the first item of the Array and calling split on it. Instead we will now take the first char (/ in this case) and call split on this. As it contains no : the subsequent array access fails. See:

return dockerVolumeBind[0].split(":")[1]

But as @anilkeshav27 already mentioned: Previously the dockerVolumeBind option was ignored when running on Kubernetes. Therefore your Pipeline seems to work without it and you can simply drop the dockerVolumeBind option (and probably also the createDir call)

@c0d1ngm0nk3y
Copy link
Member

@vtintillier Like Philipp already mentioned, the dockerVolumeBind property is documented as "docker only" and was ignored on kubernetes. So it had no effect. Now, it is actually used and fails since you provide only a string. But what you want to do is still not supported. On kubernetes, you can only mount an empty directory.

So I would propose, you just remove the dockerVolumeBind property from your pipeline, since it had no effect anyway.

@vtintillier
Copy link
Member Author

I confirm it works without the dockerVolumeBind. It was something we carried over when converting from Jenkins native docker.image(...).inside(...) (running on agent with docker installed) when moving to piper+k8s.

@vtintillier
Copy link
Member Author

Thank you all for your help!

@vtintillier vtintillier closed this as not planned Won't fix, can't repro, duplicate, stale Dec 4, 2023
@c0d1ngm0nk3y
Copy link
Member

@c0d1ngm0nk3y , this seems to have broken pipelines that run in jenkins instance where volume binds can be done without any restrictions

But this never worked anyway and even on docker, it is expected in a different format.

@Kiemes
Copy link
Member

Kiemes commented Dec 5, 2023

Thanks for the volume bind option for K8S.
This also broke one of our pipelines because of a copy&paste issue where a dockerVolumeBind parameter was specified but never needed. Removing it solved the issue in that case for us as well.

However, I wonder why we do now use different formats of the dockerVolumeBind parameter for K8S and Jenkins worker nodes?
We successfully use this notation dockerVolumeBind: ['/home/ccloud/.m2': '/home/piper/.m2'] on our worker node (ccloud).
This notation doesn't work on K8S pods now.
Shouldn't we allow the same notations in both cases?

@anilkeshav27
Copy link
Member

We successfully use this notation dockerVolumeBind: ['/home/ccloud/.m2': '/home/piper/.m2'] on our worker node (ccloud).

@Kiemes , the volume bind we have allowed is very restricted , it only allow a source emptyDir : target can be any location the user wants inside the container. its more the container adding content to the volume when it runs and not to bring in a volume from the host. We cannot support the standard notion sourceDir:targetDir since the k8s is a shared cluster in jaas which is a security risk of allowing same path to interfere in 2 pipeline runs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants