Skip to content

Instantly share code, notes, and snippets.

@yuvalif

yuvalif/rook_pr8426_followup.md

Last active January 5, 2023 00:50
Show Gist options
  • Star 2 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save yuvalif/c4c7aed3dbc956a65399865a77c6a929 to your computer and use it in GitHub Desktop.
Save yuvalif/c4c7aed3dbc956a65399865a77c6a929 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
rook_pr8426_followup.md

Goal

This is list TODOs for the bucket notification PR in Rook. Based on these comments:

Priority 1

Refactoring and Unit Tests

  • code reuse with reused function (and unit test for above reused function)
  • use the regular S3 agent for notification provisioning (topic will still need the special SNS client)
  • move the OBC label predicate to WatchPredicateForNonCRDObject
  • add unit tests for notification and topic controllers
  • PR

Integration Tests

  • different order: OBC - Topic - Notification
  • different order: OBC - Notification - Topic
  • adding a label to an existing OBC
  • deleting a label from an existing OBC
  • PR

Enhanced CephBucketNotification Status

Notification Deletion

  • fetch the list of notifications from the RGW and do the diff with the labels instead of deleting all notifications on any label change. The process should be:
    • when the OBC label controller is invoked, we should first use the GetBucketNotification API to get all notifications fo the bucket (instead of deleting them).
    • then loop through the labels and add them (similarly to how the code currently works). note that adding a notification that is already there is not a problem, since the RGW API is idempotent. while going throuh the loop we should mark which of the notifications from the prevouse step id still in the list of labels
    • any existing notification which is not in the list of labels should be removed. note that we would need to add a new API in the s3ext.go file to remove a specific notification, since removing a specific notification of a bucket is not supported by the AWS SDK
    • PR

End2edn Tests

write end2end integration tests that actually verify sending the notifications to an HTTP endpoint pod. e.g use a python POST server, and verify the notifications with the logs from that pod:

wget https://gist.githubusercontent.com/mdonkers/63e115cc0c79b4f6b8b3a6b797e485c7/raw/a6a1d090ac8549dac8f2bd607bd64925de997d40/server.py
python server.py 10900

PR

Priority 2

  • implement bucket notification status with list of provisioned OBCs
  • (test) use events in integration tests to verify reconcile start/end
  • make the object store dependent with the topics (prevent object store dletion if it has topics)

note that dependency with the notifications is not needed, since the actual provisioned notifications are going to be deleted once the OBC is deleted and the object store is already dependent with the OBC. this requires further discussion

  • (test) verify that the existing mechanism for adding certificated into a cluster works with amqp/kafka endpoints (no need to use the CA file parameter to the topic, since the certificate will be in the default location)
  • support the metadata and tag filter extensions
  • prevent topic deletion if notifications has dependencies on it
  • implement cascade deletion of bucket notifications when the notification CR is deleted

notes:

  • we should prevent race conditions via some syncing mechanism:
    • mutex
    • channel
    • recocile events
  • notification deletion on the RGW is going to happen anyways on bucket deletion, this is why this is priority 2
  • implement cascade update of bucket notifications when the notification CR is updated
  • (test) topic update
  • (test) notification update and delete

Not in Scope

  • brownfield deployments
  • UI
  • Knative integration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment