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

Driver Controller: Ensure CSI Config Creation #104

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

nb-ohad
Copy link
Collaborator

@nb-ohad nb-ohad commented Aug 6, 2024

Describe what this PR does

If a driver CR exists but no client profile CR exists, any container that needs to mount the ceph-csi-config config map will fail. To solve the issue, the following behavior is added:

  • Driver controller creates an empty ceph-csi-config config map if not already exist and adds an owner ref
  • Driver controller watch for delete events for ceph-csi-config config map

@nb-ohad nb-ohad requested a review from Madhu-1 August 6, 2024 22:41
@nb-ohad
Copy link
Collaborator Author

nb-ohad commented Aug 6, 2024

/hold

This PR is rebased on top of #103 and should not be merged until #103 is merged

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Aug 7, 2024

  • Driver controller creates an empty ceph-csi-config config map if not already exist and adds an owner ref
  • Driver controller watch for delete events for ceph-csi-config config map

Instead of watching for delete events can't we add the driver as the ownerRef if the configmap already exists and we don't need to consider delete events?

nb-ohad added 2 commits August 7, 2024 10:39
Signed-off-by: nb-ohad <mitrani.ohad@gmail.com>
Signed-off-by: nb-ohad <mitrani.ohad@gmail.com>
@nb-ohad nb-ohad force-pushed the csi-config-creation-in-driver branch from f568c90 to 71956cb Compare August 7, 2024 07:40
@nb-ohad
Copy link
Collaborator Author

nb-ohad commented Aug 7, 2024

/unhold

#103 merged, @Madhu-1 can you please add this to your review queue?

@nb-ohad
Copy link
Collaborator Author

nb-ohad commented Aug 7, 2024

  • Driver controller creates an empty ceph-csi-config config map if not already exist and adds an owner ref
  • Driver controller watch for delete events for ceph-csi-config config map

Instead of watching for delete events can't we add the driver as the ownerRef if the configmap already exists and we don't need to consider delete events?

We are already adding the driver as an owner ref. But there is still a need to watch for delete events for the case where the configmap goes away and need to be recreated. The delete filter makes sure that we are not queuing an based on create or update events

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Aug 7, 2024

  • Driver controller creates an empty ceph-csi-config config map if not already exist and adds an owner ref
  • Driver controller watch for delete events for ceph-csi-config config map

Instead of watching for delete events can't we add the driver as the ownerRef if the configmap already exists and we don't need to consider delete events?

We are already adding the driver as an owner ref. But there is still a need to watch for delete events for the case where the configmap goes away and need to be recreated. The delete filter makes sure that we are not queuing an based on create or update events

Sorry am failing to understand it as we are adding the owner ref if the configmap already exists how it will go away if the driver is not deleted? Do you mean the use will manually remove the configmap by hand?

@nb-ohad
Copy link
Collaborator Author

nb-ohad commented Aug 7, 2024

Yes.
It can be faulty code in external automation, or some bug in rook or client, or a mistake by an admin, or someone just overwrote the entire ownerRef section instead of adding to it.

If a controller takes the responsibility to move a system toward a certain actual state it also needs to take the responsibility to readjust if that actual state has changed (for any reason)

@Madhu-1 Madhu-1 merged commit f23d80f into ceph:main Aug 7, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants