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

Implement Driver API reconciliation #24

Merged

Madhu-1 merged 14 commits into ceph:main from nb-ohad:driver-reconcile

Jul 26, 2024

Collaborator

nb-ohad commented Jun 26, 2024

Describe what this PR does

The PR implements driver API reconciliation.

Read and validate the desired state specified by a driver CR
Merges desired state with defaults specified in an optional Operator Config CR and with local hardcoded values
Loads a custom image set or defaults to upstream CSI images
Concurrently Create or update the following resources (per driver)
- Ceph CSI Plugin Deamon Set (and sidecars)
- Ceph CSI Provisioner deployment (and sidecars)
- CSI Driver definition
- Optional Livness k8s Service

Is there anything that requires special attention

This PR does not include tests that cover the above changes.
A second PR for test implementation will come later

Future concerns

List items that are not part of the PR and do not impact it's
functionality, but are work items that can be taken up subsequently.

nb-ohad force-pushed the driver-reconcile branch from aa1fe45 to d935b17 Compare

June 27, 2024 07:30

leelavg reviewed

View reviewed changes

internal/controller/defaults.go Show resolved Hide resolved

nb-ohad force-pushed the driver-reconcile branch 3 times, most recently from beda133 to 1edde70 Compare

June 27, 2024 11:29

raaizik reviewed

View reviewed changes

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

Madhu-1 requested changes

View reviewed changes

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

subhamkrai reviewed

View reviewed changes

go.mod Show resolved Hide resolved

nb-ohad force-pushed the driver-reconcile branch 6 times, most recently from dd95cb8 to 2832a7e Compare

July 11, 2024 11:17

leelavg reviewed

View reviewed changes

Contributor

leelavg left a comment

Reviewed only a 3rd portion I guess, will do the rest in another round, pls excuse.

api/v1alpha1/driver_types.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Show resolved Hide resolved

internal/controller/defaults.go Show resolved Hide resolved

internal/controller/defaults.go Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Show resolved Hide resolved

internal/controller/driver_controller.go Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

parth-gr suggested changes

View reviewed changes

internal/controller/driver_controller.go Show resolved Hide resolved

nb-ohad force-pushed the driver-reconcile branch 7 times, most recently from f5a0b45 to badc03f Compare

July 15, 2024 19:22

leelavg reviewed

View reviewed changes

Contributor

leelavg left a comment

reviewed around 2/3 rd, will do the rest after the updates.

internal/controller/defaults.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/utils/core.go Outdated Show resolved Hide resolved

internal/utils/core.go Outdated Show resolved Hide resolved

internal/utils/core.go Show resolved Hide resolved

rewantsoni suggested changes

View reviewed changes

internal/controller/defaults.go Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

internal/controller/driver_controller.go Outdated Show resolved Hide resolved

nb-ohad force-pushed the driver-reconcile branch from badc03f to 1357d08 Compare

July 17, 2024 14:36

nb-ohad marked this pull request as ready for review

July 17, 2024 14:48

nb-ohad force-pushed the driver-reconcile branch 3 times, most recently from cf2b0f6 to 07100fd Compare

July 17, 2024 19:20

nb-ohad force-pushed the driver-reconcile branch 2 times, most recently from b8523e8 to ec54f4d Compare

July 21, 2024 22:39

nb-ohad added 14 commits

July 22, 2024 10:59


          Driver Controller: Validate, load and parse driver resource

e4e9d91

- validate driver name structure
- parse and split driver name into short name and driver type
- load desired driver resource

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: Load driver defaults (from operator config) and me…

2ed066b

…rge them

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: Handle image set loading and merging

bfb1896

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: Scaffolding for concurrent reconcile

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: Reconcile k8s CSIDriver resource

b535f85

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: Reconcile Controller Plugin k8s Deployment

feb5e28

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: Reconcile Node Plugin k8s DaemonSet

4e2ef89

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: Reconcile liveness service

89e1fd0

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: CSI Service Account and RBAC

a05ac60

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: Inline function invocation readability enhancements

19d1942

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: Align with standard library utilities

e0fb6d7

Signed-off-by: nb-ohad <[email protected]>


          Driver Controller: remove unused reutrn value to solve a lint issue

1cc4862

Signed-off-by: nb-ohad <[email protected]>


          Docker File: Bump golang, and missing packages

ace4d8b

Signed-off-by: nb-ohad <[email protected]>


          Operator Configuration: Personalize operator's pod selector

badf2d6

Signed-off-by: nb-ohad <[email protected]>

nb-ohad force-pushed the driver-reconcile branch from ec54f4d to badf2d6 Compare

July 22, 2024 07:59

nb-ohad mentioned this pull request

"Config" in the CRD name is redundant IMO. How about a name without "Config"? #8

Closed

Contributor

parth-gr commented Jul 24, 2024

While installing node plugin pods I see

  Normal   Scheduled    65s               default-scheduler  Successfully assigned ceph-csi-operator-system/rook-ceph.rbd.csi.ceph.com-nodeplugin-jgbc2 to minikube
  Warning  FailedMount  1s (x8 over 65s)  kubelet            MountVolume.SetUp failed for volume "registration-dir" : hostPath type check failed: /plugins_registry is not a directory
  Warning  FailedMount  1s (x8 over 65s)  kubelet            MountVolume.SetUp failed for volume "pods-mount-dir" : hostPath type check failed: /pods is not a directory

This was referenced Jul 26, 2024

can we log the error so that if it fails we will know whats happening? #52

Open

construct the driver name and add it to the err what is not matching, helps in debugging #53

Closed

add drivername to the error #54

Closed

Use update for kubernetes csidriver object when required #55

Closed

the caller should handle this as error or panic if anything else is passed #56

Closed

This will not work for older images because this flag needs to be set only if volumegroupsnapshot CRDs are present and if the sidecar image supports this flag. #57

Closed

why a deepcopy here? afaik, client.Get fills in the memory that we supply and this is already a copy. #58

Closed

Madhu-1 approved these changes

View reviewed changes

Madhu-1 merged commit 788f7c2 into ceph:main

7 checks passed

Madhu-1 pushed a commit to Madhu-1/ceph-csi-operator that referenced this pull request


          Merge pull request ceph#24 from iamniting/prefix

40292ab

bundle: rename the prefix from ocscsi to ceph-csi

leelavg mentioned this pull request

remove extra deep copy of object from cache #155

Merged

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

Reviewers

raaizik raaizik left review comments

parth-gr parth-gr requested changes

leelavg leelavg left review comments

subhamkrai subhamkrai left review comments

rewantsoni rewantsoni requested changes

Madhu-1 Madhu-1 approved these changes

Labels

None yet