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

[JENKINS-54295] Fix docker image #456

Merged
merged 6 commits into from Nov 10, 2018
Merged

Conversation

raul-arabaolaza
Copy link
Contributor

@raul-arabaolaza raul-arabaolaza commented Oct 28, 2018

JENKINS-54295 This version was working perfectly for the 1.62 tag

  • DO NOT MERGE YET, I HAVE CREATED THIS PR TO VALIDATE MY THEORY*

This version was working perfectly for the 1.62 tag
@olivergondza
Copy link
Member

@lvotypko, will this work as a temporary measure?

@raul-arabaolaza
Copy link
Contributor Author

@olivergondza This PR is not yet complete, I need more things to investigate given the error. We already have temporary measures in core that can be easily applied here by using jenkins/ath:acceptance-test-harness-1.62 instead of latest.
Still working on fix the master build, I will ping you when this works

@lvotypko
Copy link
Member

Hi, I am working on upgrade to selenium 3, because we can not be on current selenium version forever. But it takes me more time then I expected, so I am fine with setting the version of Firefox as temporary fix (I think it should be something lesser then 58, but not sure if I remember it well) and consider it as permanent setting. Because sometime it is hard to make it work for any browser version.
error in build is caused wrong url for docker binaries. Try to change it for this one https://download.docker.com/linux/static/stable/x86_64/docker-18.06.1-ce.tgz

@raul-arabaolaza
Copy link
Contributor Author

Temporarily clossing to try to trigger a new PR builder run as the build got a temporarily unavailable error from debian apt repos

@raul-arabaolaza raul-arabaolaza changed the title [JENKINS-54295] Set the proper version of firefox-esr [JENKINS-54295] Fix docker image Oct 30, 2018
@raul-arabaolaza
Copy link
Contributor Author

Hi, I am working on upgrade to selenium 3, because we can not be on current selenium version forever. But it takes me more time then I expected, so I am fine with setting the version of Firefox as temporary fix (I think it should be something lesser then 58, but not sure if I remember it well) and consider it as permanent setting. Because sometime it is hard to make it work for any browser version.

I fully agree with updating the selenium version, several previous tries have proven how difficult it is so I fully understand you. I would suggest that as part of that change the firefox version in the docker image has to be also updated to the proper version so people that runs the ATH using the docker image are not affected by the change. In any case we need to start fixing things in the docker image so changes in the deb repos (like a new version of openjdk) do not make things fail without obvious reasons and the docker image is more deterministic.

@raul-arabaolaza
Copy link
Contributor Author

The current failure is due to a breaking change introduced in the last openjdk8-jdk debian package (see ticket for details). My PR includes a workaround for that but as I am not a trusted source is not evaluated.

Loading trusted files from base branch master at 3618c0c3c2aa5b6318b2d990c3181fd93fc37972 rather than a52ba2b8403220b3d85725d2a2c63eaf15fb10ac
Obtained Jenkinsfile from 3618c0c3c2aa5b6318b2d990c3181fd93fc37972

@olivergondza or @lvotypko can you run my changes to check the validity? It worked on my local runs but I have no way to try them in the PR Builder

@lvotypko
Copy link
Member

@raul-arabaolaza, I use the same workaround for selenium3 and it works on our machines. I will try to run your branch too.

@raul-arabaolaza
Copy link
Contributor Author

@lvotypko any news?

@lvotypko
Copy link
Member

lvotypko commented Nov 6, 2018

Hello, I realized in my pull request that there is a problem with version of docker, so we need binaries closer to previous version (there is a problem with sharing docker.sock). In my pull request it worked with docker-17.03.2. But your test failed even before. I will try to run it from Jenkinsfile (I do not use it because we have a different enviromnet and it needs a few modifications) and find out what is wrong. I had to use more memory with selenium 3. It is possible that current memory given by default is not enough even for selenium 2. But it is only theory.

@lvotypko
Copy link
Member

lvotypko commented Nov 7, 2018

I tried it several time locally and have no problem with jvm crash. But you will need to change docker version too.

RUN curl -fsSLO https://download.docker.com/linux/static/stable/x86_64/docker-17.03.2-ce.tgz && \
    tar --strip-components=1 -xvzf docker-17.03.2-ce.tgz -C /usr/local/bin

Try to commit it and we will see, if it fails again.

@raul-arabaolaza
Copy link
Contributor Author

Yeah, will do. Thanks!

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Or just update Surefire in the POM instead.

@@ -13,15 +13,15 @@ RUN apt-get clean && \
git \
imagemagick \
iptables \
firefox-esr \
firefox-esr=52.9.0esr-1~deb9u1 \
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I have had serious problems specifying exact versions in apt commands in Dockerfiles: after security fixes are released, vulnerable versions are deleted from the repositories, and new docker builds start failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jglick This is only needed until #457 gets merged, I am not very fond to do big changes in the Dockerfile until then... Just the minimum to get master working again.

@raul-arabaolaza
Copy link
Contributor Author

@lvotypko Changed the docker version as you suggested

@raul-arabaolaza raul-arabaolaza mentioned this pull request Nov 7, 2018
@lvotypko
Copy link
Member

lvotypko commented Nov 8, 2018

Hello,
for some reason in upstream log there is:
mvn test -Dmaven.test.failure.ignore=true -DforkCount=1 -B

but it should be:
mvn test -Dmaven.test.failure.ignore=true -DforkCount=1 -DargLine="-Djdk.net.URLClassPath.disableClassPathURLCheck=true" -B

When I run your branch in my environment, I have desired
-DargLine="-Djdk.net.URLClassPath.disableClassPathURLCheck=true"

I do not understand why it is not here... Any idea?

@lvotypko
Copy link
Member

lvotypko commented Nov 8, 2018

I think I have an idea why it happens. But it is only theory... if the build is started with master Jenkinsfile and after the change is merged, it is running with Jenkinsfile on master not with Jenkinsfile in your branch. But it is only theory, otherwise I am not able to explain this behavior.

@lvotypko
Copy link
Member

lvotypko commented Nov 8, 2018

I think this is the cause of it. But why git does it?
Loading trusted files from base branch master at 3618c0c rather than 932ce30

@raul-arabaolaza
Copy link
Contributor Author

That is a security feature, changes in Jenkinsfiles done by PR are only used if the owner of the PR is a trusted source, which for the ATH I am not :) That is the reason the pr builder for this change is not accurate and I am requesting you to run my changes

@lvotypko
Copy link
Member

lvotypko commented Nov 9, 2018

Ok, now we can merge the commit. We can merge the commit so we have at least some results then #457 is merged. I hope it will be sooner then new version of Firefox. Thank you!

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

lgtm

@lvotypko lvotypko merged commit 7624292 into jenkinsci:master Nov 10, 2018
@raul-arabaolaza raul-arabaolaza deleted the patch-1 branch November 12, 2018 10:51
@raul-arabaolaza
Copy link
Contributor Author

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

5 participants