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

virtcontainers: Extract shim related operations to a separate package#888

Closed
zsc-hw wants to merge 1 commit into
kata-containers:masterfrom
zsc-hw:master
Closed

virtcontainers: Extract shim related operations to a separate package#888
zsc-hw wants to merge 1 commit into
kata-containers:masterfrom
zsc-hw:master

Conversation

@zsc-hw
Copy link
Copy Markdown

@zsc-hw zsc-hw commented Nov 7, 2018 •
edited
Loading

With the increase of code in the "virtcontainers" package,
code coupling is also increasing. Therefore, I want to
recfactor the code of shim, agent, proxy, network
and other related modules, decouple them and put
them into a separate package.
Now I put the relevant code of "shim" into the independent
"shim" package. I hope it can be perfected through
your suggestions. If possible, I will continue to reconstruct
agent, proxy, network and so on.

Fixes: #890

Signed-off-by: Shucheng Zhang zhangshucheng@huawei.com

jshachm
jshachm reviewed Nov 7, 2018
View reviewed changes
Comment thread virtcontainers/pkg/oci/utils.go Outdated
@zsc-hw zsc-hw force-pushed the master branch 2 times, most recently from 7be41a4 to 921baf1 Compare November 7, 2018 10:16
@zsc-hw zsc-hw changed the title Refactoring code to extract shim related operations to a separate package virtcontainers:Refactoring code to extract shim related operations to a separate package Nov 7, 2018
@sboeuf
Copy link
Copy Markdown

sboeuf commented Nov 7, 2018

@zsc-hw
Hi, I just got a look at your PR and I have several comments:

  • What's the benefit of moving the shim interface to its own package? I feel that decoupling too much will lead to some complexity that's not really worth it.
  • Moving the types to a separate and global package might be a good idea, but if we do so, please move those to virtcontainers/pkg/types.
  • You didn't modify the unit tests yet, but that's where you might have some trouble with all those new packages.
  • Please make sure your commit message complies with Kata Containers guidelines.

/cc @bergwolf @WeiZhang555 @egernst @amshinde

@WeiZhang555
Copy link
Copy Markdown
Member

WeiZhang555 commented Nov 8, 2018 •
edited
Loading

@sboeuf

What's the benefit of moving the shim interface to its own package? I feel that decoupling too much will lead to some complexity that's not really worth it.

It will lead to some complexity, but I think that's worth it. You know, I mentioned a lot that I hope to refactor the virtcontainers and decouple each components to seperate dirs, this requires lots of work, but I'm so lazy so this has been postponed for long time. I remember that @bergwolf mentioned this too.

Two benefits for seperating these dirs:

  1. More friendly for scalability. Shim may not be a good example, take Hypervisor as an example, currently we only support qemu, we may want to support more hypervisors such as xen/libvirt/hyperV, putting them in seperate dir can make programmers easier to understand how to extend this.

  2. Better readability. hierarchical codes are more friendly to code readers.

So I think @zsc-hw gives us a good start :-)

@bergwolf
Copy link
Copy Markdown
Member

bergwolf commented Nov 8, 2018

@sboeuf I tend to agree with @WeiZhang555. Modules with clear interfaces can be put in separate packages and it makes code maintenance easier IMHO.

@sboeuf
Copy link
Copy Markdown

sboeuf commented Nov 8, 2018

@bergwolf @WeiZhang555
If you really think this would be easier to understand for other developers, then I'm fine with that :)

@bergwolf
Copy link
Copy Markdown
Member

bergwolf commented Nov 8, 2018

Thanks @sboeuf ! The refactoring process might be slow but I think it is the right direction.

@zsc-hw zsc-hw force-pushed the master branch 2 times, most recently from 03242c7 to 5e8e3f1 Compare November 9, 2018 02:20
@zsc-hw zsc-hw changed the title virtcontainers:Refactoring code to extract shim related operations to a separate package virtcontainers:extract shim related operations to a separate package Nov 9, 2018
@zsc-hw zsc-hw force-pushed the master branch 7 times, most recently from 7b9cc15 to dddac52 Compare November 13, 2018 07:34
@egernst
Copy link
Copy Markdown
Member

egernst commented Nov 28, 2018

@sboeuf can you take another look at this PR?

@sboeuf
Copy link
Copy Markdown

sboeuf commented Nov 28, 2018

@egernst yes I will!

sboeuf
sboeuf suggested changes Nov 28, 2018
View reviewed changes
Copy link
Copy Markdown

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

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

@zsc-hw the overall PR looks good but I still have a few comments here:

  • Please rework the commit message to comply with Kata Containers guidelines (Look at other already merged commits as an example):
    • 72 characters max for each line of the commit message
    • the Fixes #890 should be after the message
    • Make sure you add a space between virtcontainers: and the rest of the commit title
  • Split this commit into at least two different commits:
    • First commit taking care of moving the structure definitions to virtcontainers/pkg/types and fixing everything that needs to be fixed
    • Second commit being the move of all shim related parts

@sameo sameo mentioned this pull request Dec 20, 2018
Closed
8 tasks
@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Dec 20, 2018

@zsc-hw any updates on this PR?

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Jan 7, 2019

Ping @zsc-hw - do you think you'll be able to work on reformatting the commit message?

Also, it feels a bit odd having the *_shim_test.go files under virtcontainers/ but the implementations in virtcontainers/shim/*.go - shouldn't the tests be moved too?

@egernst
Copy link
Copy Markdown
Member

egernst commented Jan 8, 2019

@WeiZhang555 @zsc-hw - I'd love to see this PR move forward -- any chance for updates here?

@zsc-hw
Copy link
Copy Markdown
Author

zsc-hw commented Jan 15, 2019

@jodh-intel @egernst
I'll update it these days.

@zsc-hw zsc-hw force-pushed the master branch 2 times, most recently from 1c4f043 to bc4160a Compare January 17, 2019 02:49
@zsc-hw zsc-hw force-pushed the master branch 5 times, most recently from 57f0db8 to 58e0398 Compare January 18, 2019 06:37
@zsc-hw zsc-hw changed the title virtcontainers:extract shim related operations to a separate package virtcontainers: Extract shim related operations to a separate package Jan 18, 2019
@zsc-hw
virtcontainers: Extract shim related operations to a separate package
723e37b 
reason: With the increase of code in the "virtcontainers" package,
 code coupling  is also increasing. Therefore, I want to recfactor
 the code of shim, agent, proxy, network and other related modules,
 decouple them and put them into a separate package.
Now I put the relevant code of "shim" into the independent "shim"
 package. I hope it can be perfected through your suggestions.
 If possible, I will continue to reconstruct agent,
 proxy, network and so on.

Fixes: kata-containers#890

Signed-off-by: Shucheng Zhang  <zhangshucheng@huawei.com>
@caoruidong
Copy link
Copy Markdown
Member

caoruidong commented Jan 18, 2019

/retest

@zsc-hw
Copy link
Copy Markdown
Author

zsc-hw commented Jan 18, 2019

@jodh-intel @egernst @sameo @WeiZhang555
Please review the code for shim, with a few additional notes below.

  1. This PR still keeps'*_shim_test.go'under the'virtcontainers/' package.
    In order to make the code clear, I want to put it in the next PR,
    because it involves'SandboxConfig'struct, and therefore NetworkConfig,
    HypervisorConfig, ProxyConfig, ContainerConfig, and so on.
  2. I moved the files under the'virtcontainers/types'package to the
    'virtcontainers/pkg/types' package,
    and I felt that it might be better to merge the two packages into one.
    In my future PR submissions, more content will be placed under the
    'virtcontainers/pkg/types'package, as shown in the figure below.

2019-01-18_175636

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Jan 23, 2019

@zsc-hw - thanks for explaining. Please could you split this into 2 commits as requested by @sboeuf?

@raravena80
Copy link
Copy Markdown
Member

raravena80 commented Feb 5, 2019

@zsc-hw ping any updates? Thx!

@shuchengzg
Copy link
Copy Markdown

shuchengzg commented Feb 11, 2019

@raravena80
It will be updated in the next few days

@egernst
Copy link
Copy Markdown
Member

egernst commented Feb 26, 2019

friendly ping :)

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Feb 26, 2019

Branch needs updating due to conflicts.

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Mar 5, 2019

@sameo - ptal, as afaik you are also looking at and undertaking much vc refactor and breakout.
@zsc-hw - looks like a major rebase is needed at least.

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Mar 11, 2019

Hi @zsc-hw - please could you rebase to resolve the conflicts on this PR?

@jodh-intel jodh-intel closed this Mar 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jshachm jshachm jshachm left review comments

@sboeuf sboeuf sboeuf requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.