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

Update sriov images to v1.3.0 #6706

Closed
wants to merge 1 commit into from

Conversation

mgfritch
Copy link
Contributor

@mgfritch mgfritch commented Sep 4, 2024

Also updates node-feature-discovery to 0.15.6
Issue: #6705

Chart releases:

Also updates node-feature-discovery to `0.15.6`

Signed-off-by: Michael Fritch <[email protected]>
@mgfritch mgfritch requested a review from a team as a code owner September 4, 2024 06:31
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.18%. Comparing base (58dc3a0) to head (74fc499).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6706   +/-   ##
=======================================
  Coverage   25.18%   25.18%           
=======================================
  Files          33       33           
  Lines        2831     2831           
=======================================
  Hits          713      713           
  Misses       2071     2071           
  Partials       47       47           
Flag Coverage Δ
inttests 9.57% <ø> (ø)
unittests 17.80% <ø> (ø)

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.

Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

Where's the corresponding chart bump? Just bumping the images in the tarball doesn't change what the chart uses.

@mgfritch
Copy link
Contributor Author

mgfritch commented Sep 5, 2024

Where's the corresponding chart bump? Just bumping the images in the tarball doesn't change what the chart uses.

The SR-IOV chart is currently maintained in the SUSE edge repo and we've added a deprecation warning for it in the rancher chart repo.

@brandond
Copy link
Member

brandond commented Sep 5, 2024

OK but how do we ensure that the images included in the airgap tarball are the same ones referenced by the chart? Do the RKE2 docs need to cover what specific version of the SR-IOV chart its airgap images are valid with? What versions of the chart are these NEW images valid with? I see that https://docs.rke2.io/networking/multus_sriov#using-multus-with-sr-iov just points to rancher charts, but it doesn't tell you what version of the chart to deploy.

@rbrtbnfgl
Copy link
Contributor

I think we can't. SR-IOV chart is installed through helm and it's not associated to the specific RKE2 version. If installed with rancher is associated to the rancher version from what I know.

@brandond
Copy link
Member

brandond commented Sep 5, 2024

I'm confused why we have SRIOV images in our multus tarball, if they are no longer bundled with multus or in any way tied to the multus chart version.

I feel like we should either remove these from the multus tarball, or call out in the release notes what version of the SRIOV chart the images are meant to be used with.

SR-IOV chart is installed through helm and it's not associated to the specific RKE2 version

If the chart version is not associated with a specific RKE2 version, then we shouldn't be bundling images for it. I think perhaps including these in the airgap tarball goes back to when we were bundling (or considering bundling) the sriov chart as a subchart of multus?

@rbrtbnfgl
Copy link
Contributor

@thomasferrandiz has more knowledge of SRIOV

@thomasferrandiz
Copy link
Contributor

@brandond I think you're right and the images were added in the multus tarball because at first sriov was supposed to be bundled and installed with multus.
Since the sriov chart had to be moved in Rancher later, we just ended up in a situation where we hope that the version of the images in the airgap tarball matches the version used by the chart in the Rancher repo.

We could add the versions to the release notes to help users understand if they have the right versions.
Is it done only through the PR description?

@brandond
Copy link
Member

brandond commented Sep 5, 2024

This needs to go deeper than just PR titles or descriptions.

I think we should do one of two things:

    • Remove the SRIOV images from the multus tarball, since multus and sriov are no longer tied together.
    • Work with the QA and release teams to figure out how to best indicate which version of the sriov chart the multus airgap tarball is meant to be used with, and add this to the component version table in the release notes.
    • Add CI checks to ensure that the images are correct for the indicated chart version - like we have for other chart images.

Lets talk about this with the team at standup or design?

@thomasferrandiz
Copy link
Contributor

As discussed, it is simpler to just remove the sriov images from the airgap tarball: #6747

@mgfritch mgfritch deleted the update-sriov-v1.3.0 branch September 7, 2024 22:56
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.

5 participants