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

base: add SNMP module #67

Merged
merged 2 commits into from
Aug 7, 2024
Merged

base: add SNMP module #67

merged 2 commits into from
Aug 7, 2024

Conversation

guirodrigueslima
Copy link
Contributor

Added the SNMP module, developed by SLAC (National Accelerator Laboratory). This module will be used to develop the IOC for Aruba Switches to control PoE ports.

Copy link
Collaborator

@henriquesimoes henriquesimoes left a comment

Choose a reason for hiding this comment

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

As we have discussed in person, there are two repositories for SNMP at slac-epics:

Both of them seem to implement the support for SNMP. Taking a brief look at them, the latter has been changed on GitHub more recently (6 months versus 3 years ago), but supports less record types than the former. slac-epics/epics-snmp also has more custom configurations in its build, which would require us to override them.

Considering that:

  • we have tested with slac-epics/snmp and it has worked just fine for the simple IOC we need;
  • the IOC is simpler with the mbbo records we would lose with epics-snmp;
  • the build required no custom handling from our part

IMHO, we can stick with it, at least for now. If it turns out to be deprecated by SLAC or the other becomes more appealing, we can reconsider this choice.

@henriquesimoes
Copy link
Collaborator

I've complemented the commit message with the relevant points regarding the choice of using slac-epics/snmp. Can you double check if everything is okay with it?

@henriquesimoes
Copy link
Collaborator

I've asked in tech-talk about the other modules and for recommendations to see if this is indeed the best choice.

Let's see the community point of view of this.

guirodrigueslima and others added 2 commits August 2, 2024 16:25
Now lnls-get-n-unpack function downloads and extracts .zip files as
well. File extension (case insensitive) is used to check the archive
type, as it should provide a reasonable file type hint. A more robust
approach would be to use `file -b --mime-type`, but this would introduce
yet another dependency in both build and runtime images. Let's assume
file extension conventions are followed instead.

Overwrite option (-o) and quiet mode (-q) are enabled, following the
default behavior from `tar`, and avoiding interactive prompts. Ownership
is not kept by default, so no extra flag is needed.

There is no need to install unzip in IOC and musl-based image, since
busybox(1) already provides it. In the runtime image, we only need to
symlink unzip, so that it becomes available in the PATH.

Co-authored-by: Henrique F. Simoes <henrique.simoes@lnls.br>
Over time, several SNMP device support have been implemented, most (or
all) of them derived from DESY's implementation, including one from
Sheng Peng, Diamond and NSCL/FRIB. By 2013, most of community members
were willing to test NSCL/FRIB's implementation or to switch to it [1].
Advantages by then were:

- no memory leak (opposed to Sheng Peng's implementation)
- easier to work with (compared to original DESY implementation)
- supports SNMP writes (compared to Sheng's)
- depends more heavily on libsnmp (compared to Diamond's implementation)

Nowadays, Sheng Peng's implementation seems not to be developed anymore,
as well as Diamond's. From all forks, NSCL/FRIB's and SLAC's are still
around [2]. Looking through SLAC's repository history, it seems to be
also based on Sheng Peng's module [3], with several changes done
afterwards, mostly for patching bugs and supporting new record types.

Between the two, NSCL/FRIB module [4] (also forked in
slac-epics/epics-snmp [5]) has been preferred over slac-epics/snmp,
since it had more features in the past and the community seems to be
actively using, discussing and contributing to it recently [2].

Overall, record API is the same. For general SNMP-aware devices,
changing from one implementation to the other is a matter of changing
the record DTYP used, paying attention to usage of extra parsing
features in INP/OUT fields and record types used. Therefore, changing
the implementation in the future does have costs, but it should not be
very high.

Co-authored-by: Henrique F. Simoes <henrique.simoes@lnls.br>

[1]: https://epics.anl.gov/tech-talk/2013/msg00537.php
[2]: https://epics.anl.gov/tech-talk/2024/msg00889.php
[3]: slac-epics/snmp@d82df4e
[4]: https://groups.nscl.msu.edu/controls/files/devSnmp.html
[5]: https://github.com/slac-epics/epics-snmp
@henriquesimoes
Copy link
Collaborator

@guirodrigueslima, can you take a look in the changes I've made?

They are mostly documentation changes, apart from some changes to make unzip behave as tar. I had to test if they really worked, so I edited the commit directly instead of asking for changes during review.

I think we are good to merge after that.

CHANGES.md Show resolved Hide resolved
@gustavosr8 gustavosr8 self-requested a review August 7, 2024 19:26
@henriquesimoes henriquesimoes merged commit 60f793a into main Aug 7, 2024
2 checks passed
@henriquesimoes henriquesimoes deleted the snmp branch August 7, 2024 20:22
Comment on lines +23 to +29
filename=$(basename $download_dir/*)

if [[ ${filename,,} == *".zip" ]]; then
unzip -qo $download_dir/$filename -d $dest
else
tar --no-same-owner -xf $download_dir/$filename -C $dest
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if we end up having to support more formats, we should switch to bsdtar+libarchive directly, instead of having to handle different tools in the script. Since busybox already provides unzip, I think it's reasonable to depend on it.

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.

4 participants