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

v1.13 Backports 2024-02-29 #31049

Merged
merged 1 commit into from Mar 1, 2024
Merged

Conversation

YutaroHayakawa
Copy link
Member

@YutaroHayakawa YutaroHayakawa commented Feb 29, 2024

Once this PR is merged, a GitHub action will update the labels of these PRs:

 30742

@YutaroHayakawa YutaroHayakawa added kind/backports This PR provides functionality previously merged into master. backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. labels Feb 29, 2024
@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Feb 29, 2024

/test-backport-1.13

Job 'Cilium-PR-K8s-1.18-kernel-4.19' failed:

Click to show.

Test Name

K8sAgentPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: cannot install connectivity-check

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.18-kernel-4.19/388/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.18-kernel-4.19 so I can create one.

Then please upload the Jenkins artifacts to that issue.

@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review February 29, 2024 05:45
@YutaroHayakawa YutaroHayakawa requested review from a team as code owners February 29, 2024 05:45
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks!

[ upstream commit 6fee46f ]

[ backporter's note:
  - test-e2e-upgrade doesn't exit on this branch. Remove it.
  - Minor conflict in tests-clustermesh-upgrade.yaml

++<<<<<<< HEAD
 +          if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then
 +            SHA="${{ inputs.SHA }}"
 +          else
 +            SHA="${{ github.sha }}"
 +          fi
++=======
+           CILIUM_DOWNGRADE_VERSION=$(contrib/scripts/print-downgrade-version.sh stable)
+           echo "downgrade_version=${CILIUM_DOWNGRADE_VERSION}" >> $GITHUB_OUTPUT
++>>>>>>> 20a5826c31 (ci/ipsec: Fix downgrade version retrieval)
]

Figuring out the right "previous patch release version number" to
downgrade to in print-downgrade-version.sh turns out to be more complex
than expected [0][1][2][3].

This commit is an attempt to 1) fix issues with the current script and
2) overall make the script clearer, so we can avoid repeating these
mistakes.

As for the fixes, there are two things that are not correct with the
current version. First, we're trying to validate the existence of the
tag to downgrade to, in case the script runs on top of a release
preparation commit for which file VERSION has been updated to a value
that does not yet contains a corresponding tag. This part of the script
is actually OK, but not the way we call it in the IPsec workflow: we use
"fetch-tags: true" but "fetch-depth: 0" (the default), and the two are
not compatible, a shallow clone results in no tags being fetched.

To address this, we retrieve the tag differently: instead of relying on
"fetch-tags" from the workflow, we call "git fetch" from the script
itself, provided the preconditions are met (we only run it from a Git
repository, if the "origin" remote is defined). If the tag exists,
either locally or remotely, then we can use it. Otherwise, the script
considers that it runs from a release preparation Pull Request, and
decrements the patch release number.

The second issue is that we would return no value from the script if the
patch release is zero. This is to avoid any attempt to find a previous
patch release when working on a development branch. However, this logics
is incorrect (it comes from a previous version of the script where we
would always decrement the patch number). After the first release of a
new minor version, it's fine to have a patch number at 0. What we should
check instead is whether the version ends with "-dev".

This commit brings additional changes for clarity: more comments, and a
better separation between the "get latest patch release" and "get
previous stable branch" cases, moving the relevant code to independent
functions, plus better argument handling. We also edit the IPsec
workflow to add some logs about the version retrieved. The logs should
also display the script's error messages, if any, that are printed to
stderr.

Sample output from the script:

    VERSION     Tag exists  Prevous minor   Previous patch release

    1.14.3      Y           v1.13           v1.14.3
    1.14.1      Y           v1.13           v1.14.1
    1.14.0      Y           v1.13           v1.14.0
    1.14.1-dev  N           v1.13           <error>
    1.15.0-dev  N           v1.14           <error>
    1.13.90     N           v1.12           v1.13.89  <- decremented
    2.0.0       N           <error>         <error>
    2.0.1       N           <error>         v2.0.0    <- decremented
    2.1.1       N           v2.0            v2.1.0    <- decremented

[0] 56dfec2 ("contrib/scripts: Support patch releases in print-downgrade-version.sh")
[1] 4d7902f ("contrib/scripts: Remove special handling for patch release number 90")
[2] 5581963 ("ci/ipsec: Fix version retrieval for downgrades to closest patch release")
[3] 3803f53 ("ci/ipsec: Fix downgrade version for release preparation commits")

Fixes: 3803f53 ("ci/ipsec: Fix downgrade version for release preparation commits")
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa force-pushed the pr/v1.13-backport-2024-02-29-01-17 branch from 8c2eb49 to 22e12a7 Compare February 29, 2024 13:28
@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Feb 29, 2024

/test-backport-1.13

Job 'Cilium-PR-K8s-1.21-kernel-4.19' hit: #30802 (91.34% similarity)

@YutaroHayakawa
Copy link
Member Author

Cilium IPsec upgrade: 1.1.1.1 timeout
Conformance IPsec E2E: 1.1.1.1 timeout
k8s-1.21-kernel-4.19: #30802

@YutaroHayakawa
Copy link
Member Author

/test-1.21-4.19

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 1, 2024
@YutaroHayakawa YutaroHayakawa merged commit 1a6200e into v1.13 Mar 1, 2024
147 checks passed
@YutaroHayakawa YutaroHayakawa deleted the pr/v1.13-backport-2024-02-29-01-17 branch March 1, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants