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/6088 #6095

Open
wants to merge 6 commits into
base: devel
Choose a base branch
from
Open

fix/6088 #6095

wants to merge 6 commits into from

Conversation

jrouzierinverse
Copy link
Member

Description

Avoid looking up a mac lookup when bouncing a port when the mac is known.

Impacts

Bouncing ports

NEWS file entries

Bug Fixes

  • Bounce the correct node when bounce a port.

Issue

fixes #6088

Delete branch after merge

NO

Checklist

  • Document the feature
  • Add unit tests
  • Add acceptance tests (TestLink)

@lyubomirtraykov
Copy link
Contributor

What about:
lib/pf/UnifiedApi/Controller/Nodes.pm - sub do_restart_switchport
lib/pf/api.pm - sub _reassignSNMPConnections

@fdurand
Copy link
Member

fdurand commented Jan 14, 2021

Not really agree with this PR.
setAdminStatus is a SNMP based function and now we use it for CoA.

We should first use something like _bouncePortCoa here https://github.com/inverse-inc/packetfence/pull/6095/files#diff-7846c679ffe675238b4ec682eb6ed4537c7de86e1dfb409781b3b9a3d0f71ceeR196 and only keep the $mac extra parameter to bouncePort.

@jrouzierinverse
Copy link
Member Author

@jrouzierinverse
Copy link
Member Author

jrouzierinverse commented Jan 14, 2021

@fdurand setAdminStatus was already using radius calls for the following modules.
lib/pf/Switch/Juniper/EX2300.pm
lib/pf/Switch/Pica8.pm
This PR does not change that.

if (!$mac) {
$logger->info("Can't find MAC address in the locationlog... we won't perform port bounce");
return $TRUE;
}

if ( !$self->isProductionMode() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check should be in the bouncePort sub, like Template module.


#We need to fetch the MAC on the ifIndex in order to bounce switch port with CoA.
if (!$mac) {
my @locationlog = locationlog_view_open_switchport_no_VoIP( $self->{_ip}, $ifIndex );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? The lookup is already done in the Node.pm

@lyubomirtraykov
Copy link
Contributor

Will this fix be merged before 10.3 release?

@nqb
Copy link
Contributor

nqb commented Apr 14, 2021

no

@lyubomirtraykov
Copy link
Contributor

Will this be merged anytime soon?

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.

bouncePortCoA can bounce wrong node
4 participants