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.

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

object: add check specific to name and namespace for ceph cosi driver #13623

Merged
merged 1 commit into from Feb 5, 2024

Conversation

thotz
Copy link
Contributor

@thotz thotz commented Jan 25, 2024

The name for cephcosi driver should be ceph-cosi-driver and namespace should same as the rook operator. Otherwise request need to be failed.

Fixes: #13025

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

// Set the default CephCOSIDriver name if not already set
if cephCOSIDriver.Name == "" {
cephCOSIDriver.Name = CephCOSIDriverName
// The ceph-cosi-driver CRD must have the name "ceph-cosi-driver"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This seems like an unnecessary limitation when the operator can just know to query for a single CephCOSIDrivers in the operator namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed accordingly

@thotz thotz force-pushed the ceph-cosi-driver-name-namespace-check branch from f4108c9 to 613b5a1 Compare January 29, 2024 16:30
@thotz thotz requested a review from BlaineEXE January 29, 2024 16:30
@thotz thotz force-pushed the ceph-cosi-driver-name-namespace-check branch from 9b0eecf to 6d57324 Compare January 30, 2024 10:23
@thotz thotz requested a review from travisn January 30, 2024 10:23
Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix golint errors.

The name for cephcosi driver should be ceph-cosi-driver and namespace
should same as the rook operator. Otherwise request need to be failed.

Fixes: rook#13025
Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
@thotz thotz force-pushed the ceph-cosi-driver-name-namespace-check branch from 6d57324 to 5b5e1c0 Compare January 31, 2024 06:02
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good. Since there aren't integration tests for these error cases of more than one cosi driver, could you comment if it's tested manually in minikube?

@thotz
Copy link
Contributor Author

thotz commented Feb 5, 2024

Test on my minikube setup

kubectl create -f /tmp/cephcosidriver1.yaml
cephcosidriver.ceph.rook.io/ceph-cosi-driver-1 created
jiffin@fedora:~/rook$ kubectl -n rook-ceph get pods
NAME                                           READY   STATUS      RESTARTS        AGE
ceph-cosi-driver-6d78bcc5c9-tf725              2/2     Running     4 (2d3h ago)    2d4h
csi-cephfsplugin-pl49j                         2/2     Running     14 (2d3h ago)   3d2h
csi-cephfsplugin-provisioner-bfbd6db5b-5pjpc   5/5     Running     31 (2d3h ago)   3d2h
csi-rbdplugin-b4rp5                            2/2     Running     14 (2d3h ago)   3d2h
csi-rbdplugin-provisioner-5d46874df5-bs8jt     5/5     Running     30 (2d3h ago)   3d2h
rook-ceph-exporter-minikube-59c966d65d-7bk9h   1/1     Running     0               28s
rook-ceph-mgr-a-7469f65cfb-4sgbs               1/1     Running     6 (2d3h ago)    3d2h
rook-ceph-mon-a-75fdbc7bb-ccjwz                1/1     Running     6 (2d3h ago)    3d2h
rook-ceph-operator-577fcddf8-k576w             1/1     Running     0               34s
rook-ceph-osd-0-6d459f5699-rr4b4               1/1     Running     5 (2d3h ago)    3d2h
rook-ceph-osd-1-5fd4d9b498-fzwgm               1/1     Running     5 (2d3h ago)    3d2h
rook-ceph-osd-prepare-minikube-4lssr           0/1     Completed   0               32m
rook-ceph-rgw-my-store-a-5569f94b6c-5fpgl      1/1     Running     4 (2d3h ago)    2d22h

logs:

37.461135 I | cephclient: getting or creating ceph auth key "mgr.a"
2024-02-05 11:31:37.942201 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:37.945422 I | op-mgr: deployment for mgr rook-ceph-mgr-a already exists. updating if needed
2024-02-05 11:31:37.947639 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:37.954993 I | op-k8sutil: updating deployment "rook-ceph-mgr-a" after verifying it is safe to stop
2024-02-05 11:31:37.955009 I | op-mon: checking if we can stop the deployment rook-ceph-mgr-a
2024-02-05 11:31:37.958478 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:37.979162 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:38.019865 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:38.100802 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:38.261101 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:38.582116 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:39.224455 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:40.506253 E | ceph-cosi-controller: failed to reconcile CephCOSIDriver "rook-ceph/ceph-cosi-driver-1". more than one instance of CephCOSIDriver found
2024-02-05 11:31:41.400010 I | op-k8sutil: finished waiting for updated deployment "rook-ceph-mgr-a"

Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good. Thanks for including testing output and adding unit tests for the error cases.

@travisn travisn merged commit 7f7e8d4 into rook:master Feb 5, 2024
50 checks passed
travisn added a commit that referenced this pull request Feb 6, 2024
object: add check specific to name and namespace for ceph cosi driver (backport #13623)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more checks related to cephcosidriver CRD in cosi-controller
3 participants