Skip to content

Conversation

@csrwng
Copy link

@csrwng csrwng commented Oct 22, 2018

Introduces the capability of adding machines to specified Load Balancers as they are created or updated.

The LoadBalancer field in ProviderConfig is renamed from LoadBalancerIDs to LoadBalancerNames and its type set to string. AWS only allows specifying load balancers by name, and their describe call doesn't allow Filters.

Depends on #81

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2018
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 22, 2018
@csrwng csrwng force-pushed the elb_update branch 2 times, most recently from 0ee4af6 to ad560c4 Compare October 24, 2018 14:27
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 24, 2018
@csrwng csrwng changed the title WIP: Load Balancer update Load Balancer update Oct 24, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2018
@csrwng
Copy link
Author

csrwng commented Oct 24, 2018

@ingvagabund ptal

Changes the type/name of the LoadBalancers field in ProviderConfig. AWS
only allows identifying load balancers by name, and their Describe call
doesn't allow Filters.
@ingvagabund
Copy link
Member

ingvagabund commented Oct 24, 2018

@spangenberg good candidate for another e2e test. Something like:

  1. actuator updates ELB
  2. e2e checks an instance is registered to the ELB

Could be even done as part of the Can create AWS instances. Can you take a look at it?


}

err = a.UpdateLoadBalancers(client, machineProviderConfig, newestInstance, mLog)
Copy link
Member

Choose a reason for hiding this comment

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

could inline it here

@enxebre
Copy link
Member

enxebre commented Oct 24, 2018

/lgtm
cc @ingvagabund

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2018
Instances: []*elb.Instance{elbInstance},
LoadBalancerName: aws.String(elbName),
}
_, err := client.RegisterInstancesWithLoadBalancer(req)
Copy link
Member

Choose a reason for hiding this comment

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

could inline here as well

@ingvagabund
Copy link
Member

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2018
@csrwng
Copy link
Author

csrwng commented Oct 24, 2018

looks like the router test is not finding what it expects:

Found router in openshift-ingress
error: .status.conditions accessor error: Failure is of the type string, expected map[string]interface{}
error deploy/router did not come up

/retest

@openshift-merge-robot openshift-merge-robot merged commit 512c094 into openshift:master Oct 24, 2018
@csrwng csrwng deleted the elb_update branch November 1, 2018 20:49
vikaschoudhary16 pushed a commit to vikaschoudhary16/cluster-api-provider-aws that referenced this pull request Jan 16, 2019
Signed-off-by: Vince Prignano <[email protected]>

add comments

Signed-off-by: Vince Prignano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants