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.

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

Bucket Notifcations #1467

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Bucket Notifcations #1467

merged 1 commit into from
Nov 19, 2024

Conversation

alphaprinz
Copy link
Contributor

-New field notificationsPVC in noobaa that is mounted to endpoints and core
-Secrets containing info on how to connect to remote server are mounted to core

Explain the changes

  1. Implement a way to specify a pvc to use for notification files.
  2. Implement a way to specify secrets with information of how to connect to external notification target.

Testing Instructions:

  1. Get a pvc.
  2. Specify pvc name in notificationsPVC in noobaa crd under spec.
  3. Create a secret from the connect file, label it with app=noobaa and noobaa=notifications
  • Doc added/updated
  • Tests added

@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 31, 2024
jackyalbo
jackyalbo reviewed Nov 10, 2024
View reviewed changes
pkg/system/phase2_creating.go Outdated
//mount them so core can read them
notificatoinSecrets := &corev1.SecretList{}
noobaaNotifSelector, _ := labels.Parse("app=noobaa,noobaa=notifications")
util.KubeList(notificatoinSecrets, &client.ListOptions{Namespace: options.Namespace, LabelSelector: noobaaNotifSelector})
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we don't find any secret? Do we have a default? we should reject? warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using default doesn't make sense, because user at least needs to specify url for remote server and its protocol (http/kafka).
I can look into going into reject, but that can be done in operator only if there are no secrets at all. It would still be possible to be missing some of the secrets and operator won't have a way to know that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should give the user as much info as we can. If we don't have any secrets and we know the feature won't work this way - we should at least write a warning in the logs (I think we should also Reject)
It can be nice to also check that the secret has the needed fields we expect and maybe add some documentation about how the secret is supposed to look.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the user should give us the specific secret that he wants us to use and we won't just list all the secrets? WDYT?

pkg/system/phase2_creating.go Outdated
@@ -450,6 +450,16 @@ func (r *Reconciler) setDesiredCoreEnv(c *corev1.Container) {
}
}
}

if r.NooBaa.Spec.NotificationsPVC != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should support something similar to what we have for bucket logging. We should create a pvc in case we are in ODF. Also, don't think we should have two if any one of them is turned on. I think we should have a segment like bucketNotifications and inside we should have enabled, yes or no and an optional pvc in case bucket logging is not enabled and we are not in ODF.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jacky. we should probably consolidate the PVCs, and handle it in a similar way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-Regarding PVC in ODF - OK, I will copy bucket logging notification behavior in ODF case.
-Regarding single PVC - are you sure using a single PVC is better, performance-wise or user-experience-wise? If so, should I rename it (in our code) BucketLoggingPVC? Sth like "PersistentLogsPVC"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For using a single PVC for both bucket logging and bucket notifications, the change that makes most sense to me is that we remove pvc name from both of them and add a new field in noobaa crd for a "persistent files pvc", which both will use. Otherwise the features parameters are mixed.
Let me know what you think or if you have a different structure in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to me as well, PersistentLogsPVC sounds like a good name - just need a better explanation in the noobaa CR about the use of this PVC for both guaranteed logging and bucket notifications.

dannyzaken
dannyzaken reviewed Nov 13, 2024
View reviewed changes
pkg/apis/noobaa/v1alpha1/noobaa_types.go Outdated
// NotificationsPVC (optional) specifies the name of the Persistent Volume Claim (PVC) to be used
// for notifications persistent files.
// +optional
NotificationsPVC *string `json:"notificationsPVC,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Following a discussion with Amit and Jacky:

  • Add BucketNotificationsSpec that contains enable, optionl pvc, array of secrets.
  • we will use separate PVCs for bucket logging and bucket notifications.
  • Reuse common code between notifications and logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes are the two new commits from this week, LMKWYT

@alphaprinz alphaprinz requested review from jackyalbo and dannyzaken and removed request for shirady November 14, 2024 06:51
dannyzaken
dannyzaken reviewed Nov 17, 2024
View reviewed changes
Copy link
Contributor

@dannyzaken dannyzaken left a comment

Choose a reason for hiding this comment

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

I added one small comment, other than that LGTM.
I will let Jacky give the final approval as he is more familiar with this code.
Don't forget to squash your commits.

pkg/apis/noobaa/v1alpha1/noobaa_types.go Outdated Show resolved Hide resolved
jackyalbo
jackyalbo reviewed Nov 19, 2024
View reviewed changes
pkg/system/phase1_verifying.go Outdated Show resolved Hide resolved
pkg/system/phase1_verifying.go Outdated Show resolved Hide resolved
pkg/system/phase2_creating.go Show resolved Hide resolved
pkg/system/phase2_creating.go Outdated Show resolved Hide resolved
pkg/system/phase2_creating.go Outdated Show resolved Hide resolved
pkg/system/phase2_creating.go Outdated Show resolved Hide resolved
@alphaprinz alphaprinz merged commit a79c6b5 into noobaa:master Nov 19, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannyzaken dannyzaken dannyzaken left review comments

@jackyalbo jackyalbo jackyalbo approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alphaprinz @dannyzaken @jackyalbo