Skip to content

CI: Pin OpenBSD 7.6 #2059

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

Merged
merged 3 commits into from
May 11, 2025
Merged

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented May 6, 2025

OpenBSD 7.7 introduces some changes that fail our build.
Pin version 7.6 while we work on version 7.7 adoption.

@kinkie
Copy link
Contributor Author

kinkie commented May 6, 2025

This should fix the problems of the broken job blocking the merge of PR #2058

@kinkie
Copy link
Contributor Author

kinkie commented May 6, 2025

FreeBSD doesn't suffer the same problem, as we're pinning two releases there

@kinkie kinkie requested a review from Copilot May 6, 2025 20:41
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR pins the OpenBSD version used in the CI for "slow" checks and updates the automake version distributed with it.

  • Pin OpenBSD release to 7.7
  • Update automake from 1.16.5 to 1.17
Comments suppressed due to low confidence (1)

.github/workflows/slow.yaml:205

  • Ensure that the update to automake-1.17 is validated against all build steps, and update any related configuration or documentation if required to address potential behavioral differences.
automake-1.17

@rousskov rousskov changed the title CI: adopt openbsd v7.7 and pin it CI: Adopt OpenBSD v7.7 and pin it May 6, 2025
rousskov
rousskov previously approved these changes May 6, 2025
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thank you! I adjusted PR title/description a bit to improve consistency/grammar.

@kinkie kinkie added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels May 7, 2025
yadij
yadij previously approved these changes May 7, 2025
@yadij yadij removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label May 7, 2025
@yadij
Copy link
Contributor

yadij commented May 7, 2025

Do the other branches need this backported as well now?

squid-anubis pushed a commit that referenced this pull request May 7, 2025
Pin the version of OpenBSD we use for our "slow" checks
and update the version of automake distributed with it.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-failed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels May 7, 2025
@rousskov
Copy link
Contributor

rousskov commented May 7, 2025

@kinkie, OpenBSD test still fails. I could not tell why without studying openbsd-vm/v1/run.sh.

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels May 7, 2025
@kinkie
Copy link
Contributor Author

kinkie commented May 7, 2025

I'll check and iterate later today.
All branches will need this

@yadij
Copy link
Contributor

yadij commented May 7, 2025

  ./test-builds.sh
  
  exec ssh: cd $GITHUB_WORKSPACE;
  export MAKE=gmake
  export pjobs="-j`gnproc`"
  export AUTOMAKE_VERSION=1.16
  export amver=${AUTOMAKE_VERSION}

... this (hard coded in slow.yaml) AUTOMAKE_VERSION=1.16 causes:

  ./test-builds.sh
  /usr/bin/bash /home/runner/work/_actions/vmactions/openbsd-vm/v1/run.sh execSSHSH
  Config file: openbsd-7.7.conf
  WARNING: Cannot find automake version 1.16
  Trying /usr/local/bin/automake[34]: /usr/local/bin/automake-1.16: not found
  WARNING: Cannot find libtool version 2.4.2
  Trying libtool (not (GNU libtool)) 1.5.26
  automake () : automake
  autoconf (2.72) : autoconf
  libtool  (1.5.26) : libtool
  libtool path : /usr/bin
  Bootstrapping primary Squid sources
  /usr/local/bin/aclocal[34]: /usr/local/bin/aclocal-1.16: not found
  Autotool bootstrapping failed. You will need to investigate and correct
  before you can develop on this source tree
  aclocal failed

@yadij
Copy link
Contributor

yadij commented May 7, 2025

Do we really have to explicitly set the autotools variables on top of installing the versioned package? Surely the right package being installed is enough.
If not, perhapse our bootstrap.sh can be improved to better detect the tool binaries.

@yadij yadij added backport-to-v6 backport-to-v7 maintainer has approved these changes for v7 backporting labels May 7, 2025
@kinkie
Copy link
Contributor Author

kinkie commented May 7, 2025

Doh! Sadly yes. Openbad is brain damaged in this regard. If we don't set them it will just complain and bail

@kinkie kinkie changed the title CI: Adopt OpenBSD v7.7 and pin it CI: Pin OpenBSD 7.6 May 9, 2025
@kinkie
Copy link
Contributor Author

kinkie commented May 9, 2025

I've changed tack, and pinned OpenBSD 7.6 instead.
Staging build succeeds for it now, see https://github.com/kinkie/squid/actions/runs/14923646799/job/41923591531

@kinkie kinkie requested review from Copilot and yadij May 9, 2025 13:03
@kinkie kinkie added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 9, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR pins the OpenBSD version used in the CI workflow to 7.6 to resolve build failures introduced by OpenBSD 7.7.

  • The workflow in .github/workflows/slow.yaml now explicitly sets the release parameter to "7.6" to ensure build stability.

@kinkie kinkie removed the S-waiting-for-author author action is expected (and usually required) label May 9, 2025
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

@kinkie, good call with pivoting this PR to preserve old/tested environment!

@rousskov
Copy link
Contributor

rousskov commented May 9, 2025

2025/05/09 13:18:06 kid1| ERROR: ipcacheAddEntryFromHosts: Bad IP address '-e'

This PR now fails CI functionality tests because, AFAICT, Squid cannot parse /etc/hosts file provided by GitHub. These failures are not related to this PR changes. Other PRs are failing in a similar fashion. I assume that GitHub has changed something... We will address this problem.

Meanwhile, I am OK with allowing this specific PR to land without passing functionality tests (because those tests cannot expose problems with this PR changes and landing this PR will help land all other ready PRs).

@rousskov
Copy link
Contributor

rousskov commented May 9, 2025

2025/05/09 13:18:06 kid1| ERROR: ipcacheAddEntryFromHosts: Bad IP address '-e'

This PR now fails CI functionality tests because, AFAICT, Squid cannot parse /etc/hosts file provided by GitHub. These failures are not related to this PR changes. Other PRs are failing in a similar fashion. I assume that GitHub has changed something... We will address this problem.

Here are more details: AFAICT, GitHub recently changed /etc/hosts on their ubuntu-22.04 GitHub Actions runner. Twice. Version 20250427.1.0 of the runner (let's call it vApril27) has /etc/hosts variation (with -e characters somewhere) that Squid cannot parse. Version 20250504.1.0 of the runner (let's call it vMay4) has /etc/hosts variation that Squid can parse. I do not know the algorithm GitHub uses to assign a runner to GitHub Actions runs, but I see that Factory repository gets vMay4 runner, while this official repository gets vApril27 runner, even for new PRs (I have rerun a few failed tests and even created one official PR to test that latter assertion).

Going forward, I see several options:

  1. Do nothing but wait for vApril27 to disappear. It will happen eventually. Retried official repository tests will then pass. ETA: Unknown, but probably a few days.
  2. (Ab)use official repository to figure out what /etc/hosts variation Squid does not like. If it is a Squid bug or limitation, improve Squid.
  3. Temporary adjust functionality tests in official Squid repository to install our own version of /etc/hosts. We can use the one from vMay4. This option requires two official PRs and commits: One to add and one to remove that temporary code.
  4. Temporary ignore functionality-tests failures attributed to /etc/hosts parsing as detailed in my earlier comment and force-merge this PR (and possibly other otherwise ready PRs that are unlikely to fail functionality-tests for other reasons).

I am OK with any of the above options, but recommend option 4 followed by option 1. What is your preference?

@kinkie
Copy link
Contributor Author

kinkie commented May 9, 2025

  1. Temporary ignore functionality-tests failures attributed to /etc/hosts parsing as detailed in my earlier comment and force-merge this PR (and possibly other otherwise ready PRs that are unlikely to fail functionality-tests for other reasons).

I am OK with any of the above options, but recommend option 4 followed by option 1. What is your preference?

I agree with going with option 4. and reassessing the situation in 1-2 weeks.

@rousskov
Copy link
Contributor

rousskov commented May 9, 2025

  1. Temporary ignore functionality-tests failures attributed to /etc/hosts parsing as detailed in my earlier comment and force-merge this PR (and possibly other otherwise ready PRs that are unlikely to fail functionality-tests for other reasons).

I agree with going with option 4.

Great! If @yadij approves, then we can merge this PR "as is".

If you do merging, please make sure to squash and provide the right commit message. No PR branch modifications are necessary or desired. Please let me know if you want me to merge instead.

@kinkie
Copy link
Contributor Author

kinkie commented May 9, 2025

Feel free to merge whenever convenient for you

@kinkie
Copy link
Contributor Author

kinkie commented May 11, 2025

Proceeding with the merge, it's been pending for a while now

@kinkie kinkie merged commit 1723d15 into squid-cache:master May 11, 2025
33 of 38 checks passed
@rousskov rousskov removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 11, 2025
@rousskov
Copy link
Contributor

Alex: If @yadij approves, then we can merge this PR "as is".

Francesco: Proceeding with the merge, it's been pending for a while now

Please do not merge PRs that are awaiting review, even if you think that review request should be ignored.

@kinkie
Copy link
Contributor Author

kinkie commented May 11, 2025

Please do not merge PRs that are awaiting review, even if you think that review request should be ignored.

Whoops, sorry, I was convinced @yadij had approved. Apologies.

@rousskov rousskov removed the M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label May 12, 2025
@rousskov
Copy link
Contributor

2025/05/09 13:18:06 kid1| ERROR: ipcacheAddEntryFromHosts: Bad IP address '-e'
  1. ... figure out what /etc/hosts variation Squid does not like. If it is a Squid bug or limitation, improve Squid.

While working on another problem, I had an opportunity to dump the problematic /etc/hosts file from a GitHub Actions runner. Here it is:

127.0.0.1 localhost

# The following lines are desirable for IPv6 capable hosts
::1     localhost ip6-localhost ip6-loopback
fe00::0 ip6-localnet
ff00::0 ip6-mcastprefix
ff02::1 ip6-allnodes
ff02::2 ip6-allrouters
ff02::3 ip6-allhosts
-e 10.1.1.84 pkrvmberfyhpb9w.nvcrsk4lb03ebagxjwngdrxj4g.ex.internal.cloudapp.net pkrvmberfyhpb9w

This does not look like a Squid bug or limitation to me. I speculate that somebody or something incorrectly copy-pasted a command like sed -e into a file that generates /etc/hosts. The same problem is present on April-dated Ubuntu 24.04 runners (from where the above snapshot was taken). Hopefully those runners will be gone soon!

@kinkie
Copy link
Contributor Author

kinkie commented May 13, 2025

This does not look like a Squid bug or limitation to me

I agree

@rousskov
Copy link
Contributor

  1. Temporary adjust functionality tests in official Squid repository to install our own version of /etc/hosts.

FWIW, #2064 contains that adjustment as detailed at #2064 (comment)

@yadij
Copy link
Contributor

yadij commented May 14, 2025

Please do not merge PRs that are awaiting review, even if you think that review request should be ignored.

Whoops, sorry, I was convinced @yadij had approved. Apologies.

FWIW, I did approve the previous PR contents that look essentially the same and do still approve.

@yadij yadij added backport-to-v7 maintainer has approved these changes for v7 backporting and removed backport-to-v7 maintainer has approved these changes for v7 backporting backport-to-v6 labels May 29, 2025
@yadij
Copy link
Contributor

yadij commented May 29, 2025

queued for backport to v7.
removed backport to v6.

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.

4 participants