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

Revert mtu #316

Merged
merged 1 commit into from
Jan 13, 2025
Merged

Revert mtu #316

merged 1 commit into from
Jan 13, 2025

Conversation

SchSeba
Copy link
Collaborator

@SchSeba SchSeba commented Jan 1, 2025

we introduce this feature in the cni to handle cases where a pod that used the vf has net_admin and change the MTU
on the VF.

when that pod is removed the VF will go back to the host and be available
for a new pod to run but when that VF gets allocated to the pod it will
remain with an unexpected MTU.

@SchSeba
Copy link
Collaborator Author

SchSeba commented Jan 1, 2025

waiting for #314, #315

@coveralls
Copy link

coveralls commented Jan 1, 2025

Pull Request Test Coverage Report for Build 12732442456

Details

  • 13 of 20 (65.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 41.454%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/utils/netlink_manager.go 0 3 0.0%
pkg/sriov/sriov.go 13 17 76.47%
Totals Coverage Status
Change from base Build 12731882759: 0.4%
Covered Lines: 553
Relevant Lines: 1334

💛 - Coveralls

@@ -225,6 +225,18 @@ func (s *sriovManager) ReleaseVF(conf *sriovtypes.NetConf, podifName string, net
}
}

// reset MTU for VF device until if the MTU was captured in the cache
if conf.OrigVfState.MTU != 0 {
logging.Debug("Reset VF device MTU",
Copy link
Contributor

Choose a reason for hiding this comment

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

Print out the desired MTU value (conf.OrigVfState.MTU).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@adrianchiris
Copy link
Contributor

adrianchiris commented Jan 7, 2025

@SchSeba why do we need to reset VF MTU if we never set it in the CNI ?

currently we save the PF or VF mtu in conf to report it in cni results but never set it

we introduce this feature in the cni to handle
cases where a pod that used the vf has net_admin and change the MTU
on the VF.

when that po is removed the VF will go back to the host and be available
for a new pod to run but when that VF gets allocated to the pod it will
remain with an  unexpected MTU.

Signed-off-by: Sebastian Sch <[email protected]>
@SchSeba
Copy link
Collaborator Author

SchSeba commented Jan 12, 2025

Hi @adrianchiris,
we chat over slack on this last week.
I also added in the commit message the reason as requested.

let me know if that works for you

Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

usecase understood. LGTM

Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

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

LGTM

@zeeke zeeke merged commit ae8ffa4 into k8snetworkplumbingwg:master Jan 13, 2025
10 of 11 checks passed
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