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

Added more DELL switches in order to get proper CPU stats #10529

Merged
merged 21 commits into from Aug 29, 2019

Conversation

@rdezavalia
Copy link
Contributor

commented Aug 16, 2019

More DELL switches supported:

  • N4032
  • N2048
  • N2024
  • N3048
  • N2048P
  • N4064F
  • 54XX

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

Added more DELL switches in order to get proper CPU stats
More DELL switches supported:

* N4032
* N2048
* N2024
* N3048
* N2048P
* 54XX
@CLAassistant

This comment has been minimized.

Copy link

commented Aug 16, 2019

CLA assistant check
All committers have signed the CLA.

rdezavalia added 3 commits Aug 16, 2019
@label-actions

This comment has been minimized.

Copy link

commented Aug 18, 2019

Please add test data so we can ensure your change is not broken in the future.

Read the docs to find out how: https://docs.librenms.org/Developing/os/Test-Units

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

Hi @rdezavalia
Thanx for your pull request ! Looks good !
Could you please add test data for devices that this PR is adding, in order to improve the regression tests set ?
Thanx

@rdezavalia

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Thanks @PipoCanaja. Is there a way to get the test data from my switches ? Are there any guidelines in order build the test data? Hopefully I'll be able do this next week. If you have any pointer it will be very useful.

@rdezavalia

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Sorry @PipoCanaja I've just saw this link https://docs.librenms.org/Developing/os/Test-Units ... I should have an update in a few days. Thanks!

@rdezavalia

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

@PipoCanaja I've just added test data for N4064F. Could you please validated it is correct ? If it is, I will generate data for the other devices.

@rdezavalia

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

Also I was thinking about refactoring this code, so we don't need to add every switch by hand, does this code make sense to you ? I have been testing it with our hardware and it is working ok:


public function discoverProcessors()
    {
        $device = $this->getDevice();

	$device_id = array_pop(explode('.',$device['sysObjectID']));

	if ($device_id > 3019 && $device_id < 3040) {
            d_echo("Dell Powerconnect 55xx");
            return array(
                Processor::discover(
                    'powerconnect-nv',
                    $this->getDeviceId(),
                    '.1.3.6.1.4.1.89.1.7.0',
                    0
                )
            );
	} elseif ($device_id > 3039 && $device_id < 3070) {
            return $this->discoverVxworksProcessors('.1.3.6.1.4.1.674.10895.5000.2.6132.1.1.1.1.4.9.0');
        }

        return $this->discoverVxworksProcessors('.1.3.6.1.4.1.674.10895.5000.2.6132.1.1.1.1.4.4.0');
    }

@murrant

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@rdezavalia That code looks a bit cleaner but probably won't save future edits ;)

murrant added 2 commits Aug 28, 2019
@murrant

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Thanks @rdezavalia nice work with the test data.

@murrant murrant added this to the 1.55 milestone Aug 28, 2019

@rdezavalia

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

@rdezavalia That code looks a bit cleaner but probably won't save future edits ;)

Hi @murrant, Sure, I agree with you. The code does look cleaner to me. I have not yet committed this refactor code. Should I do it or do you prefer to keep the style as it was ? Also I have noted that device_id is already use inside librenms in order to identify each device that is being monitor. Probably using a different variable will make the code less confusing. Does snmp_device_id sound good to you ?

@murrant

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

I reorganized the code a bit and it looks nicer to read now. I'll merge this as-is. Feel free to submit further changes if you like. With the extra test data, it will be easy to tell if it is correct.

@murrant murrant merged commit a0ba412 into librenms:master Aug 29, 2019

5 of 6 checks passed

codeclimate 1 issue to fix
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@murrant

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-55-release-changelog-august-2019/9428/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.