default data cache enabled to false #2112
Conversation
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
cmd/gce-pd-csi-driver/main.go
Outdated
| @@ -382,7 +382,7 @@ func isDataCacheEnabledNodePool(ctx context.Context, nodeName string) (bool, err | |||
| dataCacheLSSDCount, err := driver.GetDataCacheCountFromNodeLabel(ctx, nodeName) | |||
| return dataCacheLSSDCount != 0, err | |||
| } | |||
| return true, nil | |||
| return false, nil | |||
There was a problem hiding this comment.
Should we also add *enableDataCacheFlag and len(nodeName) > 0 && nodeName != common.TestNode above?
This logic does not seem correct for gating component versions with the enableDataCacheFlag.
Should we have some unit tests for this coverage?
There was a problem hiding this comment.
Actually expanded the code above the flag check was ahead.
There was a problem hiding this comment.
Any other possibility that the nodeName maybe empty other than the bug that we already knew?
There was a problem hiding this comment.
Don't we want data cache to be enabled for E2E testing? I think there's 3 scenarios here:
- nodeName empty (false)
- nodeName is testnode (true)
- nodeName is set (check LSSD from label)
Also, would it be good to consolidate the enable/disable logic somewhere (eg: in a feature flags class), so that all codepaths are routed through the same enablement behavior? I'm thinking about the NodeUnstage case, where we aren't checking if this is a data cache enabled node.
There was a problem hiding this comment.
Moved the function to utils and added some unit test case. I have also added logic to perform this check during UnStageVolume
|
@Sneha-at: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Sneha-at
force-pushed
the
fix_data_cache_bug
branch
2 times, most recently
from
df290e7 to
bf80a6c
Compare
k8s-ci-robot
added
size/M
pkg/gce-pd-csi-driver/utils.go
Outdated
| if nodeName == common.TestNode { | ||
| return true, nil | ||
| } | ||
| if len(nodeName) > 0 { // disregard logic below when E2E testing. |
There was a problem hiding this comment.
Is this comment still applicable here? It seems like it should be moved up to line 347.
k8s-ci-robot
added
the
approved
Sneha-at
force-pushed
the
fix_data_cache_bug
branch
from
bf80a6c to
d6e553a
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pwschuurman, Sneha-at, sunnylovestiramisu 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 |
|
/cherry-pick release-1.20 |
|
@Sneha-at: new pull request created: #2135 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-1.17 |
|
@Sneha-at: new pull request created: #2143 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
We recently had an issue where data cache related watcher was running on non-data cache nodes as we were defaulting to
trueso updating the default values here tofalseWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: