Skip to content

Conversation

tmshort
Copy link
Contributor

@tmshort tmshort commented Jun 25, 2025

Rather than assuming we should use a version of kindest/node based on our version of k8s in go.mod (with a .0 patch), we check to ensure that the version kind uses is compatible with our k8s major.minor version.

We discovered this problem because it appears that kindest/node:v1.33.0 has issues with some systems (e.g. Fedora).

Using kindest/node:v1.33.1 fixes this issue. So, we don't want to fix a .0 patch version. We want to ensure that the kindest/node image is compatible.

Also note that kind never used kindest/node:v1.33.0 as a default image, kind v0.28.0/v0.29.0 use kindest/node:v1.33.1.

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@tmshort tmshort requested a review from a team as a code owner June 25, 2025 18:09
Copy link

netlify bot commented Jun 25, 2025

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit b411224
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/685c5e3e2c7e9300080d9e51
😎 Deploy Preview https://deploy-preview-2047--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

codecov bot commented Jun 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.82%. Comparing base (00b965c) to head (b411224).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2047   +/-   ##
=======================================
  Coverage   73.82%   73.82%           
=======================================
  Files          81       81           
  Lines        7365     7365           
=======================================
  Hits         5437     5437           
  Misses       1588     1588           
  Partials      340      340           
Flag Coverage Δ
e2e 44.06% <ø> (ø)
unit 60.25% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tmshort
Copy link
Contributor Author

tmshort commented Jun 25, 2025

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2025
Makefile Outdated
@@ -44,7 +44,12 @@ ENVTEST_VERSION := $(K8S_VERSION).x
# The K8S_VERSION is set by getting the version of the k8s.io/client-go dependency from the go.mod
# and sets major version to "1" and the patch version to "0". For example, a client-go version of v0.28.5
# will map to a K8S_VERSION of 1.28.0
ifeq ($(K8S_VERSION),1.33)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we pin to the 0-th patch version of the aligned kube release? That feels like a false dependency, where the real solution is to ensure that our kube/kind dependencies were always aligned (for e.g. as go-verdiff evals that we sync golang bumps, we should also bump kind when we bump kube).

Please
/hold
while we discuss

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarification on KIND and node image versions:

Just to clarify, kind is the tool, while the node image refers to the Kubernetes version/image used—these are distinct.

✅ The node image is automatically updated based on the go.mod, which is why we’re already using Kubernetes v1.33.
So what you're asking about is actually already handled.

📌 We have a separate PR to update kind along with the other Bingo-managed tools: #2037
We usually avoid combining all updates in one PR to keep reviews focused and manageable.

So, for me it is great if that solves the specific issue faced with Fedora.

/lgtm
/approved

Copy link
Contributor

Choose a reason for hiding this comment

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

Disagree.
The node image is getting pinned here to the $kube-version.0 (0 patch version) for the underlying kube version.
This effectively prevents us from using any updated node images released by kind, unless they re-release them under the same patch version, which isn't sane.
I think @joelanford participated in the initial discussion on this issue, and I think the argument had been that we would continue support for kube bump minor releases as long as we stayed on that kube version... but I admit that it's been a minute and my recollection might not be exact.
But in this case we are overriding the defaults of a version of the tool that we deliberately chose, which feels a bit like needlessly fighting ourselves.
I am not convinced that the expediency of this solution necessitates haste here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will just add that changing the kind node version from 1.33.0 to 1.33.1 makes Makefile targets work for me again on f42.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2025
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2025
@bentito
Copy link
Contributor

bentito commented Jun 25, 2025

/lgtm

Rather than assuming we should use a version of kindest/node based
on our version of k8s in go.mod (with a .0 patch), we check to esnure
that the version kind uses is compatible with our k8s major.minor
version.

We discovered this problem because it appears that kindest/node:v1.33.0
has issues with some systems (e.g. Fedora).

Using kindest/node:v1.33.1 fixes this issue. So, we don't want to fix
a .0 patch version. We want to ensure that the kindest/node image is
compatible.

Also note that kind never used kindest/node:v1.33.0 as a default image,
kind v0.28.0/v0.29.0 use kindest/node:v1.33.1.

Signed-off-by: Todd Short <[email protected]>
@tmshort tmshort force-pushed the update-kindest-image branch from 4458a39 to b411224 Compare June 25, 2025 20:38
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2025
@tmshort tmshort changed the title 🐛 Update kindest/node image to v1.33.1 🐛 Update kindest/node image to v1.33.1 via kind v0.29.0 Jun 25, 2025
@tmshort tmshort mentioned this pull request Jun 25, 2025
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

That is a better solution yet.
Great 🎉

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2025
Copy link

openshift-ci bot commented Jun 26, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, tmshort, trgeiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [camilamacedo86,tmshort]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tmshort
Copy link
Contributor Author

tmshort commented Jun 26, 2025

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit b02314b into operator-framework:main Jun 26, 2025
23 checks passed
@@ -320,8 +314,9 @@ kind-deploy: manifests

.PHONY: kind-cluster
kind-cluster: $(KIND) #EXHELP Standup a kind cluster.
env K8S_VERSION=v$(K8S_VERSION) KIND=$(KIND) GOBIN=$(GOBIN) hack/tools/validate_kindest_node.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't get to this yesterday, but would've preferred to keep this check separate from the cluster creation step, so we could include it in the verify target.

Copy link
Contributor

Choose a reason for hiding this comment

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

Followed up with #2052

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants