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

CI builds are breaking after we stopped immediately building rc.0 #2972

Closed
puerco opened this issue Mar 18, 2023 · 23 comments · Fixed by #2976
Closed

CI builds are breaking after we stopped immediately building rc.0 #2972

puerco opened this issue Mar 18, 2023 · 23 comments · Fixed by #2976
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority sig/release Categorizes an issue or PR as relevant to SIG Release.

Comments

@puerco
Copy link
Member

puerco commented Mar 18, 2023

Immediately after cutting the March patch releases, the ci builds started failing:

image

The error as shown in the build log shows its related to the version string:

level=info msg="Using build version: v1.25.8-1+3a14fe1af239a0"
level=fatal msg="failed to run: find latest version: build version v1.25.8-1+3a14fe1af239a0 is not valid for release"

Upon further investigation, the builds are failing because the regular expressions that check the version string don't catch the gitCommt string without the rc.0:

👉 This is matched: v1.25.8-rc.0.31+36b707f892c523
👉 This is not matched: v1.25.8-1+3a14fe1af239a0

We only started seeing this as these are the first releases we cut after we stopped building the rc.0 immediately after tagging the repo.

More details on this slack thread in #release-ci-signal.

Thanks to @BenTheElder for reporting.

/cc @saschagrunert @kubernetes/release-engineering

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority labels Mar 18, 2023
@BenTheElder
Copy link
Member

We could change the regex, but I think it's not expected that version string format would change.
We have other code that parses and reacts to version strings, so for now without digging too deep into that I recommend that we add an rc.0 tag (but just the tag, no release).

@BenTheElder
Copy link
Member

/kind bug
/sig release

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/release Categorizes an issue or PR as relevant to SIG Release. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Mar 18, 2023
@saschagrunert
Copy link
Member

saschagrunert commented Mar 20, 2023

We could change the regex, but I think it's not expected that version string format would change.

It's not the format which changes, it's just a case which we have not considered yet. I think we should fix the regex, because it looks like that it matches now incorrectly group 4 of the versionReleaseRE:

versionReleaseRE = `v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(-[a-zA-Z0-9]+)*\.*(0|[1-9][0-9]*)?`

screenshot

The last matching group (group 4) is wrongly matched in the regex, because it should be part of the versionBuildRE:

versionBuildRE = `([0-9]{1,})\+([0-9a-f]{5,40})`

screenshot

The assembling of the regular expressions make them fail, because in case of v1.25.8-1+3a14fe1af239a0 it's now invalid:

return regexp.MatchString("("+versionReleaseRE+`(\.`+versionBuildRE+")"+versionDirtyRE+"?)", build)

@xmudrii
Copy link
Member

xmudrii commented Mar 20, 2023

I'm +1 for changing the regex, it seems wrong.

I see two options here:

  • We reintroduce the rc.0 tag, but without building artifacts
    • This is IMO not ideal because we almost always have artifacts for pushed tags. Not having artifacts might break someone or some use cases and then we have an issue again
    • I expect that in the future we'll have questions why we do it this way instead of completely dropping the tag and fixing the issue
  • We fix the regex to match the new case
    • This feels like a proper and more sustainable fix to me, but I'm not sure if this is going to break some consumers/users. In general, I think it should be safe, but we can never know for sure

@saschagrunert
Copy link
Member

I agree having a tag without artifacts seems not correct.

@liggitt
Copy link
Member

liggitt commented Mar 20, 2023

is this also blocking presubmit e2e runs on release branches? seeing

W0320 12:43:01.804] 2023/03/20 12:43:01 main.go:328: Something went wrong: failed to acquire k8s binaries: U=file:///go/src/k8s.io/kubernetes/_output/gcs-stage R=v1.26.3-3+df96302de9bd79 get-kube.sh failed: error during /workspace/get-kube.sh: exit status 1

in presubmits as well

@BenTheElder
Copy link
Member

I'm not awake enough to vet version formats but please be careful with that, we have other packages (including one exported for external use) that know how to parse and compare / order Kubernetes versions and some tools take action based on this

@saschagrunert
Copy link
Member

@xmudrii are you planning to work on this or should I take a stab on the regex?

@xmudrii
Copy link
Member

xmudrii commented Mar 21, 2023

@saschagrunert Can you please take care of it? I need to take care of jobs generation ahead of today's rc.0.

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

In addition to the regex in the release repo, there's regexes in CI setup scripts that change where to pull artifacts from based on whether it is looking at a CI build or a release build. Currently, blocking e2e presubmits on all release branches are failing because it doesn't recognize CI builds as CI builds:

W0320 20:26:16.679] Version doesn't match regexp

W0320 20:26:25.700] 2023/03/20 20:26:21 extract_k8s.go:304: U=file:///go/src/k8s.io/kubernetes/_output/gcs-stage R=v1.25.8-4+8f59afd976b576 get-kube.sh failed: error during /workspace/get-kube.sh: exit status 1

Something went wrong: failed to acquire k8s binaries: U=file:///go/src/k8s.io/kubernetes/_output/gcs-stage R=v1.25.8-4+8f59afd976b576 get-kube.sh failed: error during /workspace/get-kube.sh

That means a fix is needed in https://github.com/kubernetes/test-infra/blob/master/kubetest/extract_k8s.go#L244 as well.

Edit: actually, I think the fix is needed in https://github.com/kubernetes/kubernetes/blob/c9ff2866682432075da1a961bc5c3f681b34c8ea/cluster/get-kube.sh/#L75 on all release branches :-/

Edit: or maybe at https://github.com/kubernetes/kubernetes/blob/c9ff2866682432075da1a961bc5c3f681b34c8ea/cluster/common.sh#L45-L46

Edit: or maybe at https://github.com/kubernetes/kubernetes/blob/c9ff2866682432075da1a961bc5c3f681b34c8ea/build/lib/release.sh#L52

saschagrunert added a commit to saschagrunert/kubernetes that referenced this issue Mar 21, 2023
This changes the version regex to allow matching non alpha, beta or rc
tags together with the version build regex.

Part of kubernetes/release#2972

Signed-off-by: Sascha Grunert <[email protected]>
@saschagrunert
Copy link
Member

@liggitt proposed the regex changes in kubernetes/kubernetes#116807

Another grep through k/k gave me no further results.

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

ha, I just opened kubernetes/kubernetes#116808 against release-1.26 to test

@saschagrunert
Copy link
Member

ha, I just opened kubernetes/kubernetes#116808 against release-1.26 to test

Alright, free to close mine depending on your preference.

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

kubernetes/kubernetes#116809 (comment) is ready for review and picked cleanly to all release branches, CI is running now. I made very incremental changes commit by commit so it is easier to review

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

we either need to reopen this or create a new issue, because blocking presubmits are still failing on all release branches

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

KUBE_CI_VERSION_REGEX in kube-up needs updating, kubernetes/kubernetes#116809 is open to do that, but the script used in presubmits is apparently not the one in the PR... see kubernetes/kubernetes#116808 (comment)

@saschagrunert saschagrunert reopened this Mar 21, 2023
@saschagrunert
Copy link
Member

Reopening until we fixed the issues.

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

kubernetes/test-infra#29102 open to trigger kubekins build to pick up kubernetes/kubernetes#116809

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

kubekins images rebuilt, kubernetes/test-infra#28975 open to update CI jobs to use them

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

release branch presubmit e2e jobs passing again 😌

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

for completeness, should probably merge the picks of the kube-up regex change (https://github.com/kubernetes/kubernetes/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+116809+-base%3Amaster), but AFAICT the presubmit breakages are resolved at this point

@saschagrunert
Copy link
Member

Alright, closing this one now.

@cici37
Copy link
Contributor

cici37 commented Apr 6, 2023

A issue was reported which is related: kubernetes/kubernetes#117115

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority sig/release Categorizes an issue or PR as relevant to SIG Release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants