Refactor PVC controller #662
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mergify
bot
requested review from
iPraveenParihar,
Madhu-1,
nixpanic,
Rakshith-R and
yati1998
|
|
Madhu-1
reviewed
Sep 5, 2024
Comment on lines
164
to
174
| if cap == relciamSpaceOp { | ||
| driverAnnotation = rsCSIAddonsDriverAnnotation | ||
| } | ||
|
|
||
| ok := r.supportsReclaimSpace(driver) | ||
| if reclaimSpaceSupportedByDriver && !ok { | ||
| logger.Info("Driver supports spacereclamation but driver is not registered in the connection pool, Reqeueing request", "DriverName", driver) | ||
| return true, false | ||
| if cap == keyRotationOp { | ||
| driverAnnotation = krCSIAddonsDriverAnnotation |
There was a problem hiding this comment.
Move this to switch case
var driverAnnotation string
switch cap {
case relciamSpaceOp:
driverAnnotation = rsCSIAddonsDriverAnnotation
case keyRotationOp:
driverAnnotation = krCSIAddonsDriverAnnotation
default:
logger.Info(fmt.Sprintf("Invalid capability: %s", cap))
return false, false
}| logger *logr.Logger, | ||
| annotations map[string]string, | ||
| driverName string, | ||
| cap string) (bool, bool) { |
There was a problem hiding this comment.
define a type for cap and pass it
// Define a custom type for capabilities
type Capability string
// Define constants for the different capabilities
const (
ReclaimSpaceOp Capability = "ReclaimSpace"
KeyRotationOp Capability = "KeyRotation"
)| RetryDeadlineSeconds: defaultRetryDeadlineSeconds, | ||
|
|
||
| switch jobtype { | ||
| case relciamSpaceOp: |
internal/controller/util/common.go
Outdated
| limitations under the License. | ||
| */ | ||
|
|
||
| package util |
There was a problem hiding this comment.
i think we dont need a helper file for it, this are common to PVC controller and not used outside by others, add this as functions to PVC controller only
black-dragon74
force-pushed
the
refrac-pvc-ctrl
branch
from
14d9cd9
to
40e9d2f
Compare
Madhu-1
reviewed
Sep 9, 2024
Rakshith-R
reviewed
Sep 10, 2024
There was a problem hiding this comment.
Left 2 comments.
@iPraveenParihar can you check the indexer and eventhandler parts.
Comment on lines
251
to
259
| if annotationKey == krcJobScheduleTimeAnnotation { | ||
| requeue, keyRotationSupported := r.checkDriverSupportsEncryptionKeyRotate(logger, ns.Annotations, driverName) | ||
| requeue, keyRotationSupported := r.checkDriverSupportCapability(logger, ns.Annotations, driverName, keyRotationOp) | ||
| if keyRotationSupported { | ||
| return schedule, nil | ||
| } | ||
| if requeue { | ||
| return "", ErrConnNotFoundRequeueNeeded | ||
| } | ||
| } else if annotationKey == rsCronJobScheduleTimeAnnotation { |
There was a problem hiding this comment.
Can you use switch case for annotationKey ?
| @@ -521,35 +492,63 @@ func getScheduleFromAnnotation( | |||
| return schedule, true | |||
| } | |||
|
|
|||
| // constructRSCronJob constructs and returns ReclaimSpaceCronJob. | |||
| func constructRSCronJob(name, namespace, schedule, pvcName string) *csiaddonsv1alpha1.ReclaimSpaceCronJob { | |||
| func constructCronJob(jobtype Operation, name, namespace, schedule, pvcName string) client.Object { | |||
There was a problem hiding this comment.
Please write two different contruct[KR|RS]CronJob functions.
It's better to avoid unsafe typecasting.
black-dragon74
force-pushed
the
refrac-pvc-ctrl
branch
2 times, most recently
from
90d0946
to
a3c90ba
Compare
Rakshith-R
reviewed
Sep 11, 2024
black-dragon74
force-pushed
the
refrac-pvc-ctrl
branch
from
a3c90ba
to
b43a913
Compare
| case keyRotationOp: | ||
| driverAnnotation = krCSIAddonsDriverAnnotation | ||
| default: | ||
| logger.Info("Unknown capability", "Capability", cap) |
There was a problem hiding this comment.
should it be Unknown operation?
There was a problem hiding this comment.
We check for capability inside this function. The reason constants are called Operation and not Capability is to avoid ambiguity with CSI's Capability struct.
TestingLogs (annotating PVC)Note: The relcaim-space error is related to Logs (annotating SC) |
I have updated the PR with logs. |
I’ll be updating above comment with logs for other scenarios shortly. |
black-dragon74
requested review from
Madhu-1,
iPraveenParihar and
Rakshith-R
Madhu-1
previously approved these changes
Sep 12, 2024
|
@Rakshith-R PTAL at this one again when you can? |
Rakshith-R
reviewed
Sep 13, 2024
Comment on lines
+196
to
+198
| if capFound { | ||
| return false, true | ||
| } |
There was a problem hiding this comment.
you don't need to check capFound in below lines.
There was a problem hiding this comment.
Done. Here's a diff of the latest change for ease of review:
- if driverSupportsCap && !capFound {
- logger.Info(fmt.Sprintf("Driver supports %s but driver is not registered in the connection pool, Reqeueing request", cap), "DriverName", driverName)
+ // If the driver supports the capability but the capability is not found in connection pool,
+ if driverSupportsCap {
+ logger.Info(fmt.Sprintf("Driver supports %s but driver is not registered in the connection pool, Requeuing request", cap), "DriverName", driverName)
return true, false
}
- if !capFound {
- logger.Info(fmt.Sprintf("Driver does not support %s, skip Requeue", cap), "DriverName", driverName)
- return false, false
- }
-
- // Do not reque, cap is supported
- return false, true
-
+ // If the driver does not support the capability, skip requeue
+ logger.Info(fmt.Sprintf("Driver does not support %s, skip Requeue", cap), "DriverName", driverName)
+ return false, false
}
black-dragon74
force-pushed
the
refrac-pvc-ctrl
branch
from
b43a913
to
2507163
Compare
mergify
bot
dismissed
Madhu-1’s stale review
Pull request has been modified.
Rakshith-R
approved these changes
Sep 13, 2024
Madhu-1
approved these changes
Sep 13, 2024
This PR refactors the PVC controller to reduce code duplication by extracting common logic into its own functions/packages Signed-off-by: Niraj Yadav <[email protected]>
black-dragon74
force-pushed
the
refrac-pvc-ctrl
branch
from
2507163
to
d58422b
Compare
lumiere-bot bot
referenced
this pull request
in coolguy1771/home-ops
Sep 19, 2024
… ) (#5423) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [kubernetes-csi-addons](https://redirect.github.com/csi-addons/kubernetes-csi-addons) | minor | `v0.9.1` -> `v0.10.0` | --- ### Release Notes <details> <summary>csi-addons/kubernetes-csi-addons (kubernetes-csi-addons)</summary> ### [`v0.10.0`](https://redirect.github.com/csi-addons/kubernetes-csi-addons/releases/tag/v0.10.0) [Compare Source](https://redirect.github.com/csi-addons/kubernetes-csi-addons/compare/v0.9.1...v0.10.0) #### What's Changed - Add proto and sidecar code for volumegroup by [@​Nikhil-Ladha](https://redirect.github.com/Nikhil-Ladha) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/652](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/652) - vendor: Bump google.golang.org/grpc from 1.65.0 to 1.66.0 in the golang-dependencies group by [@​dependabot](https://redirect.github.com/dependabot) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/656](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/656) - vendor: Bump sigs.k8s.io/controller-tools from 0.16.1 to 0.16.2 in /tools in the k8s-dependencies group by [@​dependabot](https://redirect.github.com/dependabot) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/658](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/658) - vendor: Bump the github-dependencies group across 1 directory with 3 updates by [@​dependabot](https://redirect.github.com/dependabot) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/657](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/657) - replication: add new Validated condition by [@​Rakshith-R](https://redirect.github.com/Rakshith-R) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/664](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/664) - bundle: Update CSV capability to Seamless Upgrades by [@​black-dragon74](https://redirect.github.com/black-dragon74) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/668](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/668) - Refactor PVC controller by [@​black-dragon74](https://redirect.github.com/black-dragon74) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/662](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/662) - bundle: Add missing CRDs to bundle CSV by [@​black-dragon74](https://redirect.github.com/black-dragon74) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/669](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/669) - vendor: bump the k8s-dependencies group with 3 updates by [@​dependabot](https://redirect.github.com/dependabot) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/673](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/673) #### New Contributors - [@​Nikhil-Ladha](https://redirect.github.com/Nikhil-Ladha) made their first contribution in [https://github.com/csi-addons/kubernetes-csi-addons/pull/652](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/652) **Full Changelog**: csi-addons/kubernetes-csi-addons@v0.9.1...v0.10.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://redirect.github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44NS4wIiwidXBkYXRlZEluVmVyIjoiMzguODUuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUvZ2l0aHViLXJlbGVhc2UiLCJ0eXBlL21pbm9yIl19--> Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
spiceratops
referenced
this pull request
in spiceratops/k8s-gitops
Sep 26, 2024
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [kubernetes-csi-addons](https://redirect.github.com/csi-addons/kubernetes-csi-addons) | minor | `v0.9.1` -> `v0.10.0` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>csi-addons/kubernetes-csi-addons (kubernetes-csi-addons)</summary> ### [`v0.10.0`](https://redirect.github.com/csi-addons/kubernetes-csi-addons/releases/tag/v0.10.0) [Compare Source](https://redirect.github.com/csi-addons/kubernetes-csi-addons/compare/v0.9.1...v0.10.0) #### What's Changed - Add proto and sidecar code for volumegroup by [@​Nikhil-Ladha](https://redirect.github.com/Nikhil-Ladha) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/652](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/652) - vendor: Bump google.golang.org/grpc from 1.65.0 to 1.66.0 in the golang-dependencies group by [@​dependabot](https://redirect.github.com/dependabot) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/656](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/656) - vendor: Bump sigs.k8s.io/controller-tools from 0.16.1 to 0.16.2 in /tools in the k8s-dependencies group by [@​dependabot](https://redirect.github.com/dependabot) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/658](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/658) - vendor: Bump the github-dependencies group across 1 directory with 3 updates by [@​dependabot](https://redirect.github.com/dependabot) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/657](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/657) - replication: add new Validated condition by [@​Rakshith-R](https://redirect.github.com/Rakshith-R) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/664](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/664) - bundle: Update CSV capability to Seamless Upgrades by [@​black-dragon74](https://redirect.github.com/black-dragon74) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/668](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/668) - Refactor PVC controller by [@​black-dragon74](https://redirect.github.com/black-dragon74) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/662](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/662) - bundle: Add missing CRDs to bundle CSV by [@​black-dragon74](https://redirect.github.com/black-dragon74) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/669](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/669) - vendor: bump the k8s-dependencies group with 3 updates by [@​dependabot](https://redirect.github.com/dependabot) in [https://github.com/csi-addons/kubernetes-csi-addons/pull/673](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/673) #### New Contributors - [@​Nikhil-Ladha](https://redirect.github.com/Nikhil-Ladha) made their first contribution in [https://github.com/csi-addons/kubernetes-csi-addons/pull/652](https://redirect.github.com/csi-addons/kubernetes-csi-addons/pull/652) **Full Changelog**: csi-addons/kubernetes-csi-addons@v0.9.1...v0.10.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://redirect.github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44NS4wIiwidXBkYXRlZEluVmVyIjoiMzguODUuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUvZ2l0aHViLXJlbGVhc2UiLCJ0eXBlL21pbm9yIl19-->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the PVC controller to reduce
code duplication by extracting common logic
into its own functions/packages