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

New Resource Suggestions #76

Closed
PlagueHO opened this issue Dec 29, 2015 · 28 comments
Closed

New Resource Suggestions #76

PlagueHO opened this issue Dec 29, 2015 · 28 comments

Comments

@PlagueHO
Copy link
Member

Now that the majority of core networking DSC resources are pretty much complete, it might be useful to look at implementing some of the more advanced networking features for servers.

In my mind QoS Policy is high up on this list and something I use regularly (especially to manage Live Migration and Cluster traffic). I'll implement this at some point in 2016, unless someone out there wants to have a go at it? 😄

@tysonjhayes
Copy link
Collaborator

Host File managment would be high on my list. Requested WAY back in #8 and we've never gotten back to it.

On my list was (in no real order)

Then really anything else that makes sense to add from the Network Cmdlet list

@PlagueHO
Copy link
Member Author

I was thinking of implementing NLB a while back, but I thought it should actually be implemented in a separate DSC module (e.g. MSFT_xNLB) because it is would require several resources itself:

  1. Resource for creating the cluster and specifying hosts.
  2. Configure port rules
  3. Control ports for specific hosts (e.g. host port prioritization etc).

But IPSec commands would be a definite important one.

As for the Hosts resource, it looks like @rchaganti was going to submit a PR with this resource at some point. Is this still available? Might save us some time, otherwise it shouldn't be a hard one to get done.

Shall I change the name of this issue to be a "Networking Resource Wish List"?

@tysonjhayes
Copy link
Collaborator

I think breaking it out into seperate issues is probably best, I just globbed onto it as I've been meaning to write it down and wanted to discuss priorities on some of them anyway.

Looks like the Host File DSC resource was deleted. The PR #13 has what he wanted to merge not sure as to the state of the code though.

@PlagueHO
Copy link
Member Author

Def - totally agree - when we the new resources start to be implemented we could create new issues for the actual resource. But having a "wish list" might be a nice place to keep ideas for future stuff in a single place until someone wants to move them forward. Might keep the issues list from getting too cluttered with "nice to have'".

I think I looked at the Hosts resource and it was OK, but was done so long ago it didn't have any of the updated best practices that have been introduced since (e.g. Unit/Integration tests). Would probably be just as easy to create it from scratch as over half the work would go into the tests anyway I think.

@rchaganti
Copy link
Contributor

I removed the HostsFile resource PR as it was consistently failing tests. We don't yet know what was the root-cause. All the similar or same tests were passing outside the AppVeyor. There is more work on the style guidelines too. I feel, it is a little tedious to contribute new resources to this repository. While I understand that the guidelines ensure quality, we need to have those script analyzer rules to test it offline and not keep changing PRs here.

@PlagueHO
Copy link
Member Author

@rchaganti, good to hear from you 😄

We have done quite a bit of work on standardizing the unit and integration test formats in recent months, specifically by creating unit and integration test templates (see the Templates folder for the templates). These templates help ensure that you can run these tests on your local machine as well as up on AppVeyor. So if you can update your unit tests to this new template format, then you should be able to run all the tests on your local computer without pushing the commits. The templates also ensure the DSCResoure.Tests are downloaded and run on the resource as well (which includes running PSScriptAnalyzer).

I'm happy to help out with anything I can on this if you're still interested in merging this resource into this module?

@tysonjhayes
Copy link
Collaborator

It should be noted it downloads and runs PSScriptAnalyzer if i you have WMF 5.0 installed, as backwards compatability for PSScriptAnalyzer isn't done yet.

@rchaganti you noted it was tedious to contribute here, could you elaborate more on what is tedious about contributing?

@rchaganti
Copy link
Contributor

@tysonjhayes the style guidelines are listed well but making those changes to existing code is certainly tedious. My coding style, like many others, is:
identifier {

}
however, style guidelines expects this as
identifier
{

}
This is a trivial thing to change but making sure I have everything as per styling guidelines is tedious. I was looking for some script analyzer rules so that I can fix everything before I make a pull request but did not find any. I have not yet looked at the templates @PlagueHO mentioned. I will review that see how I can automate a few of these things so that it doesn't become a time consuming task. I have over 30 custom resources that I want to contribute here. Updating all that as per contribution guidelines here is certainly tedious! :)

@PlagueHO PlagueHO changed the title New Resource Suggestion - QoS Policy New Resource Suggestions Jan 1, 2016
@PlagueHO
Copy link
Member Author

PlagueHO commented Jan 1, 2016

I was also of the:

identifier {
}

style, but adjusting to using this style for new projects has been fairly easy (although I do still make the occasional mistake as @tysonjhayes will attest). I can see though that changing many existing projects to use this style would be more difficult and error prone.

But perhaps there is a way of detecting this issue and automating the resolution - just as we do with tabs/spaces and non-UTF8 files. A simple Regex could easily detect when/where the problem occurs and added to Meta.Tests.ps1 in DSCResource.Test. A fixer could then be added to the DSCResource.Tests as well. @tysonjhayes, is this in your opinion something worth pursuing? If so I could raise an Issue in DSCResource.Tests with the suggestion.

@tysonjhayes
Copy link
Collaborator

I think it's worth bringing up more broadly, I believe the style is for DSC resources and we should be held to that standard. The only issue I could foresee with putting it into DSCResource.Test is it'll fail builds until everything is fixed (which depending on the style adherence in current resources may be a while... 😆 ) I lack exposure to PSScriptAnalyzer, does it account for this? I could see it as a warning and being useful there as we migrate all resources to the standard.

However, I am more interested in getting the logic of the code vs a debate about where a brace lives. I would be OK with a PR that doesn't match that style with the intention of fixing it at a later point. Even if I'm the one to do it... 😦 however I would want it to at least adhere to verb-noun naming, with approved verbs, as renaming functions can be a pain in the ass depending on how the module/tests are written, and obviously unit tests, which would need to pass before merge. Integration is more optional as we're still figuring out how to do it well without breaking everyones machines. 👍

What's everyone's thoughts on this?

@PlagueHO
Copy link
Member Author

PlagueHO commented Jan 1, 2016

I'm all for consistency when it comes to code - as long as it's all the same I'm not too fussed.

PSScriptAnalyzer does have an extensible rule model where we could add additional rules. Whether or not these rules will support what we want to test for I'm not. This brace rule could be flagged as a warnings as opposed to an error, but currently the DSCResource.Tests/Meta.tests.ps1 PSScriptAnalyzer tests only checks for Errors - and tests will fail if there are any.

It could be adjusted so that it displayed warnings but only failed on errors though I think.

I'd actually be OK with getting the code in there and fixing this sort of style issue after the fact as well. And I'm sure someone else out there as had a need to do something similar (with C# or Java files) so I'm sure there must be existing fixers that could be used to automate this process to some degree. The Verb-noun cmdlet naming isn't actually detected in the PSScriptAnalyzer default rules either.

I'm actually most interested in ensuring Unit tests are provided and if possible Integration tests (xLocalhosts should be able to have integration tests added easily).

Sorry about the jumble of ideas here! But the more I think on this the more I'd definitely like to try and make sure we adhere to the DSC Styleguide as much as possible - even if it is a bit painful to begin with. My 2c.

I think PSScriptAnalyzer will pick up Verb-Noun naming as well as other standard PS Style related issues. But I think it is currently set up as errors only

@PlagueHO
Copy link
Member Author

PlagueHO commented Jan 1, 2016

Also, I've got a PR waiting in the DSCResource.Tests module to fix the PSScriptAnalyzer tests as they're not quite doing what they should (the failed rules aren't actually displayed so you can't see what failed).

Interesting Note I just learned: This style of braces is called Egyptian Braces:
identifier {

}

I've found that SAPIEN PowerShell Studio 2014 will actually format code in the required format:
https://www.sapien.com/blog/2014/06/10/powershell-studio-2014-code-formatting/

I don't suppose anyone is using this tool? If so they could possibly use to to make the changes to the braces more easily for any commited code. This could save @tysonjhayes making the changes to the braces manually.

@rchaganti
Copy link
Contributor

Agree with you guys on the style and consistency. Within the style guidelines, can we mark the ones that we think are most desired vs good to have? I think that will clear some air. I am working on updating my code and will push a PR soon.

@PlagueHO
Copy link
Member Author

PlagueHO commented Jan 4, 2016

Awesome @rchaganti ! Thank you 👍

I'll leave @tysonjhayes to comment on the style guidelines "most wanted" :)

@PlagueHO
Copy link
Member Author

PlagueHO commented Jan 4, 2016

One final resource I'd like to see implemented:
NIC Teaming

:)

@rchaganti
Copy link
Contributor

@PlagueHO
Copy link
Member Author

PlagueHO commented Jan 5, 2016

Awesome! 👍 @tysonjhayes has split this Issue into separate issues, so that would be Issue #80 you'd be addressing with that.

I can't speak for @tysonjhayes, but I'm happy to help out in any way you need to get these resources integrated into this module. I invested in a license for PowerShell studio 2015 which can convert { to new lines } with one click.

@kbick675
Copy link

kbick675 commented Mar 5, 2016

A resource for Add-NetLbfoTeamNic would be really nice to have implemented. I regularly have to implement multiple VLANs/Nics per network team.

@rchaganti
Copy link
Contributor

It's already there in the xnetworking module.

Sent from Outlook Mobilehttps://aka.ms/blhgte

On Fri, Mar 4, 2016 at 4:48 PM -0800, "kbick675" <[email protected]mailto:[email protected]> wrote:

A resource for Add-NetLbfoTeamNic would be really nice to have implemented.

Reply to this email directly or view it on GitHubhttps://github.com//issues/76#issuecomment-192539461.

@kbick675
Copy link

kbick675 commented Mar 5, 2016

@rchaganti There is a resource for the team, not the Nics you would create and are associated with the team. The resource that exists is equivalent to New-NetLbfoTeam.

@rchaganti
Copy link
Contributor

Aah! Gotcha! Wait for it... :-)

Sent from Outlook Mobilehttps://aka.ms/blhgte

On Fri, Mar 4, 2016 at 5:03 PM -0800, "kbick675" <[email protected]mailto:[email protected]> wrote:

@rchagantihttps://github.com/rchaganti There is a resource for the team, not the Nics you would create and are associated with the team. The resource that exists is equivalent to New-NetLbfoTeam.

Reply to this email directly or view it on GitHubhttps://github.com//issues/76#issuecomment-192541184.

@randomchance
Copy link

randomchance commented Apr 27, 2016

I would also like to have a New-NetLbfoTeamNic resource, and it would be nice to have resources for New-NetTransportFilter as well.

On the subject of NLB resources - Server 2016 has VXLAN support in the VirtualSwitch, which, if enabled, does not allow mangling MAC address, which in turn rules out NLB. I think the new SDN Software LoadBalancer will be the replacement for it but I'm not really sure...

I'm not saying don't make an NLB resource, it could be super handy right now, just pointing out that it looks like NLB may start going the way of the dodo.

@PlagueHO
Copy link
Member Author

PlagueHO commented Apr 28, 2016

I must look into this VXLAN stuff you speak of though. But I think the VXLAN support would only rule out NLB if using it in mulitcast mode. If it was a unicast mode (most are I'd think) NLB then it would still work I think because the MAC doesn't need to be spoofed in that case.

I have been meaning to implement a new DSC Module for NLB, but I think it'll be unlikely I'll get time to do it before WS2016 RTM is out anyway 😄 (someone else might get to it though) - It should be implemented as a completely separate resource module anyway (because it would consist of multiple resources).

I too look forward to knowing what MS will do with NLB - I guess it would be smart of them to use the Azure SDN load balancer. But I haven't seen anything either way.

Thanks for the info!

@rchaganti
Copy link
Contributor

@randomchance NetLBFOTeam resource is already there. I will push the VMNetAdapter, VMNetadapterVlan, and VMNetAdapterSettings soon after I finish some more testing.

@PlagueHO
Copy link
Member Author

@rchaganti - That is awesome! 👍

@PlagueHO
Copy link
Member Author

@rchaganti - Am I correct in thinking you're adding these to the Hyper-V resource? Definitely the best place for them!

@tysonjhayes , @randomchance , @kbick675: Is it OK if I close this issue? Any future resource requests could be added as separate issues so they're easier to track.

@tysonjhayes or @TravisEz13: Could you possibly mark the existing [New Resource] issues as "Needs Help" and "Enhancement"?

#115
#114
#111

Thank you!

@rchaganti
Copy link
Contributor

yes! xVMNetworkAdapter is already pushed to xHyper-V and waiting to be merged. xVMNetworkAdapterVLAN and xVMNetworkAdapterSettings will also land there.

@tysonjhayes
Copy link
Collaborator

Marked everything accordingly. Went ahead and closed the issue too. thanks everyone!

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

No branches or pull requests

5 participants