Refactor Routes, and dynamically configure minion CIDRs on AWS #9720
Conversation
|
GCE e2e build/test failed for commit 86d492a3cc6bdb1994a583936f154db33c562203. |
|
@cjcullen FYI |
| } | ||
| }(route.Name) | ||
| }(&routeCopy) |
There was a problem hiding this comment.
I don't think you need the routeCopy here for the go func. You could pass route directly to the function because the value of the pointer is captured right then (even though the pointer is reused).
ArtfulCoder
added
sig/network
|
I like the general direction. I had to read up on how AWS does routes, but I think this is a reasonable way to make this work for both GCE and AWS. |
justinsb
force-pushed
the
aws_routes
branch
3 times, most recently
from
37dc2e0
to
ef2a78b
Compare
|
GCE e2e build/test failed for commit f45a5eea2c811da8027b3db68c9657e02ba5eee2. |
|
GCE e2e build/test passed for commit 37dc2e0e6f138c79ead261de8e34ac5f46b014de. |
|
GCE e2e build/test failed for commit ef2a78bc327b4eda914d8852d3e2dbbf3a93421c. |
|
Thanks @cjcullen - I removed the unnecessary copy. This is now working (slightly to my surprise - I expected a lot more issues!) I had to add the final commit which I also submitted separately as #9745 (should be a trivial rebase if it is even considered a conflict), and fix a few mistakes I made originally, but I'm removing the WIP designation. |
justinsb
changed the title
|
Oh - doh! This depends on #9728! Changing subject to reflect that. |
justinsb
changed the title
| @@ -30,8 +30,8 @@ function detect-minion-image (){ | |||
|
|
|||
| function generate-minion-user-data() { | |||
| i=$1 | |||
| # TODO(bakins): Is this actually used? | |||
There was a problem hiding this comment.
@bakins I can't see anywhere MINION_PRIVATE_IP is used? Is there somewhere I'm missing?
Also, heads up on this patch. Shouldn't affect CoreOS, but you never know! My plan is to finish the stream on AWS/Ubuntu and then fix what I've broken on AWS/CoreOS!
What I'm working towards here is putting all the minions in an AWS auto-scaling group (like GCE does)
There was a problem hiding this comment.
I do not see it being used anywhere. I guess I didn't when I wrote it either, so not sure why I even have it there.
|
GCE e2e build/test passed for commit e84a7d3. |
justinsb
changed the title
We need to implement the Routes interface, and then enable the functionality in the cluster scripts.
|
Dependencies have now merged, so I rebased to so we can get a test of the latest code version. |
|
GCE e2e build/test passed for commit a4e15cd. |
|
Is this one and #9940 now the same thing? |
| "github.com/aws/aws-sdk-go/service/ec2" | ||
| ) | ||
|
|
||
| func (s *AWSCloud) findRouteTable(clusterName string) (*ec2.RouteTable, error) { |
There was a problem hiding this comment.
Copying over my nit from the other PR:
You could avoid the empty declarations and just have:
filters := []*ec2.Filter{newEc2Filter("tag:"+TagNameKubernetesCluster, clusterName))}
request := &ec2.DescribeRouteTablesInput{ Filters: s.addFilters(filters) }
|
LGTM. I like it conceptually. It appears to keep GCE functionality unchanged. As we move more of this logic from individual controllers to inside the cloudprovider package we lose test coverage. But making our cloudproviders more testable is well beyond the scope of this PR :). |
cjcullen
added
the
lgtm
|
Is this ready for merge or does it still depend on other changes? |
Thanks for the suggestion @cjcullen
|
@cjcullen: Applied the code cleanup - thanks! @satnam6502: Ready for merge (no other dependencies) :-) (once 0ad16a1 passes testing) |
Refactor Routes, and dynamically configure minion CIDRs on AWS
|
GCE e2e build/test passed for commit 0ad16a1. |
WIP (edit: no longer, depended on 9728, now merged) but would greatly appreciate comments on the refactoring of the Routes interface (the first commit). We assumed that routes have settable names, but that isn't the case everywhere. I think the code is cleaner for the refactor.