search

jcjorel / ec2-spot-converter

A tool to convert AWS EC2 instances back and forth between On-Demand and Spot billing models.
MIT License
190 stars 20 forks source link

ec2 target group registration is lost after transition #3

Closed regevbr closed 3 years ago

regevbr commented 3 years ago

I have load balancer target groups that have 2c2 instances registered as their targets. After the transition to spot, those registrations are lost. Suggested fix: save the registration information of the instances and reattach them after the new instance is up

regevbr commented 3 years ago

I'm willing to try to create a PR will update soon on my progress

jcjorel commented 3 years ago

Hi, Indeed, the tool do not try to guess where the initial instance Id is used (like TargetGroups but can be also elsewhere...) for sake of simplicity and so resilience. As it is a common use-case, indeed, we could assess if it is a feature that can be added safely. About a PR, please keep in mind that it must be bulletproof against any possible error conditions and must allow state machine step replay (= be able to restart the tool at any point of interruption safely).

regevbr commented 3 years ago

Yes indeed it can happen in many different services of AWS. Of course, it will be bulletproof and I hope you will be able to provide a meaningful CR to achieve that :-) Work is already underway

jcjorel commented 3 years ago

Hi, Sorry. I had no time to review all your PRs. I'm going to look at them this evening!

regevbr commented 3 years ago

Great thanks @jcjorel I think I'm done working on it hahah

jcjorel commented 3 years ago

There are too much changes to merge directly into the master branch. Could you create a new PR to the 'elb-support' branch plz? Thanks.

jcjorel commented 3 years ago

Finally, I forgot that I could change the base branch myself. I merged your PR in the 'elb-support' branch.

jcjorel commented 3 years ago

(BTW, that's an impressive job in such limited amount of time. ;-) )

regevbr commented 3 years ago

(BTW, that's an impressive job in such limited amount of time. ;-) )

Thanks :-)

There are too much changes to merge directly into the master branch. Could you create a new PR to the 'elb-support' branch plz? Thanks.

@jcjorel I'm not sure why did you merge it to the elb-support branch? what is the difference if eventually, you will merge it all into the master?

And sorry, yes there are many changes there but 90% of them was in order to support and test out the elb feature :-)

jcjorel commented 3 years ago

v0.9.0 released with ELB registration preservation support!! Thank you!

regevbr commented 3 years ago

@jcjorel awesome job on the final touches :-)

Just a few comments for the future (might be petty sorry):

I would love to hear your thoughts on what I said, even if they are opposite to what I believe :-)

jcjorel commented 3 years ago

Thanks for highlighting this to me. As I'm very new to GitHub, I have limited knowledge about the practices and habits. I do not know how to use GitHub to review code from someone else and it is why I pulled your PR to a branch so I was able to see it as real files: I assume that there is another way to do it differently. Do you know where I could see in depth the GitHub collaboration best-practices you are talking about?

Le mer. 27 janv. 2021 à 00:00, Regev Brody notifications@github.com a écrit :

@jcjorel https://github.com/jcjorel awesome job on the final touches :-)

Just a few comments for the future (might be petty sorry):

  • When someone submits a PR, they expect to get a CR and make all the fixes and modifications themselves. A major part of contributing to open source (in my humble opinion), is to get reviewed by other skilled programmers.
  • If you do go ahead and make al thel modifications yourself, at least keep a squashed commit of all the contributor changes in the master commit log so the contribution stats will not be lost.

I would love to hear your thoughts on what I said, even if they are opposite to what I believe :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jcjorel/ec2-spot-converter/issues/3#issuecomment-767883835, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJPPKSZOMCQFT46MTN4YKCTS35CSHANCNFSM4WQWIZLA .

regevbr commented 3 years ago

That is ok, I'm glad you are interested :-)

In the pr you can switch to the Files changed tab to see a full diff and to be able to comment on lines of codes (similar to how we communicated on your commits) image

If you want to check out the actual files, you can open up the link to the repo and branch the PR is being merged from (PruvoNet:#3): image If you want to push new commits there (so they will be part of the PR), you can ask the contributor to give you access to their fork.

About best practices, this looks promising but I didn't read it. For me, I just learned by contributing over the years and owning my own projects. If you have any specific questions, you are more than welcome to ask me here :-)

jcjorel commented 3 years ago

Hummm.... Was definitely not aware that PR are still mutables! (Thought it was like a snapshot of static bulk changes).

Thanks a lot. I will look at your document as, really, I am bare new in this OpenSource world. As with my job, I frequently identify many opportunities to improve the way people use AWS, I started to create and publish stuff that fix people's burdens. But, yes... the process is still very naive 😎

Le mer. 27 janv. 2021 à 01:06, Regev Brody notifications@github.com a écrit :

That is ok, I'm glad you are interested :-)

In the pr https://github.com/jcjorel/ec2-spot-converter/pull/4 you can switch to the Files changed tab to see a full diff and to be able to comment on lines of codes (similar to how we communicated on your commits) [image: image] https://user-images.githubusercontent.com/5775519/105922404-4d3f4700-6043-11eb-80c2-f1a08444bb96.png

If you want to check out the actual files, you can open up the link to the repo and branch the PR is being merged from (PruvoNet:#3): [image: image] https://user-images.githubusercontent.com/5775519/105922233-ef126400-6042-11eb-9126-59917842dae3.png If you want to push new commits there (so they will be part of the PR), you can ask the contributor to give you access to their fork.

About best practices, this https://opensource.guide/ looks promising but I didn't read it. For me, I just learned by contributing over the years and owning my own projects. If you have any specific questions, you are more than welcome to ask me here :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jcjorel/ec2-spot-converter/issues/3#issuecomment-767912271, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJPPKSZDHK4OCK56JDYIHZTS35KHLANCNFSM4WQWIZLA .

regevbr commented 3 years ago

haha no worries, that is excatly the right approach for open source