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

Add telemetry job - option 2 #1392

Closed

Conversation

pleshakov
Copy link
Contributor

This PR is for high-level (design) feedback.

Proposed changes

This is PoC for feedback

  • Use Manager leader election intead of our own
  • Add telemetry job with a Stdout Exporter
  • Report telemetry every 10s (good enough for PoC)

Upgrading from edge version (which uses different (our own, not Manager) leader election) was ok:

Update:

NAME                                        READY   STATUS    RESTARTS   AGE
ngf-nginx-gateway-fabric-85ff9dc987-qvwpm   2/2     Running   0          98s
ngf-nginx-gateway-fabric-85ff9dc987-vfqsx   2/2     Running   0          2m7s

...

NAME                                        READY   STATUS              RESTARTS   AGE
ngf-nginx-gateway-fabric-74566c8679-5cmmk   0/2     ContainerCreating   0          1s
ngf-nginx-gateway-fabric-74566c8679-pwg56   2/2     Running             0          5s                                                                                                                               ngf-nginx-gateway-fabric-85ff9dc987-vfqsx   2/2     Running             0          2m17s

... 

NAME                                        READY   STATUS    RESTARTS   AGE
ngf-nginx-gateway-fabric-74566c8679-5cmmk   2/2     Running   0          9s
ngf-nginx-gateway-fabric-74566c8679-pwg56   2/2     Running   0          13s

Lease before:

kubectl get lease
NAME                                       HOLDER                                      AGE
ngf-nginx-gateway-fabric-leader-election   ngf-nginx-gateway-fabric-85ff9dc987-vfqsx   2m26s

Lease after:

kubectl get lease
NAME                                       HOLDER                                                                           AGE
ngf-nginx-gateway-fabric-leader-election   ngf-nginx-gateway-fabric-74566c8679-5cmmk_24f0a772-b5b3-4a55-b8fc-c4d48e22253f   2m33

Closes #1382

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

This is PoC for feedback

- Use Manager leader election intead of our own
- Add telemetry job with a Stdout Exporter
- Report telemetry every 10s (good enough for PoC)
@github-actions github-actions bot added the enhancement New feature or request label Dec 14, 2023
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

One small question, but overall I like this option better than option 1.

if err != nil {
return err
}
if err = mgr.Add(&enableOnLeaderElectedJob{enable: statusUpdater.Enable}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart way to do this without a big refactor 👍

@sjberman
Copy link
Collaborator

Always a fan of utilizing all that controller-runtime has to offer, so I do prefer this option.

@pleshakov pleshakov mentioned this pull request Dec 16, 2023
6 tasks
@pleshakov
Copy link
Contributor Author

Thanks for the feedback. I will proceed with this approach and move this PR from draft to ready to review once it is ready.

@pleshakov
Copy link
Contributor Author

closing in favor of #1448

@pleshakov pleshakov closed this Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Telemetry Job (NGF)
3 participants