Skip to content
This repository was archived by the owner on Oct 7, 2022. It is now read-only.

Upgrade aws-sdk to v2 #32

Open
wants to merge 4 commits into
base: 0.3.0
Choose a base branch
from

Conversation

tatsuyafw
Copy link

@tatsuyafw tatsuyafw commented Oct 19, 2017

Hi!

This PR upgrades aws-sdk to v2.
I have tested with my personal AWS account, and all specs passed.

@tatsuyafw tatsuyafw changed the title Upgrade aws sdk to v2 Upgrade aws-sdk to v2 Oct 19, 2017
@k1LoW
Copy link
Contributor

k1LoW commented Nov 18, 2017

Hi @tatsuyafw ! Thank you for GREAT CONTRIBUTION !!!! I want to solve this, too.

Could you check following point ?

1. Some test fail

In my environment (using this ), bundle exec rspec spec/piculet_update_permission_spec.rb fail.

  1) Piculet::Client update tcp ingress permission allow from ip ranges and groups
     Failure/Error: @security_group.authorize_ingress(params)

     Aws::EC2::Errors::InvalidPermissionDuplicate:
       the specified rule "peer: sg-16ed1a6f, TCP, from port: 80, to port: 81, ALLOW" already exists
     # ./lib/piculet/wrapper/permission-collection.rb:37:in `authorize'
     # ./lib/piculet/wrapper/permission.rb:35:in `update'
     # ./lib/piculet/client.rb:189:in `block in walk_permissions'
     # ./lib/piculet/client.rb:184:in `each'
     # ./lib/piculet/client.rb:184:in `walk_permissions'
     # ./lib/piculet/client.rb:169:in `walk_security_group'
     # ./lib/piculet/client.rb:143:in `block in walk_ec2'
     # ./lib/piculet/client.rb:137:in `each'
     # ./lib/piculet/client.rb:137:in `walk_ec2'
     # ./lib/piculet/client.rb:107:in `block in walk'
     # ./lib/piculet/client.rb:99:in `each'
     # ./lib/piculet/client.rb:99:in `walk'
     # ./lib/piculet/client.rb:13:in `apply'
     # ./spec/spec_helper.rb:43:in `block in groupfile'
     # ./spec/spec_helper.rb:41:in `each'
     # ./spec/spec_helper.rb:41:in `groupfile'
     # ./spec/piculet_update_permission_spec.rb:58:in `block (5 levels) in <top (required)>'

Could you check again ?

2. Very slow

I ran the test, but I feel it is very slow....

See following output that use --profile option

Top 6 slowest example groups:
  Piculet::Client
    214.65 seconds average (2146.47 seconds / 10 examples) ./spec/piculet_update_permission_spec.rb:1
  Piculet::Client
    170.86 seconds average (1708.58 seconds / 10 examples) ./spec/piculet_delete_permission_spec.rb:1
  Piculet::Client
    130.04 seconds average (6501.76 seconds / 50 examples) ./spec/piculet_create_permission_spec.rb:1
  Piculet::Client
    73.98 seconds average (73.98 seconds / 1 example) ./spec/piculet_merge_spec.rb:1
  Piculet::Client
    65.83 seconds average (329.15 seconds / 5 examples) ./spec/piculet_update_tags_spec.rb:1
  Piculet::Client
    55.21 seconds average (276.03 seconds / 5 examples) ./spec/piculet_spec.rb:1
Finished in 183 minutes 11 seconds (files took 0.48543 seconds to load)
81 examples, 10 failures

How about in your environment ?

@tatsuyafw
Copy link
Author

tatsuyafw commented Nov 19, 2017

@k1LoW

Thank you for reply!

First, this PR has conflicts, so I will fix that. Then I'll check the points you've mentioned.
Please wait a few days.

@tatsuyafw tatsuyafw force-pushed the upgrade-aws-sdk-to-v2 branch from 58d9690 to d8a53a6 Compare November 19, 2017 14:25
@tatsuyafw
Copy link
Author

@k1LoW

I've rebased this PR from latest 0.3.0 branch to fix conflicts.

  1. Some test fail

In my environment (using this ), bundle exec rspec spec/piculet_update_permission_spec.rb fail.

I find some tests are unstable... Sometimes all tests passed, and sometimes some tests failed on my machine.
I try to fix this issue within a few days.

  1. Very slow

I ran the test, but I feel it is very slow....

Finished in 183 minutes 11 seconds (files took 0.48543 seconds to load)
81 examples, 10 failures

Hmm, It's too slow... 😢
In my environment, the tests were more faster as follows.

$ be rspec --profile
.................................................................................
# abbr.
Top 6 slowest example groups:
  Piculet::Client
    8.21 seconds average (82.15 seconds / 10 examples) ./spec/piculet_update_permission_spec.rb:1
  Piculet::Client
    6.75 seconds average (6.75 seconds / 1 example) ./spec/piculet_merge_spec.rb:1
  Piculet::Client
    5.66 seconds average (56.64 seconds / 10 examples) ./spec/piculet_delete_permission_spec.rb:1
  Piculet::Client
    5.55 seconds average (27.75 seconds / 5 examples) ./spec/piculet_update_tags_spec.rb:1
  Piculet::Client
    4.66 seconds average (233.2 seconds / 50 examples) ./spec/piculet_create_permission_spec.rb:1
  Piculet::Client
    3.46 seconds average (17.29 seconds / 5 examples) ./spec/piculet_spec.rb:1

Finished in 7 minutes 4 seconds (files took 0.47945 seconds to load)
81 examples, 0 failures
$

@k1LoW
Copy link
Contributor

k1LoW commented Nov 19, 2017

I try to fix this issue within a few days.

Thank you ! I'm looking forward to good news !

In my environment, the tests were more faster as follows.

OK! It may only happen in my environment. Thank you !!

@k1LoW
Copy link
Contributor

k1LoW commented Nov 22, 2017

I use rbtrace.gem.

Aws::EC2::Resource#create_random_security_group seems to be slow.

                                                                   Net::Protocol#ssl_socket_connect <0.427559>
                                                                 Net::HTTP#connect <0.635031>
                                                               Net::HTTP#do_start <0.635046>
                                                             Net::HTTP#start <0.635054>
                                                           Delegator#method_missing <0.635062>
                                                         Seahorse::Client::NetHttp::ConnectionPool#start_session <0.636190>
                                                       Seahorse::Client::NetHttp::ConnectionPool#session_for <0.849484>
                                                     Seahorse::Client::NetHttp::Handler#session <0.849721>
                                                   Seahorse::Client::NetHttp::Handler#transmit <0.849733>
                                                 Seahorse::Client::NetHttp::Handler#call <0.849786>
                                               Seahorse::Client::Plugins::ContentLength::Handler#call <0.849807>
                                             Aws::Xml::ErrorHandler#call <0.850015>
                                           Aws::Plugins::RequestSigner::Handler#call <0.850516>
                                         Aws::Plugins::HelpfulSocketErrors::Handler#call <0.850552>
                                       Aws::Plugins::RetryErrors::Handler#call <0.850637>
                                     Aws::Query::Handler#call <0.850768>
                                   Aws::Plugins::UserAgent::Handler#call <0.850796>
                                 Seahorse::Client::Plugins::Endpoint::Handler#call <0.850967>
                               Aws::Plugins::ParamValidator::Handler#call <0.850991>
                             Seahorse::Client::Plugins::RaiseResponseErrors::Handler#call <0.851087>
                           Aws::Plugins::JsonvalueConverter::Handler#call <0.851128>
                         Aws::Plugins::IdempotencyToken::Handler#call <0.851152>
                       Aws::Plugins::ParamConverter::Handler#call <0.851193>
                     Seahorse::Client::Plugins::ResponseTarget::Handler#call <0.851208>
                   Seahorse::Client::Request#send_request <0.851382>
                 Aws::IAM::Client#get_user <0.851785>
               Aws::EC2::Resource#get_owner_id_from_iam <0.862053>
                     Kernel#sleep <3.004508>
                     Kernel#sleep <3.004647>
                     Kernel#sleep <3.004382>
                   Integer#times <9.015396>
                 Aws::EC2::Resource#create_random_security_group <9.015412>
               Aws::EC2::Resource#get_owner_id_from_security_group <9.015422>
             Aws::EC2::Resource#owner_id <9.877501>
           Aws::EC2::Resource#own? <9.877518>
         Piculet::EC2Wrapper::SecurityGroupCollection::SecurityGroup::PermissionCollection#log_id <9.877634>
       Piculet::EC2Wrapper::SecurityGroupCollection::SecurityGroup::PermissionCollection::Permission#log_id <9.877691>
     Piculet::EC2Wrapper::SecurityGroupCollection::SecurityGroup::PermissionCollection::Permission#delete <10.108319>
   Array#each <10.108384>
 Piculet::EC2Wrapper::SecurityGroupCollection::SecurityGroup::PermissionCollection#each <10.108414>

I removed rescue nil ( https://github.com/codenize-tools/piculet/pull/32/files#diff-b96bfc3cb87b45cf69e541d4888dd333R45 )

So, raise following error.

     Failure/Error: security_group = self.security_groups.create(name)

     NoMethodError:
       undefined method `create' for #<Aws::Resources::Collection:0x00007fd65a2f6ad8>

@tatsuyafw
Copy link
Author

So, raise following error.

 Failure/Error: security_group = self.security_groups.create(name)

 NoMethodError:
   undefined method `create' for #<Aws::Resources::Collection:0x00007fd65a2f6ad8>

@k1LoW

Sorry for my late reply, and thank you for more detail!

I've fixed this NoMethodError, and confirmed that Aws::EC2::Resource#create_random_security_group works fine.
I think the tests in your environment become faster than before. Would you be able to check that?

@k1LoW
Copy link
Contributor

k1LoW commented Dec 1, 2017

screncaptured 2017-12-02 0 15 47

:port_range => port_range,
:ip_ranges => ip_ranges,
:groups => ip_perm.user_id_group_pairs.map {|group|
group = @ec2.security_groups.find { |g| g.id == group.group_id }
Copy link
Contributor

Choose a reason for hiding this comment

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

L61's group and L63's group is different.

And L61's group may be nil.

I try bundle exec bin/piculet -e on my environment . So, raise following error.

[Aws::EC2::Client 200 0.186472 0 retries] describe_security_groups()

bundler: failed to load command: bin/piculet (bin/piculet)
NoMethodError: undefined method `group_id' for nil:NilClass
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:63:in `block (2 levels) in export_ip_permissions'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:60:in `map'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:60:in `block in export_ip_permissions'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:51:in `map'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:51:in `export_ip_permissions'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:43:in `export_security_group'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:30:in `block in export'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:22:in `each'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:22:in `export'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/exporter.rb:5:in `export'
  /Users/k1low/.ghq/github.com/codenize-tools/piculet/lib/piculet/client.rb:41:in `export'
  bin/piculet:182:in `<top (required)>'

Copy link
Author

@tatsuyafw tatsuyafw Dec 2, 2017

Choose a reason for hiding this comment

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

Thanks for your review!

I cannot immediately come up with why @ec2.security_groups.find { |g| g.id == group.group_id } returns nil, so I try to search the cause.

BTW, I added the L61 @ec2.security_groups.find { |g| g.id == group.group_id } to fetch group.group_name info. Because ip_perm.user_id_group_pair's group (L60) has always no information about group_name as below.

    57:           :protocol   => ip_protocol,
    58:           :port_range => port_range,
    59:           :ip_ranges  => ip_ranges,
    60:           :groups => ip_perm.user_id_group_pairs.map {|group|
    61:             require 'pry'
 => 62:             binding.pry
    63:             group = @ec2.security_groups.find { |g| g.id == group.group_id }
    64:             {
    65:               :id       => group.group_id,
    66:               :name     => group.group_name,
    67:               :owner_id => group.owner_id,

[1] 2.4.1-p111(#<Piculet::Exporter>)> group
=> #<struct Aws::EC2::Types::UserIdGroupPair
 description=nil,
 group_id="sg-xxx",
 group_name=nil, # here!
 peering_status=nil,
 user_id="752xxxxxxxxx",
 vpc_id=nil,
 vpc_peering_connection_id=nil>
[2] 2.4.1-p111(#<Piculet::Exporter>)>

@tatsuyafw
Copy link
Author

@k1LoW

Sorry for my late reply...
I refactored the code, but I did not found why bundle exec bin/piculet -e caused the nil error in your environment.

Could you try running bundle exec bin/piculet -e in your environment using the latest commit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants