search

aws-samples / cloud-gaming-on-ec2-instances

https://aws.amazon.com/blogs/compute/use-amazon-ec2-for-cost-efficient-cloud-gaming-with-pay-as-you-go-pricing/
64 stars 19 forks source link

Refactor BaseEc2Stack #17

Open yeralin opened 11 months ago

yeralin commented 11 months ago

Description of changes: Increase readability of the BaseEc2Stack by breaking creation of resources into sub-methods, and refactoring Nvidia and AMD configurations into separate files.

Confirmed that the changes did not introduce any regressions as the final CF templates are identical before and after changes.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

markusz commented 11 months ago

Hi @yeralin

thank you very much for your pull request. Let me test and review this and get back to you

Regards Markus

markusz commented 11 months ago

Hi @yeralin

I reviewed the changes and have two remarks:

  1. ESLint is failing, mostly due to indentation, dangling commas etc.

image

  1. I get a diff between a stack deployed from aws-samples:main and yearlin:main. Can you please explain what the change does and why it is required? Ideally, there would be no diff at all, as this might lead to unexpected behavior for already deployed stacks

image

yeralin commented 11 months ago

Apologies, my editor was configured incorrectly. Resolved eslint errors.

I believe that the stack diff output is a red hearing. All config sets were left intact. Most likely CloudFormation gets confused since they were refactored into separate functions.

markusz commented 11 months ago

ESLint looks good now, but I think the diff between the two version is real.

If you compare the CloudFormation template from the original branch to the CF template synthesized by your CR, there is a delta in it

image

yeralin commented 10 months ago

Ok, got rid of diff on the configs.

All I had to do is to move them in between helpersPreinstall and reboot configs i.e.:

helpersPreinstall: ...,
...nvidiaConfigs,
...amdConfigs,
reboot: ...

So it was red hearing after all.

As per the fingerprint, I am also getting it, but could not get rid of it. Checked the code line-by-line, and it looks identical.

yeralin commented 10 months ago

Btw, I noticed that by default the stack doesn't open RDP port 3389.

Which causes RDP connection to fail:

You may not be able to connect to this instance as ports 3389 may need to be open in order to be accessible. The current associated security groups don't have ports 3389 open.

I think it should be added to the list of OPEN_PORTS. Any reason it wasn't?

Seblat5ch commented 10 months ago

Btw, I noticed that by default the stack doesn't open RDP port 3389.

Which causes RDP connection to fail:

You may not be able to connect to this instance as ports 3389 may need to be open in order to be accessible. The current associated security groups don't have ports 3389 open.

I think it should be added to the list of OPEN_PORTS. Any reason it wasn't?

I removed RDP because this solution focuses on NICEDCV which in this cases uses Port 8443 on TCP/UDP depending if QUIC is used or not. RDP port 3389 is also a very well known port and could bring with unexpected security risks when open up to the Internet

yeralin commented 10 months ago

Got it, then most likely this article must be updated: https://aws.amazon.com/blogs/compute/use-amazon-ec2-for-cost-efficient-cloud-gaming-with-pay-as-you-go-pricing

It confused me a little while I was setting up the stack.

Seblat5ch commented 9 months ago

Got it, then most likely this article must be updated: https://aws.amazon.com/blogs/compute/use-amazon-ec2-for-cost-efficient-cloud-gaming-with-pay-as-you-go-pricing

It confused me a little while I was setting up the stack.

Agreed. We are working on it.