[internal/hcs] Migrate package from HCS V1 to V2#2735
Conversation
54b7a44 to
d3f62f2
Compare
c399db9 to
e76ebeb
Compare
helsaawy
left a comment
There was a problem hiding this comment.
this change will cause the shim to always block on all operations, right? do we have any perf numbers on how much impact this will have?
| // owns the operation handle leads to use-after-free crashes | ||
| // (EXCEPTION_ACCESS_VIOLATION) inside computecore.dll. Callers must not | ||
| // rely on ctx to bound the call's duration. | ||
| func runOperation(ctx context.Context, fn func(op computecore.HcsOperation) error) (resultDoc string, err error) { |
There was a problem hiding this comment.
we always end up calling processHcsResult on the operation result; ie:
resultJSON, err := runOperation(ctx, func(op computecore.HcsOperation) error { /* ... */ })
if err != nil {
return nil, makeSystemError(computeSystem, operation, err, processHcsResult(ctx, resultJSON))
}can we elevate hcsResult to an error type (since it already (sorta) matches the ResultError API type), and then move the processHcsResult logic into run[Process]Operation?
that would reduce a lot of boilerplate
There was a problem hiding this comment.
Thanks for the suggestion. Incorporated the same.
78d3a26 to
5e7dd9c
Compare
|
@helsaawy You're right that every call now goes through Looking at We are planning on creating a perf calculation tool which can run in CI and can provide us a ballpark of the figures. |
| @@ -361,36 +225,33 @@ func (computeSystem *System) FinalizeLiveMigration(ctx context.Context, resume b | |||
| } | |||
| optionsJSON, err := json.Marshal(hcsschema.MigrationFinalizedOptions{FinalizedOperation: finalOp}) | |||
There was a problem hiding this comment.
MigrationFinalizedOptions takes two parameters Origin and FinalizedOperation. HCS uses Origin to evaluate if this is a normal finalization or a rollback after failure. Omitting origin would lead to wrong recovery path. It's safer to set the origin field here.
There was a problem hiding this comment.
Thanks for the suggestion! I have updated FinalizeLiveMigration to the same pattern as other methods. We will accept the opts which are passed by the caller and will set all the options there.
9fd1f62 to
67168df
Compare
Presently, we were using HCS V1 (vmcompute) within internal/hcs but it has been deprecated and therefore, we are moving to use computecore within hcs v2 package. Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
67168df to
cf50569
Compare
Presently, we were using HCS V1 (vmcompute) within internal/hcs but it has been deprecated and therefore, we are moving to use computecore within hcs package.