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.

Sign up for GitHub

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

Jump to bottom

Skip linux-bridge filtering if no "bridge" section #690

Merged
merged 1 commit into from
Feb 1, 2021
Merged

Skip linux-bridge filtering if no "bridge" section #690

merged 1 commit into from
Feb 1, 2021

Conversation

qinqon
Copy link
Member

@qinqon qinqon commented Feb 1, 2021

Is this a BUG FIX or a FEATURE ?:
We have found that at some envs the "bridge" section from linux-bridge
is not present in case the bridge is down, this change ignore the
filtering in this is the case to preveng a segmentation fault.

Uncomment only one, leave it on its own line:

/kind bug

/kind enhancement

What this PR does / why we need it:

Special notes for your reviewer:

Release note:

NONE

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Feb 1, 2021
@qinqon
Copy link
Member Author

qinqon commented Feb 1, 2021

/cherry-pick release-0.37

@qinqon qinqon closed this Feb 1, 2021
@kubevirt-bot
Copy link
Collaborator

@qinqon: cannot cherry-pick an unmerged PR

In response to this:

/cherry-pick release-0.37

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@qinqon qinqon reopened this Feb 1, 2021
@qinqon
Copy link
Member Author

qinqon commented Feb 1, 2021

/cherry-pick release-0.37

@kubevirt-bot
Copy link
Collaborator

@qinqon: once the present PR merges, I will cherry-pick it on top of release-0.37 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.37

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@qinqon qinqon requested review from phoracek and rhrazdil February 1, 2021 14:11
phoracek
phoracek reviewed Feb 1, 2021
View reviewed changes
pkg/nmstatectl/remote.go Outdated
@@ -0,0 +1,68 @@
package nmstatectl
Copy link
Member

Choose a reason for hiding this comment

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

This should not be a part of this PR, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed, this is from the PoC.

@qinqon qinqon force-pushed the check-bridge-key-on-filter branch from e3265bf to cedbf10 Compare February 1, 2021 14:13
@kubevirt-bot kubevirt-bot added size/M and removed size/L labels Feb 1, 2021
@qinqon qinqon requested a review from phoracek February 1, 2021 14:14
phoracek
phoracek requested changes Feb 1, 2021
View reviewed changes
pkg/state/filter.go Outdated
@@ -58,9 +58,24 @@ func filterOutDynamicAttributes(iface map[string]interface{}) {
return
}

bridge := iface["bridge"].(map[string]interface{})
bridgeIface, ok := iface["bridge"]
Copy link
Member

Choose a reason for hiding this comment

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

use hasBridge instead of ok. Then the following condition makes more sense.

pkg/state/filter.go Outdated Show resolved Hide resolved
pkg/state/filter_test.go Outdated Show resolved Hide resolved
rhrazdil
rhrazdil reviewed Feb 1, 2021
View reviewed changes
pkg/state/filter_test.go Outdated Show resolved Hide resolved
@qinqon qinqon force-pushed the check-bridge-key-on-filter branch 2 times, most recently from 1f44fd3 to 2139727 Compare February 1, 2021 14:23
@qinqon qinqon requested review from rhrazdil and phoracek February 1, 2021 14:23
@qinqon qinqon force-pushed the check-bridge-key-on-filter branch from 2139727 to c85db66 Compare February 1, 2021 14:25
phoracek
phoracek reviewed Feb 1, 2021
View reviewed changes
pkg/state/filter_test.go Outdated Show resolved Hide resolved
phoracek
phoracek requested changes Feb 1, 2021
View reviewed changes
pkg/state/filter.go Outdated Show resolved Hide resolved
@qinqon qinqon force-pushed the check-bridge-key-on-filter branch from c85db66 to 2d3b0bd Compare February 1, 2021 14:34
@qinqon
Skip linux-bridge filtering if no "bridge" section
ef79eff 
We have found that at some envs the "bridge" section from linux-bridge
is not present in case the bridge is down, this change ignore the
filtering in this is the case to preveng a segmentation fault.

Signed-off-by: Quique Llorente <[email protected]>
@qinqon qinqon force-pushed the check-bridge-key-on-filter branch from 2d3b0bd to ef79eff Compare February 1, 2021 14:36
@qinqon qinqon requested a review from phoracek February 1, 2021 14:36
phoracek
phoracek reviewed Feb 1, 2021
View reviewed changes
Copy link
Member

@phoracek phoracek left a comment

Choose a reason for hiding this comment

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

/approve

Looks good. Would you lgtm @rhrazdil if all seems fine to you?

@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phoracek

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2021
rhrazdil
rhrazdil reviewed Feb 1, 2021
View reviewed changes
Copy link
Collaborator

@rhrazdil rhrazdil left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2021
@enp0s3
Copy link

enp0s3 commented Feb 1, 2021

@phoracek @qinqon sorry but it's not clear to me what do you mean by "bridge is down" - was that administratively down or was the link of the bridge down? Perhaps I am not familiar with the states

@qinqon
Copy link
Member Author

qinqon commented Feb 1, 2021

/retest

@kubevirt-bot kubevirt-bot merged commit 6503a35 into nmstate:master Feb 1, 2021
@kubevirt-bot kubevirt-bot mentioned this pull request Feb 1, 2021
Merged
@kubevirt-bot
Copy link
Collaborator

@qinqon: new pull request created: #691

In response to this:

/cherry-pick release-0.37

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhrazdil rhrazdil rhrazdil left review comments

@phoracek phoracek phoracek requested changes

@bcrochet bcrochet Awaiting requested review from bcrochet

@RamLavi RamLavi Awaiting requested review from RamLavi

Assignees

@phoracek phoracek

@rhrazdil rhrazdil

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@qinqon @kubevirt-bot @enp0s3 @phoracek @rhrazdil