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

clh: Add some error handling for clh#2863

Merged
likebreath merged 1 commit into
kata-containers:masterfrom
amshinde:add-clh-error-handling
Aug 21, 2020
Merged

clh: Add some error handling for clh#2863
likebreath merged 1 commit into
kata-containers:masterfrom
amshinde:add-clh-error-handling

Conversation

@amshinde
Copy link
Copy Markdown
Member

@amshinde amshinde commented Jul 28, 2020

The function for getting version info was failing on a system
without any indication of the error. Add some error handling
around this function.

Fixes: github.com/kata-containers/kata-containers#463

Signed-off-by: Archana Shinde archana.m.shinde@intel.com

@auto-comment
Copy link
Copy Markdown

auto-comment Bot commented Jul 28, 2020

Thank you for raising your pull request. Please note that the main development of Kata Containers has moved to the 2.0-dev branch of https://github.com/kata-containers/kata-containers repository. The kata-containers/runtime repository is kept for 1.x release maintenance. Please check twice if your change should go to the 2.0-dev branch directly.

If it is strongly required for adding the change to Kata Containers 1.x releases, please ping @kata-containers/runtime to assign a dedicated developer to be responsible for porting the change to 2.0-dev branch. Thanks!

@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Jul 28, 2020

/test-ubuntu

likebreath
likebreath approved these changes Jul 28, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@likebreath likebreath left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@amshinde amshinde force-pushed the add-clh-error-handling branch from 221d5f8 to b42a4e6 Compare July 28, 2020 23:15
@likebreath
Copy link
Copy Markdown
Contributor

likebreath commented Jul 28, 2020

/test-clh

Note: we are expecting an regression failure on clh-docker CI as observed from the other two PRs (#2840 and #2833). Here is the issue #2864 I just opened to track this failure.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 29, 2020 •
edited
Loading

Codecov Report

Merging #2863 into master will increase coverage by 0.34%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
+ Coverage   51.05%   51.40%   +0.34%     
==========================================
  Files         118      118              
  Lines       17364    17441      +77     
==========================================
+ Hits         8866     8965      +99     
+ Misses       7419     7393      -26     
- Partials     1079     1083       +4

jodh-intel
jodh-intel reviewed Jul 29, 2020
View reviewed changes
Comment thread virtcontainers/clh.go Outdated
major, err := strconv.ParseUint(versionSplit[0], 10, 64)
if err != nil {
return err
return fmt.Errorf("Failed to parse cloud-hypervisor version: %s, Unexpected format", words[1])
Copy link
Copy Markdown

@jodh-intel jodh-intel Jul 29, 2020

Choose a reason for hiding this comment

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

  • if we get to this error scenario, something unexpected happened so maybe words[1] isn't what we expect either. As such, I'd consider logging words, not just words[1] (same comment for the other errors below).
  • To help with debugging, you might want to log words when the check on the number of fields above fails too?

Copy link
Copy Markdown
Member Author

@amshinde amshinde Aug 20, 2020

Choose a reason for hiding this comment

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

Agree. I am using words in all error logs now. Thanks.

@likebreath likebreath mentioned this pull request Jul 29, 2020
Merged
@amshinde amshinde force-pushed the add-clh-error-handling branch from b42a4e6 to ada0467 Compare August 20, 2020 20:53
@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Aug 20, 2020

/test-ubuntu

@likebreath
Copy link
Copy Markdown
Contributor

likebreath commented Aug 20, 2020

Travis CI is complaining, and it looks like some formatting issue?

virtcontainers/clh.go:133: File is not `gofmt`-ed with `-s` (gofmt)

@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Aug 20, 2020

@likebreath Yes, I was just taking a look at that. But I cant seem to figure out why. Running gofmt -w virtcontainers/clh.go shows no difference in formatting for me.
Can you help me out by running gofmt at your end and see if there is a difference?

@likebreath
Copy link
Copy Markdown
Contributor

likebreath commented Aug 20, 2020

@amshinde Looks like your gofmt is different from ours... I have the following by running gofmt on your PR. You can remove the changes you made in your PR, and the Travis CI should be happy.

 1diff --git a/virtcontainers/clh.go b/virtcontainers/clh.go                                                                                                                                                       
 2index 74aa364d..2fe4fe01 100644                                                                                                                                                                                  
 3--- a/virtcontainers/clh.go                                                                                                                                                                                      
 4+++ b/virtcontainers/clh.go                                                                                                                                                                                      
 5@@ -128,9 +128,9 @@ type cloudHypervisor struct {                                                                                                                                                                
 6 var clhKernelParams = []Param{                                                                                                                                                                                  
 7                                                                                                                                                                                                                 
 8        {"root", "/dev/pmem0p1"},                                                                                                                                                                                
 9-       {"panic", "1"},                                     // upon kernel panic wait 1 second before reboot                                                                                                     
10-       {"no_timer_check", ""},                             // do not check broken timer IRQ resources                                                                                                           
11-       {"noreplace-smp", ""},                              // do not replace SMP instructions                                                                                                                   
12+       {"panic", "1"},         // upon kernel panic wait 1 second before reboot                                                                                                                                 
13+       {"no_timer_check", ""}, // do not check broken timer IRQ resources                                                                                                                                       
14+       {"noreplace-smp", ""},  // do not replace SMP instructions                                                                                                                                               
15        {"rootflags", "data=ordered,errors=remount-ro ro"}, // mount the root filesystem as readonly                                                                                                             
16        {"rootfstype", "ext4"},                                                                                                                                                                                  
17 }

likebreath
likebreath reviewed Aug 20, 2020
View reviewed changes
Comment thread virtcontainers/clh.go Outdated
Comment on lines +131 to +133
{"panic", "1"}, // upon kernel panic wait 1 second before reboot
{"no_timer_check", ""}, // do not check broken timer IRQ resources
{"noreplace-smp", ""}, // do not replace SMP instructions
Copy link
Copy Markdown
Contributor

@likebreath likebreath Aug 20, 2020

Choose a reason for hiding this comment

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

This is where gofmt is not happy about. Looks like your gofmt made the change?

@amshinde
clh: Add some error handling for clh
c87ff44 
The function for getting version info was failing on a system
without any indication of the error. Add some error handling
around this function.

Fixes: github.com/kata-containers/kata-containers#463

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde force-pushed the add-clh-error-handling branch from ada0467 to c87ff44 Compare August 21, 2020 00:19
@amshinde
Copy link
Copy Markdown
Member Author

amshinde commented Aug 21, 2020

/test-ubuntu

@likebreath likebreath merged commit ba86cad into kata-containers:master Aug 21, 2020
@likebreath likebreath added the port-to-2.0 PRs that need to be ported to kata 2.0-dev branch label Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jcvenegas jcvenegas Awaiting requested review from jcvenegas

3 more reviewers

@likebreath likebreath likebreath approved these changes

@jodh-intel jodh-intel jodh-intel left review comments

@egernst egernst egernst approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

port-to-2.0 PRs that need to be ported to kata 2.0-dev branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@amshinde @likebreath @egernst @jodh-intel