[internal/hcs] Migrate package from HCS V1 to V2#2735
Conversation
rawahars
force-pushed
the
migrate_hcs_v1_v2
branch
15 times, most recently
from
54b7a44 to
d3f62f2
Compare
rawahars
force-pushed
the
migrate_hcs_v1_v2
branch
3 times, most recently
from
adb6ee4 to
c399db9
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. Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
rawahars
force-pushed
the
migrate_hcs_v1_v2
branch
from
c399db9 to
e76ebeb
Compare
helsaawy
left a comment
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?
| closeOnce sync.Once | ||
| exited chan struct{} | ||
| closed chan struct{} | ||
| raw string |
There was a problem hiding this comment.
might be cleaner to use:
| raw string | |
| raw json.RawMessage |
There was a problem hiding this comment.
Thanks for the suggestion. Incorporated the same.
| // 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.
rawahars
force-pushed
the
migrate_hcs_v1_v2
branch
from
3325c27 to
78d3a26
Compare
Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
rawahars
force-pushed
the
migrate_hcs_v1_v2
branch
from
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. |
Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
Previously, Close signaled waitBackground via a dedicated notify.closed channel, and waitBackground had to forward that into waitBlock with the right waitError. This left two channels doing the same job and split the "finalize the wait state" logic across two places. Now Close finalizes the wait state itself: under closedWaitOnce it sets waitError = ErrAlreadyClosed and closes waitBlock directly. waitBackground selects on waitBlock (Close path, return early) or notify.exited (real exit, parse status and publish). The notify.closed channel and its signalClosed helper are removed. Behavior is unchanged: Close still does not synthesize an exit, so Wait/WaitChannel callers see ErrAlreadyClosed instead of a fake exit code. Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
The V2 notification handler only dispatched System/ProcessExited, so a
ServiceDisconnect (no payload) left waitBackground blocked forever and
Wait() hung until Close. V1 returned ErrUnexpectedProcessAbort here.
notificationState now has two channels -- exit (payload) and abort
(error) -- and waitBackground selects on both. ServiceDisconnect routes
to signalAbort(ErrUnexpectedProcessAbort), surfaced via make{System,
Process}Error.
Also stop swallowing HcsSet*Callback failures in registerNotification:
return the error so Create/Open{ComputeSystem,Process} can tear down
the handle (OpenProcess gains the missing cleanup defer).
Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
| @@ -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.
5 participants
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.