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.

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

builder/cli/daemon: integrate with buildkit@v0.3.3 #2779

Merged
merged 2 commits into from
May 9, 2019
Merged

builder/cli/daemon: integrate with buildkit@v0.3.3 #2779

merged 2 commits into from
May 9, 2019

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Mar 28, 2019

Ⅰ. Describe what this PR did

PouchContainer uses builkit to support build functionality. This is
commit which brings very basic build functionality and there is a lot of
improvements will be supported in following pull request.

This builder functionality is controlled by daemon flag
--enable-builder. By default, the builder is disable. If it is stable
and under fully testing, we will make it enable by default.

asciicast

Ⅱ. Does this pull request fix one issue?

#1919

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

add one case

Ⅳ. Describe how to verify it

CI

Ⅴ. Special notes for reviews

@fuweid fuweid requested review from rudyfly and Ace-Tang March 28, 2019 04:31
@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #2779 into master will increase coverage by 1.84%.
The diff coverage is 64.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2779      +/-   ##
==========================================
+ Coverage   67.27%   69.12%   +1.84%     
==========================================
  Files         279      285       +6     
  Lines       17603    17754     +151     
==========================================
+ Hits        11842    12272     +430     
+ Misses       4429     4083     -346     
- Partials     1332     1399      +67
Flag Coverage Δ
#criv1alpha2_test 39.02% <0%> (-0.36%) ⬇️
#integration_test_0 36.62% <64.23%> (+0.23%) ⬆️
#integration_test_1 35.39% <53.64%> (?)
#integration_test_2 36.54% <53.64%> (+0.17%) ⬆️
#integration_test_3 35.41% <53.64%> (+0.15%) ⬆️
#node_e2e_test 34.77% <0%> (-0.34%) ⬇️
#unittest 28.62% <0%> (-0.08%) ⬇️
Impacted Files Coverage Δ
daemon/config/config.go 69.51% <ø> (ø) ⬆️
builder/builder.go 57.69% <57.69%> (ø)
daemon/daemon.go 69.8% <60%> (-0.25%) ⬇️
builder/config.go 60% <60%> (ø)
builder/utils.go 61.9% <61.9%> (ø)
builder/exporter.go 62.5% <62.5%> (ø)
builder/worker.go 70% <70%> (ø)
daemon/builder.go 75% <75%> (ø)
apis/server/utils.go 71.15% <0%> (-3.85%) ⬇️
cri/ocicni/cni_manager.go 58.49% <0%> (-1.89%) ⬇️
... and 34 more

@fuweid fuweid changed the title Me introduce buildkit builder/cli/daemon: integrate with buildkit@v0.3.3 Mar 28, 2019
Copy link
Contributor

@zhuangqh zhuangqh left a comment

Choose a reason for hiding this comment

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

LGTM overall. Thanks for you great job!

if d.config.EnableBuilder {
go func() {
logrus.Info("serving builder server...")
if err := d.runBuilderServer(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are three type of service here, cri service, http server and builder server. A little messy to start them. We can make an abstract interface from them and manage them uniformity later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah

Copy link
Collaborator

Choose a reason for hiding this comment

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

if builder is enabled and start builder server failed, it is an error not a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! thanks @yyb196

}
namedRef = reference.TrimTagForDigest(reference.WithDefaultTagIfMissing(namedRef))
tagList = append(tagList, namedRef.String())

Copy link
Contributor

Choose a reason for hiding this comment

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

redundant empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

v0.4.0 version of buildkit is using very higher golang version. We don't
want to follow this so fast. This is why we use v0.3.3 here.

Using following command to add the packages:

    govendor fetch github.com/moby/buildkit/...@v0.3.3
    govendor fetch github.com/tonistiigi/fsutil@2862f6bc5ac9b97124e552a5c108230b38a1b0ca
    govendor fetch github.com/hashicorp/go-immutable-radix::github.com/tonistiigi/go-immutable-radix@826af9ccf0feeee615d546d69b11f8e98da8c8f1

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@allencloud
Copy link
Collaborator

LGTM, greatly thanks for your work.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 7, 2019
@@ -35,7 +35,8 @@ type Daemon struct {
ctrdDaemon *supervisord.Daemon

// ctrdClient is grpc client connecting to the containerd
ctrdClient ctrd.APIClient
ctrdClient ctrd.APIClient

Copy link
Collaborator

Choose a reason for hiding this comment

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

need format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is already be format.

PouchContainer uses builkit to support build functionality. This is
commit which brings very basic build functionality and there is a lot of
improvements will be supported in following pull request.

This builder functionality is controlled by daemon flag
`--enable-builder`. By default, the builder is disable. If it is stable
and under fully testing, we will make it enable by default.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@yyb196
Copy link
Collaborator

yyb196 commented May 8, 2019

LGTM

@allencloud allencloud merged commit 19851cc into AliyunContainerService:master May 9, 2019
@fuweid fuweid deleted the me-introduce-buildkit branch May 9, 2019 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants