-
Notifications
You must be signed in to change notification settings - Fork 4
[drbd-build-svr]: add readme #332
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
base: vkarpochev-drbd-build-server
Are you sure you want to change the base?
[drbd-build-svr]: add readme #332
Conversation
| - `kernel_version` (optional): Kernel version (alternative to header) | ||
|
|
||
| **Response:** | ||
| - `202 Accepted`: Build job created or already in progress |
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.
Should we mention error responses here?
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 suppose later we should make an open API spec later.
| "created_at": "2024-01-01T12:00:00Z", | ||
| "completed_at": "2024-01-01T12:05:00Z", | ||
| "duration": "5m0s", | ||
| "cache_path": "/var/cache/drbd-build-server/abc123....tar.gz", |
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.
This looks like server internal info we should not leak. Maybe keep as debug header configured on server side?
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 should refactor API later as written above
| Initiates a build request for DRBD kernel modules. | ||
|
|
||
| **Request:** | ||
| - **Body**: Kernel headers as `tar.gz` archive |
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.
Should we give an example of header archive structure?
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.
Now we don't use a specific archive structure. So, I don't think we should explain it. Perhaps it would be more convenient to provide more detailed examples at the MVC stage.
|
|
||
| The server uses an intelligent caching mechanism: | ||
|
|
||
| - **Cache Key**: SHA256 hash of kernel version + kernel headers data |
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.
So if user do not specify the version and specified exact version as in headers will it be different cache record and builds?
Also I guess we could specify that headers hash calculated on uncompressed headers
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.
May be it would be more correct to not explain hashing algorithm
| Initiates a build request for DRBD kernel modules. | ||
|
|
||
| **Request:** | ||
| - **Body**: Kernel headers as `tar.gz` archive |
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'm sure we also need from the user the body Content-Type: application/gzip or maybe Content-Type: application/x-tar and Content-Encoding: gzip. I guess the first one is the correct way.
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 suppose later we should make an open API spec . We should get information how it will be useful and fix one. Now for prototype stage we can skip it.
asergunov
left a comment
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.
So it should be at least two requests. One is POST to give headers to server and receive the hash based download link and job status link. Second request is GET of artefact blocking while it's building. Optionally user can ask for job status with separate parallel request but I'd make RESTless option when user should not invent its own timeouts but receive the status messages to the opened connection.
|
|
||
| **Response:** | ||
| - `202 Accepted`: Build job created or already in progress | ||
| - `200 OK`: Cached module found, returns module file immediately |
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.
Do we have the same json response both return codes? May be give download link right away? What if we give download link even if it's not done yet? I mean we can give an option to keep connection opened while it's building. So user will not have to write loop and invent timeout values.
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 think we should conduct a few tests, evaluate the build speed, and then make a decision based on the results. This is beyond the scope of the PR. Actually we need refactor API almost I suppose
| } | ||
| ``` | ||
|
|
||
| ### GET `/api/v1/download/{job_id}` |
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 should be a hash instead of job id here. So proxies can cache it.
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.
Now job id is the first 16 digets of hash
|
I actually can't see any reason to have job id. Because in our logic it's the same as build hash. I mean we should mention that job id is not a number, but string. This way we could change server behaviour without changing client |
|
Let's just add todo sections to this document and some explanation: that's what we have now and that's what we going to change, why we want it. So we can merge this PR |
0f90bb7 to
448f5fc
Compare
448f5fc to
b530c9a
Compare
b530c9a to
568b150
Compare
49fc3d5 to
c6f2207
Compare
|
Need close because the API and design are outdated. Useful comments should be moved to PR328. |
Please do |
No description provided.