-
Notifications
You must be signed in to change notification settings - Fork 0
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
COSI-44: Integrate Local gRPC Provisioner Server, and Refactor COSI Driver for Improved Maintainability and Observability #24
Conversation
@@ -0,0 +1,32 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just do a LICENSE
file? (nitpick)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we can, will remove from all in one go
) | ||
|
||
var ( | ||
_ cosi.IdentityClient = &COSIProvisionerClient{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do these lines do? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here’s a professional and clear response you can use to reply to the comment:
These lines are compile-time assertions that ensure COSIProvisionerClient implements the cosi.IdentityClient interface. Here’s what they do:
var (
_ cosi.IdentityClient = &COSIProvisionerClient{}
)
This syntax assigns a pointer to COSIProvisionerClient to a variable of type cosi.IdentityClient (using _ as a placeholder). It’s a way to enforce, at compile time, that COSIProvisionerClient satisfies the cosi.IdentityClient interface.
If COSIProvisionerClient is missing any methods required by the cosi.IdentityClient interface, this code will trigger a compilation error. It’s a common pattern in Go to ensure a struct or type adheres to an interface, especially when there’s no explicit assignment elsewhere in the code.
Without this line, a mismatch between the struct and interface might only be caught at runtime when the struct is used incorrectly. And I want to be aligned with COSI spec now that we have migrated the provisioner server on our side. ;)
Let me know if you’d like further clarification! 😊
(BTW, it was from SIGS repo, orignally)
Copy provisioner server to cosi-driver [Source](https://github.com/kubernetes-sigs/container-object-storage-interface-provisioner-sidecar/tree/master/pkg/provisioner) This commit sets the stage for introducing open telemetry. This introduces the gRPC server to the cosi-driver. With better control of the code running the gRPC server, we can introduce open telemetry to the cosi-driver at gRPC level.
- Renamed package `pkg/provisioner` to `pkg/grpcfactory` to better reflect its role in creating gRPC server. - Renamed files within `pkg/grpcfactory` to match the new package context: `provisioner.go` → `grpc_factory.go` - Renamed `pkg/driver/provisioner.go` to `pkg/driver/provisioner_server_impl.go` to clarify that it contains the `ProvisionerServer` implementation. - Updated imports and references in `cmd/scality-cosi-driver/cmd.go` and throughout the codebase to reflect these changes. - No functional changes; code logic remains the same.
87cf433
to
4bcaa5c
Compare
Rebased to origin/main and pushed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
@@ Coverage Diff @@
## main #24 +/- ##
==========================================
+ Coverage 75.52% 76.01% +0.48%
==========================================
Files 4 7 +3
Lines 237 346 +109
==========================================
+ Hits 179 263 +84
- Misses 51 70 +19
- Partials 7 13 +6
|
IdentityClient: cosi.NewIdentityClient(conn), | ||
ProvisionerClient: cosi.NewProvisionerClient(conn), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I didn't know you could assign an embedded interface like this, good to know!
pkg/grpcfactory/server_test.go
Outdated
ctx context.Context | ||
cancel context.CancelFunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need those, if you add the argument ctx SpecContext
to each test case, it would be canceled automatically when the test case completes, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep forgetting that, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
select { | ||
case err := <-errChan: | ||
Expect(err).NotTo(HaveOccurred()) | ||
default: | ||
// No errors | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me this select would give 100ms for the server to crash with an error, and succeed in all other cases (the server continues to run after 100ms, or the server crashes after 100ms). Would it be better to have only one good behavior expected, whether it's that the server doesn't stop or that the server stops with a success response? (I believe the former is what's expected, in case we can fail the test if the channel has any value, including nil
)
Expect(err).NotTo(HaveOccurred()) | ||
Expect(client).NotTo(BeNil()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it doesn't matter much, but we don't seem to check anything about whether the interceptor is present or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! I didn't put much effort into it yet, as it was part of migrated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But will be improved in the coming weeks: https://scality.atlassian.net/browse/COSI-62
Made improvements to the provisioner package to enhance code simplicity and handle deprecated gRPC functions. - Embedded cosi.IdentityClient and cosi.ProvisionerClient interfaces into COSIProvisionerClient in client.go to simplify the client implementation, reducing boilerplate code. - Corrected condition checks in apiLogger in interceptors.go to ensure JSON marshalling errors are handled properly. Enhanced logging to include error information if the API call fails. - In provisioner.go, removed deprecated grpc.WithBlock() and acknowledged grpc.Dial deprecation. Proceeded without deprecated options, allowing asynchronous connection establishment. Noted that migrating to grpc.NewClient requires significant changes. - Ensured listener cleanup in server.go by adding defer listener.Close(), preventing resource leaks. Improved error messages for clarity and consistency.
9b077b9
to
9214660
Compare
Note for Reviewers
This PR enhances code coverage and refines the cosi-driver project by integrating a local gRPC provisioner server and reorganizing related components. Key changes are as follows:
provisioner.go
indriver
package toprovisioner_ser_impl.go
Screenshots of Coverage Improvement:
New Coverage: 77%
Original Coverage: 75.52%
This PR introduces non-functional changes to the code, primarily improving maintainability and testing and preparing the codebase for observability enhancements with OpenTelemetry.