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

Fix: Resolve issue with skipped execution of sg annotations #3700

Merged

Conversation

wfnuser
Copy link
Contributor

@wfnuser wfnuser commented Feb 5, 2024

The problem causing ineffective application of sg annotations is that, during virtual machine restart, the logical switch port is intentionally not deleted.(I guess).

When sg annotations are added and the VM is restarted, the create logical switch port logic is skipped as it detects the existing lsp. Consequently, the annotation fails to attach to the lsp. Even when we sync lsp for sg, it has no effect.

A simple fix is to update the existing lsp during lsp creation if it already exists. This approach ensures correct annotation attachment and addresses the skipped execution issue.

Pull Request

What type of this PR

Examples of user facing changes:

  • Bug fixes

Which issue(s) this PR fixes

Fixes #3581

@wfnuser wfnuser force-pushed the fix/kubevirt-security-group-annotation branch 2 times, most recently from f3d438d to d242555 Compare February 5, 2024 11:31
@wfnuser wfnuser changed the title Fix: Resolve issue with skipped execution of sg annotations Draft: Fix: Resolve issue with skipped execution of sg annotations Feb 5, 2024
@wfnuser wfnuser changed the title Draft: Fix: Resolve issue with skipped execution of sg annotations Fix: Resolve issue with skipped execution of sg annotations Feb 5, 2024
@wfnuser wfnuser marked this pull request as draft February 5, 2024 11:42
@wfnuser
Copy link
Contributor Author

wfnuser commented Feb 5, 2024

It's a draft pr.

  1. I'm not sure if we should actually delete the pod and related lsp before create news during the restart of vm.
  2. If so, we might need to update more fields if the lsp exists.

pkg/ovs/ovn-nb-logical_switch_port.go Outdated Show resolved Hide resolved
@bobz965
Copy link
Collaborator

bobz965 commented Feb 6, 2024

It's a draft pr.

  1. I'm not sure if we should actually delete the pod and related lsp before create news during the restart of vm.
  2. If so, we might need to update more fields if the lsp exists.
  1. VM restart should only delete the pod. keep the LSP is betther.

@wfnuser wfnuser force-pushed the fix/kubevirt-security-group-annotation branch from d242555 to c184810 Compare February 6, 2024 06:34
@wfnuser wfnuser marked this pull request as ready for review February 6, 2024 06:35
} else {
sgList := strings.Split(securityGroups, ",")
if _, err := c.SetLogicalSwitchPortSecurityGroup(lsp, "add", sgList...); err != nil {
klog.Errorf("set logical switch port %s security groups %s: %v", lsp.Name, securityGroups, err)
Copy link

Choose a reason for hiding this comment

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

return err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code has been updated to another style.

@wfnuser wfnuser force-pushed the fix/kubevirt-security-group-annotation branch from c184810 to dc210d5 Compare February 8, 2024 03:06
@wfnuser wfnuser requested a review from bobz965 February 8, 2024 03:15
@bobz965 bobz965 force-pushed the fix/kubevirt-security-group-annotation branch from dc210d5 to e36a9c7 Compare February 18, 2024 01:16
@oilbeater
Copy link
Collaborator

There are lint issues should be fixed

@wfnuser wfnuser force-pushed the fix/kubevirt-security-group-annotation branch from e36a9c7 to 34d363d Compare February 18, 2024 04:48
@wfnuser
Copy link
Contributor Author

wfnuser commented Feb 18, 2024

There are lint issues should be fixed

Happy Chinese new year! I just returned to my apartment in BJ. Updated.

@wfnuser wfnuser requested a review from bobz965 February 18, 2024 04:49
The problem causing ineffective application of sg annotations is
that, during virtual machine restart, the logical switch port is
intentionally not deleted.(I guess).

When sg annotations are added and the VM is restarted, the create
logical switch port logic is skipped as it detects the existing
lsp. Consequently, the annotation fails to attach to the lsp. Even
when we sync lsp for sg, it has no effect.

A simple fix is to update the existing lsp during lsp creation if
it already exists. This approach ensures correct annotation
attachment and addresses the skipped execution issue.

Signed-off-by: wfnuser <[email protected]>
@bobz965 bobz965 force-pushed the fix/kubevirt-security-group-annotation branch from 34d363d to 41bfe81 Compare February 18, 2024 05:55
@oilbeater oilbeater merged commit 3552db3 into kubeovn:master Feb 18, 2024
60 checks passed
@oilbeater
Copy link
Collaborator

@wfnuser Thanks!

bobz965 pushed a commit that referenced this pull request Feb 18, 2024
The problem causing ineffective application of sg annotations is
that, during virtual machine restart, the logical switch port is
intentionally not deleted.(I guess).

When sg annotations are added and the VM is restarted, the create
logical switch port logic is skipped as it detects the existing
lsp. Consequently, the annotation fails to attach to the lsp. Even
when we sync lsp for sg, it has no effect.

A simple fix is to update the existing lsp during lsp creation if
it already exists. This approach ensures correct annotation
attachment and addresses the skipped execution issue.

Signed-off-by: wfnuser <[email protected]>
bobz965 pushed a commit that referenced this pull request Feb 18, 2024
The problem causing ineffective application of sg annotations is
that, during virtual machine restart, the logical switch port is
intentionally not deleted.(I guess).

When sg annotations are added and the VM is restarted, the create
logical switch port logic is skipped as it detects the existing
lsp. Consequently, the annotation fails to attach to the lsp. Even
when we sync lsp for sg, it has no effect.

A simple fix is to update the existing lsp during lsp creation if
it already exists. This approach ensures correct annotation
attachment and addresses the skipped execution issue.

Signed-off-by: wfnuser <[email protected]>
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.

默认VPC、默认子网下的kubevirt虚机添加安全组无效
4 participants