sandialabs / sceptre-phenix-apps

Apps written to work with the latest version of phenix
https://github.com/sandialabs/sceptre-phenix
GNU General Public License v3.0
6 stars 14 forks source link

New Scorch components #28

Closed GhostofGoes closed 1 month ago

GhostofGoes commented 4 months ago

Add new Scorch components and enhance existing components.

New components

Existing components

General changes

activeshadow commented 4 months ago

Hi @GhostofGoes. This might take me a while (looks like a pretty big PR) but I will try to get a review done ASAP. Thanks for the contributions!

@glattercj if you can help with a review I'd appreciate an extra set of eyes.

GhostofGoes commented 4 months ago

Hi @GhostofGoes. This might take me a while (looks like a pretty big PR) but I will try to get a review done ASAP. Thanks for the contributions!

@glattercj if you can help with a review I'd appreciate an extra set of eyes.

Thanks! No rush on getting it approved, this is nearly a year of development all in one PR haha.

GhostofGoes commented 3 months ago

@activeshadow Have you had a chance to take a peek at this yet?

activeshadow commented 2 months ago

@GhostofGoes no sir not yet, sorry! I'll try to get to it ASAP.

glattercj commented 2 months ago

Aside from some dependency issues (e.g. collector depends on rtds and scenario), the only rub I see is the naming of the scenario component - it seems too generic of a name for its purpose. I didn't test any of these but I'm sure @GhostofGoes has done plenty over the time developing these. Great work.

GhostofGoes commented 2 months ago

Aside from some dependency issues (e.g. collector depends on rtds and scenario), the only rub I see is the naming of the scenario component - it seems too generic of a name for its purpose. I didn't test any of these but I'm sure @GhostofGoes has done plenty over the time developing these. Great work.

Aside from some dependency issues (e.g. collector depends on rtds and scenario), the only rub I see is the naming of the scenario component - it seems too generic of a name for its purpose. I didn't test any of these but I'm sure @GhostofGoes has done plenty over the time developing these. Great work.

Yeah, collector is definitely needs work to be more generalized. I have TODOs to make it not hard depend on other modules, hopefully that'll be addressed once I come back to this work.

Regarding naming of the scenario component, what would be a better alternative? disruption_scenario?

glattercj commented 2 months ago

Aside from some dependency issues (e.g. collector depends on rtds and scenario), the only rub I see is the naming of the scenario component - it seems too generic of a name for its purpose. I didn't test any of these but I'm sure @GhostofGoes has done plenty over the time developing these. Great work.

Aside from some dependency issues (e.g. collector depends on rtds and scenario), the only rub I see is the naming of the scenario component - it seems too generic of a name for its purpose. I didn't test any of these but I'm sure @GhostofGoes has done plenty over the time developing these. Great work.

Yeah, collector is definitely needs work to be more generalized. I have TODOs to make it not hard depend on other modules, hopefully that'll be addressed once I come back to this work.

Regarding naming of the scenario component, what would be a better alternative? disruption_scenario?

Good question...maybe just disruption? It looks like it's 2 main pieces: dos + script and phys + script? I wonder if it would make more sense to separate it out and have a dos component and a phys component.

GhostofGoes commented 2 months ago

Renaming to disruption makes sense. However, I'm against splitting it into separate components. The idea is it can run multiple different types of scenarios in the future via a more flexible mechanism. But maybe that doesn't make sense for how scorch is architected?

GhostofGoes commented 2 months ago

@GhostofGoes I'm curious why you added a new scorch component for pcap capture (via minimega API) when the mm scorch component is already capable of doing that? Does the new pcap component add additional features beyond what the mm component is capable of doing? Or simply a lack of documentation on my part, such that you didn't even know the mm component could do pcap capture?

Also, can you confirm the common utility functions added are being used by multiple apps and/or components? I just want to make sure it doesn't make sense to keep some of them as part of the single app or component that is utilizing them.

Regarding pcap, to answer your question, a bit of both. This started 2 years ago, so details are a bit fuzzy. Initially, I needed to be able to capture from Windows systems and without running a tcpdump binary (I was having a LOT of issues with miniccc on the VMs in this environment). I wasn't aware until much later that the mm component could do captures. Encapsulating the functionality into it's own component made sense to me, since I was adding some additional post-processing features, and doing that in mm would make the logic branch even further than it already is.

Do you feel strongtly that pcap should be merged back into mm? To keep the if branching to a minimum I'll have to split stuff out into functions, move the functionality over, then update the documentation to clarify that mm is can be used for PCAP capture in addition to tcpdump.

Regarding utility functions, there are some that are only used by one component at the moment. However, they were sufficiently general and standalone that it made sense to put them in utils. Alternatively, I can dump them all into the component's code, and if another component needs it, then we can migrate it back into utils, or import across components.

GhostofGoes commented 2 months ago

Another note on pcap: I was considering merging tcpdump into pcap . Essentially, consolidate PCAP-related logic into one component, and provide consistent options on how they should be captured and processed.

GhostofGoes commented 1 month ago

@activeshadow What are your thoughts on the comments above? I'd like to move this PR forward, it's been almost 3 months and I've had to point multiple people who are using these components to use the branch.

activeshadow commented 1 month ago

Hey @GhostofGoes sorry I've been slammed this summer and this is a beast of a PR.

I figured you might have started this before I did the mm component with generic capture capabilities. I think keeping the specific pcap component is a good idea still. I also like your idea of merging tcpdump into your pcap component. Maybe leave the tcpdump component for backwards compatibility of existing experiments out there and we'll label it as deprecated, but add its capabilities to the pcap component and unify config options like you suggested.

Im okay with the utility components as you have them now. I was just curious. Maybe just document them or add an issue to document them.

If @glattercj is okay with this PR after having reviewed it already, then I'm okay with giving it a LGTM to get it merged. We are trying hard to add a corresponding PR for documentation of new code PRs but I'm not sure if you have bandwidth for that.

GhostofGoes commented 1 month ago

Hey @GhostofGoes sorry I've been slammed this summer and this is a beast of a PR.

I figured you might have started this before I did the mm component with generic capture capabilities. I think keeping the specific pcap component is a good idea still. I also like your idea of merging tcpdump into your pcap component. Maybe leave the tcpdump component for backwards compatibility of existing experiments out there and we'll label it as deprecated, but add its capabilities to the pcap component and unify config options like you suggested.

Im okay with the utility components as you have them now. I was just curious. Maybe just document them or add an issue to document them.

If @glattercj is okay with this PR after having reviewed it already, then I'm okay with giving it a LGTM to get it merged. We are trying hard to add a corresponding PR for documentation of new code PRs but I'm not sure if you have bandwidth for that.

Sounds good. What documentation would you like to see for this? I can add it to the backlog and tackle when things are slow at the start of FY25.

activeshadow commented 1 month ago

At a minimum I like to have a README for each component that outlines what it does and what the config options are. You've probably seen the READMEs for other components.

For the utility functions, maybe we add a README for the common module?

GhostofGoes commented 1 month ago

At a minimum I like to have a README for each component that outlines what it does and what the config options are. You've probably seen the READMEs for other components.

For the utility functions, maybe we add a README for the common module?

All of the components should have a README. Are there any components I missed? README for the common code is definitely doable!

glattercj commented 1 month ago

I'd like to see the docs added as a PR to the main docs repo instead of just READMEs, even if it's a copy/paste of the content with some intros or whatever. Having one place to view the documentation makes it much easier for users rather than looking in multiple repos or directories for various READMEs. Other than that, LGTM.

GhostofGoes commented 1 month ago

I'd like to see the docs added as a PR to the main docs repo instead of just READMEs, even if it's a copy/paste of the content with some intros or whatever. Having one place to view the documentation makes it much easier for users rather than looking in multiple repos or directories for various READMEs. Other than that, LGTM.

Where should they be added? New items on the scorch page? Or perhaps if we should split the component docs themselves to a separate page?

glattercj commented 1 month ago

I think a separate page with user-defined Scorch components would be good. We could migrate over the READMEs with better "documentation" style info.