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.

Sign up for GitHub

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

Jump to bottom

Don't recreate lb cloud resources on kcm restart #29082

Merged
merged 1 commit into from
Jul 18, 2016
Merged

Don't recreate lb cloud resources on kcm restart #29082

merged 1 commit into from
Jul 18, 2016

Conversation

bprashanth
Copy link
Contributor

Absolute dumbest way to fix it right now.
Fixes #29079

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 18, 2016
j3ffml
j3ffml reviewed Jul 18, 2016
View reviewed changes
pkg/cloudprovider/providers/gce/gce.go
@@ -760,20 +766,29 @@ func (gce *GCECloud) forwardingRuleNeedsUpdate(name, region string, loadBalancer
if isHTTPErrorCode(err, http.StatusNotFound) {
return false, true, "", nil
}
return false, false, "", fmt.Errorf("error getting load balancer's forwarding rule: %v", err)
// Err on the side of caution in case of errors. Caller should notice the error and retry.
// We never want to end up recreting resources because gce api flaked.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/recreting/recreating

j3ffml
j3ffml reviewed Jul 18, 2016
View reviewed changes
pkg/cloudprovider/providers/gce/gce.go
}
if translateAffinityType(affinityType) != tp.SessionAffinity {
// TODO: This is fickle, if either side changes defaults it could backfire.
if tp.SessionAffinity != "" && translateAffinityType(affinityType) != tp.SessionAffinity {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this one. I think SessionAffinity == "" might be equivalent to SessionAffinity NONE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

safety first? this will just prevent the resync for session affinity. or would you prefer treating empty as none?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, can we get clarity on that and document it here?

Copy link
Member

Choose a reason for hiding this comment

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

Can you comnment this REALLY REALLY well, please? Explain what we're checking and why.

@j3ffml
Copy link
Contributor

j3ffml commented Jul 18, 2016

lgtm except the SessionAffinity question.

thockin
thockin reviewed Jul 18, 2016
View reviewed changes
pkg/cloudprovider/providers/gce/gce.go
// TODO: we report loadbalancer IP through status, so we want to verify if
// that matches the forwarding rule as well.
if loadBalancerIP != "" && loadBalancerIP != fwd.IPAddress {
glog.Warningf("Loadbalancer ip for forwarding rule %v changed from %v to %v", fwd.Name, fwd.IPAddress, loadBalancerIP)
Copy link
Member

Choose a reason for hiding this comment

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

"changed" is misleading. maybe "was expected to be %v, but was actually %v"

@thockin
Copy link
Member

thockin commented Jul 18, 2016

My comments are all log messages or comments, so you can LGTM when done.

@thockin thockin added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 18, 2016
@thockin thockin added this to the v1.3 milestone Jul 18, 2016
@bprashanth
Copy link
Contributor Author

Comments addressed, applying LGTM as discussed.

My "emptystring is not None" assertion also means that users won't be able to move out of "None" for session affinity as long the gce api keeps returning the empty string.

@bprashanth bprashanth added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2016
@bprashanth bprashanth mentioned this pull request Jul 18, 2016
Merged
@thockin
Copy link
Member

thockin commented Jul 18, 2016

Did we confirm experimentally what GCE returns for that field? If we feed it NONE what do we get back?

@thockin
Copy link
Member

thockin commented Jul 18, 2016

LGTM. The TP thing skeeves me out.

@bprashanth
Copy link
Contributor Author

My UI has

Session affinity

None

But even clickin on "show as json" I don't get the field.
I think we do give it the right value (from looking at code code) because -o yaml on the service shows sessionAffinity: None and we pipe it through: https://github.com/kubernetes/contrib/blob/28813aeb2cd78d6fc2425261f0031d2e0d049da9/cluster-autoscaler/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/gce/gce.go#L887

@bprashanth bprashanth added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Jul 18, 2016
@bprashanth
Copy link
Contributor Author

not sure what the build failures about, rebased and trying again

@bprashanth bprashanth added lgtm "Looks good to me", indicates that a PR is ready to be merged. cherrypick-candidate and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. cherrypick-candidate labels Jul 18, 2016
@k8s-bot
Copy link

k8s-bot commented Jul 18, 2016

GCE e2e build/test passed for commit a9426a1.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 1e1d4b0 into kubernetes:master Jul 18, 2016
k8s-github-robot pushed a commit that referenced this pull request Jul 18, 2016
Merge pull request #29083 from bprashanth/automated-cherry-pick-of-#2…
a75a942 
…9082-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of #29082

Cherry pick of #29082 on release-1.3.
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.3" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@roberthbailey roberthbailey added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 21, 2016
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#29083 from bprashanth/automated-cherry-…
1f5e4e7 
…pick-of-#29082-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of kubernetes#29082

Cherry pick of kubernetes#29082 on release-1.3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@j3ffml j3ffml

@thockin thockin

@matchstick matchstick

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

Don't recreate forwarding rule/target pool on master restart
9 participants
@bprashanth @j3ffml @thockin @k8s-bot @k8s-github-robot @k8s-cherrypick-bot @googlebot @roberthbailey @matchstick