Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

clh: update CLH to stable/v0.5.x #2487

Merged
merged 2 commits into from Feb 20, 2020
Merged

clh: update CLH to stable/v0.5.x #2487

merged 2 commits into from Feb 20, 2020

Conversation

likebreath
Copy link
Contributor

@likebreath likebreath commented Feb 20, 2020 •
edited

Use CLH branch stable/v0.5.x, and also re-generate the openAPI client
code with the new 'cloud-hypervisor.yaml'.

Fixes: #2488

Signed-off-by: Bo Chen chen.bo@intel.com

@likebreath likebreath requested a review from a team as a code owner February 20, 2020 15:15
@likebreath
Copy link
Contributor Author

/test-ch

sameo
sameo reviewed Feb 20, 2020
View reviewed changes
Copy link

@sameo sameo left a comment

Choose a reason for hiding this comment

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

Besides the kernel cmdline typo, I'd appreciate if this could be done in 2 separate commits: One for the client package update, and another one for the ro fixes.

virtcontainers/clh.go Outdated
@@ -120,6 +120,7 @@ var clhKernelParams = []Param{
{"no_timer_check", ""}, // do not check broken timer IRQ resources
{"noreplace-smp", ""}, // do not replace SMP instructions
{"agent.log_vport", fmt.Sprintf("%d", vSockLogsPort)}, // tell the agent where to send the logs
{"rootflags", "data=ordered,errors=remount -ro ro"}, // mount the root filesystem as readonly
Copy link

Choose a reason for hiding this comment

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

typo: remount -ro should be remount-ro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sameo Thanks for correcting me. Just being updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sameo Just separate the two commits. Any other comments are appreciated. Thanks.

@likebreath likebreath changed the title WIP: clh: Update cloud-hypervisor.yaml clh: Update cloud-hypervisor.yaml Feb 20, 2020
@likebreath
Copy link
Contributor Author

/test-clh

@jcvenegas
Copy link
Member

/test-ubuntu

@likebreath
Copy link
Contributor Author

/test-clh

sboeuf
sboeuf reviewed Feb 20, 2020
View reviewed changes
virtcontainers/pkg/cloud-hypervisor/client/docs/DiskConfig.md Outdated
@@ -5,9 +5,13 @@
Name | Type | Description | Notes
------------ | ------------- | ------------- | -------------
**Path** | **string** | |
**Readonly** | **bool** | | [optional] [default to true]
Copy link

Choose a reason for hiding this comment

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

I thought we agreed this shouldn't default to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sboeuf I believe the default to true comes from the cloud-hypervsior.yaml here. Note that all the code under client/ here are auto-generated based on the yaml file.

Copy link

Choose a reason for hiding this comment

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

Yes this needs to be fixed from CH directly.

Copy link

Choose a reason for hiding this comment

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

My bad, this is fixed now. @likebreath You probaly want to fix accordingly.

@likebreath
Copy link
Contributor Author

/test-clh

@likebreath
Copy link
Contributor Author

/test-ubuntu

@amshinde
Copy link
Member

amshinde commented Feb 20, 2020 •
edited

@likebreath Travis is failing with ERROR: No "Fixes" found. Can you open an issue and address it with Fixes : in the commit message.
https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md#patch-format

@likebreath
Copy link
Contributor Author

@likebreath Travis is failing with ERROR: No "Fixes" found. Can you open an issue and address it with Fixes : in the commit message.

@amshinde Thanks for pointing this out. Yes, I am looking at the same failure. I am wondering do I need to put Fixes xxx in both commits or just the first commit? Thanks.

@grahamwhaley
Copy link
Contributor

You need a Fixes in at least one commit. You can have multiple different ones if you need across multiple commits (but generally we go with one-PR fixes one-Issue, generally)

@likebreath likebreath force-pushed the update-clh-openapi-yaml branch 2 times, most recently from b6a992b to da5daf3 Compare February 20, 2020 16:24
@likebreath
Copy link
Contributor Author

/test-clh

@likebreath
Copy link
Contributor Author

/test-ubuntu

@likebreath
Copy link
Contributor Author

/test-clh

@likebreath
Copy link
Contributor Author

/test-ubuntu

@likebreath likebreath changed the title clh: Update cloud-hypervisor.yaml clh: update to the latest master for using new openAPI knobs Feb 20, 2020
@likebreath
Copy link
Contributor Author

/test-clh

@likebreath
Copy link
Contributor Author

/test-clh

@likebreath
Copy link
Contributor Author

/test-ubuntu

@likebreath
Copy link
Contributor Author

/test-clh

@likebreath
Copy link
Contributor Author

/test-ubuntu

@likebreath likebreath changed the title clh: update to the latest master for using new openAPI knobs clh: update CLH to stable/v0.5.x Feb 20, 2020
@codecov
Copy link

codecov bot commented Feb 20, 2020 •
edited

Codecov Report

Merging #2487 into master will increase coverage by 5.44%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2487      +/-   ##
==========================================
+ Coverage   45.63%   51.07%   +5.44%     
==========================================
  Files         114      114              
  Lines       16219    16220       +1     
==========================================
+ Hits         7401     8285     +884     
+ Misses       8001     6919    -1082     
- Partials      817     1016     +199

@likebreath
Copy link
Contributor Author

/test-clh

@likebreath
Copy link
Contributor Author

/test-ubuntu

@likebreath
clh: Update CLH to stable/v0.5.x
de8fe25 
Use CLH branch stable/v0.5.x, and also re-generate the openAPI client
code with the new 'cloud-hypervisor.yaml'.

Fixes: kata-containers#2488

Signed-off-by: Bo Chen <chen.bo@intel.com>
@likebreath
clh: Update clh driver to use the latest openAPI knobs
12d10eb 
We leverage the new openAPI knobs from CLH to set readonly for disk image
and we also pass kernel cmd to set guest root filesystem readonly.

Signed-off-by: Bo Chen <chen.bo@intel.com>
@jcvenegas jcvenegas merged commit d54723a into kata-containers:master Feb 20, 2020
@amshinde amshinde mentioned this pull request May 29, 2020
Merged
@yuvalavra yuvalavra mentioned this pull request Jun 13, 2020
Merged
@tatianab tatianab mentioned this pull request Nov 8, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sboeuf sboeuf sboeuf left review comments

@sameo sameo sameo approved these changes

@jcvenegas jcvenegas jcvenegas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clh: update CLH to stable/v0.5.x
6 participants
@likebreath @jcvenegas @amshinde @grahamwhaley @sameo @sboeuf