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

pod should use mac and ips provider by multus firstly #4800

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

changluyi
Copy link
Collaborator

Pull Request

What type of this PR

Examples of user facing changes:

  • Features
  • Bug fixes
  • Docs
  • Tests

Which issue(s) this PR fixes

Fixes #(issue-number)

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 6, 2024
@changluyi changluyi added the bug Something isn't working label Dec 6, 2024
@coveralls
Copy link

coveralls commented Dec 6, 2024

Pull Request Test Coverage Report for Build 12275027635

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 21 (0.0%) changed or added relevant lines in 1 file are covered.
  • 32 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+3.2%) to 21.899%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controller/pod.go 0 21 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/webhook/static_ip.go 3 0.0%
pkg/ovs/ovn-nb-logical_router_route.go 29 75.64%
Totals Coverage Status
Change from base Build 12195383681: 3.2%
Covered Lines: 10185
Relevant Lines: 46508

💛 - Coveralls

@zhangzujian
Copy link
Member

加个 e2e case?

if err := c.config.KubeOvnClient.KubeovnV1().IPs().Delete(context.Background(), portNeedDel, metav1.DeleteOptions{}); err != nil {
if !k8serrors.IsNotFound(err) {
klog.Errorf("failed to delete ip %s, %v", portNeedDel, err)
if len(portsNeedToDel) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

这个判断应该不需要吧?

Comment on lines 1230 to 1232
if len(cachedPod.Annotations) == len(pod.Annotations) {
return pod, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

这里需要检查下 annotations 的 kv 吗?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

应该不需要

Comment on lines 1575 to 1577
if attach.MacRequest != "" {
ret.MacRequest = attach.MacRequest
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if attach.MacRequest != "" {
ret.MacRequest = attach.MacRequest
}
ret.MacRequest = attach.MacRequest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1

annotationsNeedToAdd[fmt.Sprintf(util.IPAddressAnnotationTemplate, podNet.ProviderName)] = podNet.IPRequest
}

if podNet.MacRequest != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

如果用的不是 kubeovn 的 ip 和 mac,是不是补充个 log ? 不然查问题的话,不确定 mac 和 ip 哪里来的

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

已加

@changluyi
Copy link
Collaborator Author

加个 e2e case?

1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants