Skip to content

Conversation

@MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Jun 3, 2023

Download from mirrors instead of repo.jenkins-ci.org

Reduce bandwidth use for container builds by downloading the Jenkins war file from the nearest mirror site rather than downloading from repo.jenkins-ci.org.

  • Download war file from Jenkins mirrors
  • Include RELEASE_LINE in build scripts

Testing done

Ran Linux based build and test with make build and make test

Did not run Windows based tests, though attempted to make the necessary adjustments to Windows scripts for them to be tested in CI.

I'm not 100% confident that all the cases are handled for LTS builds on Windows. The automated tests and my interactive tests on Linux are all passing and well behaved. I was expecting to need more changes for Windows builds, but did not find the locations that needed the changes.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Reduce bandwidth demands on artifactory

Faster downloads
Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

This PR is good to me, nice catch!

Only a nitpick (not blocking at all): the name DOWNLOAD_DIR is a bit confusing as it (for me) seems related to destionation (e.g. the location of the downloaded file) while it's on the origin URL.

WDYT about RELEASE_LINE, JENKINS_RELEASE_LINE, JENKINS_LINE or something like these?

@MarkEWaite
Copy link
Contributor Author

Only a nitpick (not blocking at all): the name DOWNLOAD_DIR is a bit confusing as it (for me) seems related to destionation (e.g. the location of the downloaded file) while it's on the origin URL.

WDYT about RELEASE_LINE, JENKINS_RELEASE_LINE, JENKINS_LINE or something like these?

I like that suggestion. I've changed DOWNLOAD_DIR to RELEASE_LINE.

Comment on lines 64 to 80
if [[ "${JENKINS_VERSION}" == "${latest_weekly_version}" ]]
then
LATEST_WEEKLY="true"
RELEASE_LINE="war"
else
LATEST_WEEKLY="false"
RELEASE_LINE="war-stable"
fi

if [[ "${JENKINS_VERSION}" == "${latest_lts_version}" ]]
then
LATEST_LTS="true"
RELEASE_LINE="war-stable"
else
LATEST_LTS="false"
RELEASE_LINE="war"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if [[ "${JENKINS_VERSION}" == "${latest_weekly_version}" ]]
then
LATEST_WEEKLY="true"
RELEASE_LINE="war"
else
LATEST_WEEKLY="false"
RELEASE_LINE="war-stable"
fi
if [[ "${JENKINS_VERSION}" == "${latest_lts_version}" ]]
then
LATEST_LTS="true"
RELEASE_LINE="war-stable"
else
LATEST_LTS="false"
RELEASE_LINE="war"
fi
# Assume weekly line by default
RELEASE_LINE="war"
if [[ "${JENKINS_VERSION}" == "${latest_weekly_version}" ]]
then
LATEST_WEEKLY="true"
else
LATEST_WEEKLY="false"
fi
if [[ "${JENKINS_VERSION}" == "${latest_lts_version}" ]]
then
LATEST_LTS="true"
RELEASE_LINE="war-stable"
else
LATEST_LTS="false"
fi

Nitpick: simpler code to avoid too many "if / else" cases

For the Linux publication, I wonder if we cannot use the same "regex parsing" trick you added in the powershell tests, but inside the docker-bake.hcl.

That would determine the release line on a single location for all linux images?

=> this suggestion and the question are NOT blocking the PR at all. It's about code simplification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer. While staring at the logic thanks to your question, I realized that there is a flaw in the technique that I used.

The choice of release line should be distinct from the determination if this is a latest weekly version or if it is a latest LTS version. If it is not a latest weekly version and not a latest LTS version (maybe we're tagging an older LTS or an older weekly), then the release line determination is incorrect in the code as I implemented it.

I'll rework that code to correctly determine the release line based on the Jenkins version, without involving the calculation of LATEST_LTS or LATEST_WEEKLY.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed that logic error in 9b014ed. Thanks so much for detecting that mistake. Future me thanks you very much!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the Linux publication, I wonder if we cannot use the same "regex parsing" trick you added in the powershell tests, but inside the docker-bake.hcl.

I think that is the best approach because it will remove the logic from the publish.sh script completely. I'll see if I can implement that logic within the next day or two.

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

2 non blocking elements, might be worth a subsequent PR

I previously made the mistake of attempting to reuse the logic that
was deciding whether this is the latest weekly or the latest LTS.
Reusing that logic fails to determine the correct release line for a
non-latest release.

If Jenkins 2.500 has already released but we need to build 2.499, then
LATEST_WEEKLY will be false and RELEASE_LINE should be "war".

If Jenkins 2.500.2 has already released but we need to build 2.500.1,
then LATEST_LTS will be false and RELEASE_LINE should be "war-stable".

The RELEASE_LINE calculation needs to be independent of the
calculation of latest releases, since they have different conditions.
Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

👍

Use a regular expression in the docker-bake.hcl to calculate the
RELEASE_LINE so that the Dockerfile does not need to become more
complicated and so that the caller does not need to provide a release
line.  Calculate the release line based on the Jenkins version.
Remove references to RELEASE_LINE that are not needed
@MarkEWaite MarkEWaite requested a review from dduportal June 4, 2023 20:09
@MarkEWaite
Copy link
Contributor Author

MarkEWaite commented Jun 4, 2023

@dduportal I've made the transition to calculate RELEASE_LINE inside the docker-bake.hcl file thanks to the help of their regexall function. I've also removed RELEASE_LINE as an environment variable in the Dockerfile because that environment variable should not be inserted into the container at runtime.

I used JENKINS_VERSION=2.387.3 JENKINS_SHA=f40374910de94c9c1aafbd0fd190156e7f6afad6dc1534e1b55d20e125156be5 make build to build the 2.387.3 container images locally. I've tested one of the containers and watched it behave as expected, pulling the war file from get.jenkins.io

I used JENKINS_VERSION=2.406 JENKINS_SHA=9a1872f6a297961feeb34c62b8759878d1afeab09749766f6c909e59e73a6a04 make build to build the 2.406 container images locally. I've tested one of the containers and watched it behave as expected.

Ready for another review (simpler code thanks to the suggestion to use the bake file.

@MarkEWaite
Copy link
Contributor Author

@dduportal I think this is ready to merge. If we merge it before the Tuesday weekly release, we'll have some early testing.

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

Nice job! The function in the bake file is particularly cool

@dduportal dduportal merged commit 66baa18 into jenkinsci:master Jun 10, 2023
@MarkEWaite MarkEWaite deleted the download-from-mirrors branch June 10, 2023 15:08
MarkEWaite added a commit to MarkEWaite/docker that referenced this pull request Jul 26, 2023
The parallel build processes are much better served if the container
images can be created as soon as a release is published to the official
repository, repo.jenkins-ci.org, instead of waiting for the image to
be available on the mirrors.  The bandwidth used for container image
creation is small and the benefits of parallel creation of container
images are great.

Fixes jenkinsci#1671

Reverts PR jenkinsci#1648

Commits reverted include:

* a9f4d31 - Provide RELEASE_LINE in all Windows test paths
* 3dfbe26 - Add RELEASE_LINE always in make.ps1
* 4a9dbf1 - Set RELEASE_LINE in Windows make.ps1
* a7b2df5 - Limit RELEASE_LINE to use at build time
* 81e26f5 - Calculate RELEASE_LINE inside docker-bake.hcl
* 6f74cba - Export the RELEASE_LINE just like others
* 9b014ed - Identify release line based on Jenkins version
* d41891c - Use RELEASE_LINE instead of DOWNLOAD_DIR
* 7e57eb8 - Include DOWNLOAD_DIR in build scripts
MarkEWaite added a commit that referenced this pull request Jul 26, 2023
The parallel build processes are much better served if the container
images can be created as soon as a release is published to the official
repository, repo.jenkins-ci.org, instead of waiting for the image to
be available on the mirrors.  The bandwidth used for container image
creation is small and the benefits of parallel creation of container
images are great.

Fixes #1671

Reverts PR #1648

Commits reverted include:

* a9f4d31 - Provide RELEASE_LINE in all Windows test paths
* 3dfbe26 - Add RELEASE_LINE always in make.ps1
* 4a9dbf1 - Set RELEASE_LINE in Windows make.ps1
* a7b2df5 - Limit RELEASE_LINE to use at build time
* 81e26f5 - Calculate RELEASE_LINE inside docker-bake.hcl
* 6f74cba - Export the RELEASE_LINE just like others
* 9b014ed - Identify release line based on Jenkins version
* d41891c - Use RELEASE_LINE instead of DOWNLOAD_DIR
* 7e57eb8 - Include DOWNLOAD_DIR in build scripts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants