Fix panic caused by no cloudprovider in test #64709
Conversation
k8s-ci-robot
added
sig/storage
k8s-ci-robot
added
cncf-cla: yes
| { | ||
| name: "when no cloudprovider is present", | ||
| cloudProviderName: "", | ||
| expectedVolumeKey: util.AzureVolumeLimitKey, |
There was a problem hiding this comment.
How is this specific key picked up when cloud provider is set to empty?
There was a problem hiding this comment.
We could have tested any cloudprovider. I just chose to test Azure.
There was a problem hiding this comment.
also there's a loop in setVolumeLimits that goes through all the plugins so we are just picking one to test here.
pkg/volume/aws_ebs/aws_ebs.go
Outdated
| // if we can't fetch cloudprovider we return a limit assuming | ||
| // kubelet could be running without cloudprovider. | ||
| if cloud == nil { | ||
| return volumeLimits, nil |
There was a problem hiding this comment.
In the hindsight, this may not be best idea. Not returning a value from here is probably better because then an external CCM or admin can override it.
We should not panic when no cloudprovider is present
gnufied
force-pushed
the
fix-node-alpha-tests
branch
from
648a754
to
32b6919
Compare
| // default values from here will mean, no one can | ||
| // override them. | ||
| if cloud == nil { | ||
| return nil, fmt.Errorf("No cloudprovider present") |
There was a problem hiding this comment.
this is not really an error right? would this work in say local-up-cluster? would be better to pass back map[string]int64{}, nil no?
There was a problem hiding this comment.
This is not really an error. The caller of this function will just use the returned error to determine that no limit has to be set. Caller could have also used nil value to ascertain that but for logging and messaging purposes, it is best to return an error.
There was a problem hiding this comment.
@gnufied i don't see a change for setVolumeLimits (from pkg/kubelet/kubelet_node_status.go) in this PR. That's the one that also needs to change right?
There was a problem hiding this comment.
never mind, we just continue looping around there in that method even when there is an error
There was a problem hiding this comment.
In what case, the cloud could be nil?
There was a problem hiding this comment.
almost any bare metal / single node / local-up-cluster.sh scenarios.
There was a problem hiding this comment.
( or even when --cloud-provider=external )
|
/milestone v1.11 |
k8s-ci-robot
added
the
priority/important-soon
k8s-ci-robot
added
the
lgtm
|
/sig storage |
|
@kubernetes/sig-node-pr-reviews @kubernetes/sig-storage-pr-reviews |
|
/kind bug |
k8s-github-robot
added
milestone/needs-approval
and removed
milestone/incomplete-labels
labels
|
/lgtm |
|
/lgtm |
|
/assign @yujuhong |
|
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process @dims @gnufied @msau42 @saad-ali @yujuhong Pull Request Labels
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, gnufied, msau42, saad-ali, yujuhong 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 |
k8s-ci-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 64344, 64709, 64717, 63631, 58647). If you want to cherry-pick this change to another branch, please follow the instructions here. |
We should not panic when no cloudprovider is present
Fixes #64704
Also added a test to cover the panic.
/sig storage
/sig node