Skip to content

[management/client] create job channel between management and client#4367

Merged
aliamerj merged 16 commits intofeature/remote-debugfrom
jobs-channel
Aug 28, 2025
Merged

[management/client] create job channel between management and client#4367
aliamerj merged 16 commits intofeature/remote-debugfrom
jobs-channel

Conversation

@aliamerj
Copy link
Copy Markdown
Contributor

@aliamerj aliamerj commented Aug 18, 2025

Describe your changes

This PR sets up a communication channel between the management server and the client to handle job requests and responses.

What was done:

  • Created a Job channel for each peer in JobManager.
  • Send messages from management to the client.
  • Send replies from the client back to management.
  • Prepared client-side Job() stream to receive messages from the server.
  • Integrated peer lifecycle management with ephemeral and secrets managers.

Issue ticket number and link #4354

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

@aliamerj aliamerj requested a review from mlsmaycon August 18, 2025 21:01
@aliamerj aliamerj changed the title feat: create job channel between management and client [management/client] create job channel between management and client Aug 18, 2025
@aliamerj aliamerj changed the base branch from main to feature/remote-debug August 19, 2025 14:12
Comment thread shared/management/proto/management.proto Outdated
@aliamerj
Copy link
Copy Markdown
Contributor Author

@pascal-fischer
can i merge this pr if there is no other feedback ?

@pascal-fischer
Copy link
Copy Markdown
Collaborator

Ah I just had a look at the proto. Let me review the whole thing now

Comment thread client/internal/engine.go Outdated
Comment thread management/server/grpcserver.go Outdated
Comment thread management/server/grpcserver.go Outdated
Comment thread management/server/grpcserver.go Outdated
Comment thread management/server/grpcserver.go Outdated
Comment thread management/server/grpcserver.go
Comment thread management/server/grpcserver.go
Comment thread management/server/grpcserver.go Outdated
Comment thread management/server/jobChannel.go
Comment thread management/server/jobChannel.go
Comment thread management/server/grpcserver.go Outdated
@aliamerj
Copy link
Copy Markdown
Contributor Author

@pascal-fischer
can i merge it or still not ready ?

@pascal-fischer
Copy link
Copy Markdown
Collaborator

Almost. I just accepted sonar for this case (usually, there should be no code quality issues, but I loosened the rules a bit here) If you could just fix the linter complaint should be ready

@pascal-fischer
Copy link
Copy Markdown
Collaborator

The Docs test is failing because you did not set a mark here. In the PR description just set "Documentation is not needed for this change (explain why)" then this failed test is gone as well. The Benchmarks are a bit flaky depending on the runner they are executed on (we can rerun those, usually is fixed on the next run). And when all the tests are green you can merge

@aliamerj
Copy link
Copy Markdown
Contributor Author

Almost. I just accepted sonar for this case (usually, there should be no code quality issues, but I loosened the rules a bit here) If you could just fix the linter complaint should be ready

the linter complaint comes from the codespell in the generated code from .proto

@pascal-fischer
Copy link
Copy Markdown
Collaborator

Error: ./shared/management/proto/management.proto:77: successed ==> succeeded, success, successful
The source proto file already has the same typo. Once that one is fixed and proto is generated it will be fixed in all 3 places.

@pascal-fischer
Copy link
Copy Markdown
Collaborator

Ok no maybe we are not done. I just reviewed the thing as a whole again. One sec I will share

Comment thread management/server/grpcserver.go
Comment thread management/server/grpcserver.go
Comment thread shared/management/client/grpc.go
Comment thread management/server/grpcserver.go
Comment thread management/server/grpcserver.go Outdated
Read Operation = "read"
Update Operation = "update"
Delete Operation = "delete"
Job Operation = "job"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is also not right. Those are CRUD operations. You need this as a module and the Create for starting a job and get to check the status. This stays pretty much in sync with the http operation GET, POST, PUT, DELETE. We do plan on moving the whole permission validation into the http router middleware then it might get even clearer.

This repo contains a new design for our management service. You can read this repo, this is where we want to go towards with out code. Here is the example how how permissions gonna be used in the future https://github.com/netbirdio/management-refactor/blob/main/internals/modules/peers/manager/api.go

@sonarqubecloud
Copy link
Copy Markdown

@aliamerj aliamerj merged commit d4ac7f8 into feature/remote-debug Aug 28, 2025
36 of 37 checks passed
@aliamerj aliamerj deleted the jobs-channel branch August 28, 2025 13:49
aliamerj added a commit that referenced this pull request Oct 6, 2025
…#4367)

* new bi-directional stream for jobs

* create bidirectional job channel to send requests from the server and receive responses from the client

* fix tests

* fix lint and close bug

* fix lint

* clean up & fix close of closed channel

* add nolint:staticcheck

* remove some redundant code from the job channel PR since this one is a cleaner rewrite

* cleanup removes a pending job safely

* change proto

* rename to jobRequest

* apply feedback 1

* apply feedback 2

* fix typo

* apply feedback 3

* apply last feedback
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