Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use EnsureTask instead of prepending IG names to external ELB tasks #10754

Merged
merged 3 commits into from
Feb 7, 2021

Conversation

rifelpet
Copy link
Member

@rifelpet rifelpet commented Feb 7, 2021

Fixes #10715

Partially reverts #10666

This way we end up with one CLB task per CLB regardless of how many ASGs it is attached to. Less brittle than creating many CLB tasks with concatenated names and exceptions for known CLBs.

The Name -> LoadBalancerName shouldn't make a functional difference, just improving consistency with how we attach CLBs to ASGs being created vs updated.

This way we end up with one CLB task per CLB regardless of how many ASGs to which it is attached.
…g ASGs

This shouldn't have a functional change, just improving consistency with how we attach CLBs to ASGs being created
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 7, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 7, 2021
LoadBalancerName: extLB.LoadBalancerName,
Shared: fi.Bool(true),
}
t.LoadBalancers = append(t.LoadBalancers, lb)
c.AddTask(lb)
c.EnsureTask(lb)
Copy link
Member Author

Choose a reason for hiding this comment

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

note for reviewers: EnsureTask returns an error if the second invocation provides a task that isn't DeepEqual:

kops/upup/pkg/fi/task.go

Lines 70 to 74 in 0aa9cf1

// EnsureTask ensures that the specified task is configured.
// It adds the task if it does not already exist.
// If it does exist, it verifies that the existing task reflect.DeepEqual the new task,
// if they are different an error is returned.
func (c *ModelBuilderContext) EnsureTask(task Task) error {

here, the CLB tasks should always be equal if a CLB is in multiple IGs' externaLoadBalancers because the only task fields that are set use the LoadBalancerName and shared=true, so this should never return an error.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't know about this. May be a good idea to simplify the NLB side as well someday.

@hakman
Copy link
Member

hakman commented Feb 7, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2021
@k8s-ci-robot k8s-ci-robot merged commit dbd1925 into kubernetes:master Feb 7, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Feb 7, 2021
k8s-ci-robot added a commit that referenced this pull request Feb 7, 2021
…54-origin-release-1.19

Automated cherry pick of #10754: Use EnsureTask instead of prepending IG names to external
@rifelpet rifelpet deleted the fix-update-asg-elb branch May 5, 2021 13:45
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. area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", 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.

InstanceGroup name being prepended to externalLoadBalancer name
3 participants