Skip to content
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.

Sign up for GitHub

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

Jump to bottom

runtime: Update hypervisor generated code #8520

Merged
merged 2 commits into from Nov 30, 2023
Merged

runtime: Update hypervisor generated code #8520

merged 2 commits into from Nov 30, 2023

Conversation

stevenhorsman
Copy link
Member

Update to use ttrpc_out instead of grpc_out

Fixes: #8519

@katacontainersbot katacontainersbot added the size/tiny Smallest and simplest task label Nov 24, 2023
beraldoleal
beraldoleal approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

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

This lgtm, thanks. Let's see if CI will catch something.

@katacontainersbot katacontainersbot added size/small Small and simple task and removed size/tiny Smallest and simplest task labels Nov 24, 2023
@stevenhorsman stevenhorsman added the do-not-merge PR has problems or depends on another label Nov 27, 2023
@stevenhorsman
Copy link
Member Author

Adding do-not-merge whilst I investigate some issues testing this we the cloud-api-adaptor

@stevenhorsman stevenhorsman force-pushed the hypervisor-ttrpc branch 2 times, most recently from f933433 to e10e918 Compare November 29, 2023 09:32
@stevenhorsman stevenhorsman removed the do-not-merge PR has problems or depends on another label Nov 29, 2023
@stevenhorsman
Copy link
Member Author

We've got this working as best we can test with the cloud-api-adaptor with a re-vendor, so I think this is ready to go in now.

@stevenhorsman
Copy link
Member Author

/test

@stevenhorsman stevenhorsman added do-not-merge PR has problems or depends on another and removed do-not-merge PR has problems or depends on another labels Nov 29, 2023
@stevenhorsman
Copy link
Member Author

False alarm on the do-not-merge I thought something was missing, but I've checked and it's fine

wainersm
wainersm reviewed Nov 29, 2023
View reviewed changes
src/runtime/virtcontainers/remote.go
conn *grpc.ClientConn
client pb.HypervisorClient
conn net.Conn
client pb.HypervisorService
Copy link
Contributor

Choose a reason for hiding this comment

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

@stevenhorsman HypervisorClient (struct) implements HypervisorService (Interface), so it compiles. Any special reason to replace the type from the struct to interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

HypervisorClient isn't in this code base anymore? That was a grpc generated code IIRC?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm there is a hypervisorClient (lower case 'h') in 613c75b#diff-0df604aba3c29a070a7ed40486ce8193ecb2a36c2dabf91bb29e7627cff969d8R52 . This should be a private date structure so never mind.

@wainersm
Copy link
Contributor

Static checks / build-checks (runtime, make vendor) (pull_request) fail seems legit

@stevenhorsman
runtime: Update hypervisor generated code
613c75b 
Update to use ttrpc_out instead of grpc_out

Fixes: kata-containers#8519
Signed-off-by: stevenhorsman <steven@uk.ibm.com>
@stevenhorsman
runtime: remote hypervisor updates to ttrpc
47b8c31 
- Update the remote hypervisor code to match the re-genned code for
the ttrpc Hypervisor Service

Fixes: kata-containers#8519
Signed-off-by: stevenhorsman <steven@uk.ibm.com>
@stevenhorsman
Copy link
Member Author

Static checks / build-checks (runtime, make vendor) (pull_request) fail seems legit

Sorry the go mod tidy I ran was in one of the debug commits I dropped, so I've re-run and pushed. Thanks for flagging it up

wainersm
wainersm approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@wainersm wainersm left a comment

Choose a reason for hiding this comment

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

Thanks @stevenhorsman !

@fidencio
Copy link
Member

/test

@stevenhorsman stevenhorsman merged commit c611028 into kata-containers:main Nov 30, 2023
162 of 172 checks passed
@stevenhorsman stevenhorsman deleted the hypervisor-ttrpc branch November 30, 2023 10:02
@stevenhorsman stevenhorsman mentioned this pull request Dec 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beraldoleal beraldoleal beraldoleal approved these changes

@wainersm wainersm wainersm approved these changes

@GabyCT GabyCT GabyCT approved these changes

Assignees
No one assigned
Labels
ok-to-test size/small Small and simple task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CC: Switch hypervisor service to use ttRPC over gRPC
6 participants
@stevenhorsman @wainersm @fidencio @beraldoleal @GabyCT @katacontainersbot