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

feat: Dynamic watch of numaflow controller def configmap by label #67

Merged
merged 9 commits into from
Jun 21, 2024

Conversation

chandankumar4
Copy link
Contributor

Fixes #13

Modifications

  • Dynamic watch for numaflow controller definition config map based on label, it doesn't need to restart the numaplane to refresh the config

More detail approach:

  • It will use a kubernetes configmap api to looks for any event in configmap, if found then validate it by given label then add/remove from configmap data from the shared controller config data.

Verification

  • Verified in local kind cluster

Comment on lines 1131 to 1132
labels:
config: numaflow-controller-rollout
Copy link
Contributor Author

@chandankumar4 chandankumar4 Jun 20, 2024

Choose a reason for hiding this comment

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

Open for suggestion about defining the label here which will be used while lookup for controller config, this label should not be used for configmap other than controller definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

suggested in the other comment.

@chandankumar4 chandankumar4 force-pushed the config-map-watcher branch 2 times, most recently from db7d1f8 to 4204757 Compare June 20, 2024 14:37
Signed-off-by: chandankumar4 <[email protected]>
Copy link
Contributor

@xdevxy xdevxy left a comment

Choose a reason for hiding this comment

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

Can you create an issue to track adding a unit test for dynamic loading new controller definition with new config map? Thanks!

Comment on lines 20 to 21
labelKey = "config"
labelValue = "numaflow-controller-rollout"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to common.go? Also suggested labelKey is numaplane.numaproj.io/config and labelValue is numaflow-controller-definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

Comment on lines 1131 to 1132
labels:
config: numaflow-controller-rollout
Copy link
Contributor

Choose a reason for hiding this comment

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

suggested in the other comment.

return nil
}

// watchConfigMaps watches for configmaps continuously and writes the data to the given directory based on an event type.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: writes the data to the given directory is no longer accurate.

// watchConfigMaps watches for configmaps continuously and writes the data to the given directory based on an event type.
func watchConfigMaps(ctx context.Context, client kubernetes.Interface, namespace string) {
numaLogger := logger.FromContext(ctx)
watcher, err := client.CoreV1().ConfigMaps(namespace).Watch(ctx, metav1.ListOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we watch with a label selector to filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I checked the ListOptions{} struct it has the way to pass label, let me make the change. Thanks

@xdevxy xdevxy marked this pull request as draft June 20, 2024 19:11
Signed-off-by: chandankumar4 <[email protected]>
@chandankumar4
Copy link
Contributor Author

Can you create an issue to track adding a unit test for dynamic loading new controller definition with new config map? Thanks!

Sure, #69

@xdevxy xdevxy marked this pull request as ready for review June 21, 2024 17:26
@xdevxy xdevxy merged commit 3a3a5cb into numaproj:main Jun 21, 2024
4 checks passed
@chandankumar4 chandankumar4 deleted the config-map-watcher branch June 21, 2024 19:09
darshansimha referenced this pull request in darshansimha/numaplane Jun 25, 2024
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.

Each Controller definition will need to be in a different ConfigMap
2 participants