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

Avoid panic in Cloud CIDR Allocator #58186

Merged
merged 1 commit into from
Jan 13, 2018
Merged

Avoid panic in Cloud CIDR Allocator #58186

merged 1 commit into from
Jan 13, 2018

Conversation

negz
Copy link
Contributor

@negz negz commented Jan 12, 2018

What this PR does / why we need it:
I suspect a race exists where we attempt to look up the CIDR for a terminating node. By the time updateCIDRAllocation is called the node has disappeared. We determine it does not have a cloud CIDR (i.e. Alias IP Range) and attempt to record a CIDRNotAvailable node status. Unfortunately we reference node.Name while node is still nil.

By getting the node before looking up the cloud CIDR we avoid the nil pointer dereference, and potentially fail fast in the case the node has disappeared.

Which issue(s) this PR fixes:
Fixes #58181

Release note:

Avoid panic when failing to allocate a Cloud CIDR (aka GCE Alias IP Range).

Get the node before attempting to get its Alias IP ranges
c7988ba 
This allows us to fail fast if the node doesn't exist, and to record node status
changes if we fail to 'allocate' a CIDR.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 12, 2018
@negz
Copy link
Contributor Author

negz commented Jan 12, 2018

/assign @shyamjvs

@dixudx
Copy link
Member

dixudx commented Jan 12, 2018

/ok-to-test
/lgtm
@negz Nice fix. Great.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 12, 2018
@negz
Copy link
Contributor Author

negz commented Jan 12, 2018

/retest

1 similar comment
@negz
Copy link
Contributor Author

negz commented Jan 12, 2018

/retest

@negz
Copy link
Contributor Author

negz commented Jan 13, 2018

/test pull-kubernetes-cross

@freehan
Copy link
Contributor

freehan commented Jan 13, 2018

/lgtm
/approve

@bowei
Copy link
Member

bowei commented Jan 13, 2018

/lgtm /approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, dixudx, freehan, negz

Associated issue: #58181

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2018
@negz
Copy link
Contributor Author

negz commented Jan 13, 2018

/test pull-kubernetes-cross

Pretty sure this failure is unrelated to my PR.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 57266, 58187, 58186, 46245, 56509). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 15ef3a8 into kubernetes:master Jan 13, 2018
@msau42
Copy link
Member

msau42 commented Jan 20, 2018

Let's cherry pick this to 1.9

@saad-ali saad-ali added this to the v1.9 milestone Jan 20, 2018
jingax10 added a commit to jingax10/kubernetes that referenced this pull request Jan 20, 2018
@jingax10
Initialize node ahead in case we need to refer to it in error cases. …
1cd74c2 
…This is a backport of kubernetes#58186. We cannot intact backport to it due to a refactor PR kubernetes#56352.
@jingax10 jingax10 mentioned this pull request Jan 20, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request Jan 23, 2018
Merge pull request #58557 from jingax10/release-1.9
7fd1d55 
Automatic merge from submit-queue.

Initialize node ahead in case we need to refer to it in error cases

Initialize node ahead in case we need to refer to it in error cases. This is a backport of #58186. We cannot intact backport to it due to a refactor PR #56352.



**What this PR does / why we need it**:

We want to cherry pick to 1.9. Master already has the fix.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #58181

**Special notes for your reviewer**:

**Release note**:

```release-note
Avoid controller-manager to crash when enabling IP alias for K8s cluster.
```
@jingax10 jingax10 mentioned this pull request Jan 31, 2018
Closed
@k8s-cherrypick-bot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bowei bowei

@freehan freehan

@shyamjvs shyamjvs

@dixudx dixudx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.

Panic in Cloud CIDR Allocator
10 participants
@negz @dixudx @freehan @bowei @k8s-ci-robot @k8s-github-robot @msau42 @k8s-cherrypick-bot @shyamjvs @saad-ali