Feature: build cluster image with buildah #1611

justadogistaken · 2022-07-28T09:49:18Z

Describe what this PR does / why we need it

Sealer has integrated the buildah to build a cluster image.
But There are some todos.

How to save container images?
solution: scan the whole kubefile, pre-download all "http://" to build context. And check the image: of them, then save then in registry under build context. Finally add an instruction COPY registry / before real building procedure.
Semantic translator.
All basic instructions are FROM, ADD, COPY, RUN. We will implement all new instructions based on these basics.
So we are going to need a Semantic translator.

A brief flow as following:

todos:

deal with the flags from buildah
commit images by buildah.
find a place for flags
find a place for global info. Like authfile
support for sealer rootfs
sealer v1.Image refactor. And annotate it to oci image.
list sealer images only for images. Add label to image.
clean environment after build.
multiple platform support.

justadogistaken · 2022-07-29T02:16:54Z

Maybe the sealer imageSpec and image is not in need any more.

// ImageSpec defines the desired state of Image
type ImageSpec struct {
	// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
	// Important: Run "make" to regenerate code after modifying this file

	// Foo is an example field of Image. Edit Image_types.go to remove/update
	ID            string      `json:"id,omitempty"`
	Layers        []Layer     `json:"layers,omitempty"`
	SealerVersion string      `json:"sealer_version,omitempty"`
	Platform      Platform    `json:"platform"`
	ImageConfig   ImageConfig `json:"image_config"`
}

The ImageConfig is valuable probably.

type ImageConfig struct {
	// define this image is application image or normal image.
	ImageType string            `json:"image_type,omitempty"`
	Cmd       ImageCmd          `json:"cmd,omitempty"`
	Args      ImageArg          `json:"args,omitempty"`
	Labels    map[string]string `json:"labels,omitempty"`
}

We can store essential information in annotations of manifest from OCI v1 image.

type ImageCmd struct {
	//cmd list of base image
	Parent []string `json:"parent,omitempty"`
	//cmd list of current image
	Current []string `json:"current,omitempty"`
}

type ImageArg struct {
	//arg set of base image
	Parent map[string]string `json:"parent,omitempty"`
	//arg set of current image
	Current map[string]string `json:"current,omitempty"`
}

For the Cmd and Arg, I think we don't need to record the data from parent image.

cmd/sealer/cmd/build.go

justadogistaken · 2022-08-05T06:53:47Z

build/buildinstruction/utils.go

-	}
-
-	return generateDigest(tmp)
+	return "", nil


Ignore modification of this file.

justadogistaken · 2022-08-09T12:57:23Z

@sealerio/sealer-maintainers @sealerio/sealer-reviewers Help me review this, plz.

codecov-commenter · 2022-08-10T06:05:28Z

Codecov Report

Merging #1611 (4e6d169) into main (2a89fa5) will increase coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1611      +/-   ##
==========================================
+ Coverage   14.04%   14.08%   +0.03%     
==========================================
  Files          78       78              
  Lines        6691     6675      -16     
==========================================
  Hits          940      940              
+ Misses       5637     5621      -16     
  Partials      114      114

Impacted Files	Coverage Δ
apply/apply.go	`0.00% <0.00%> (ø)`
pkg/filesystem/filesystem.go	`0.00% <0.00%> (ø)`
utils/os/readers.go	`0.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

justadogistaken · 2022-08-11T05:47:48Z

test/testhelper/settings/common.go

@@ -78,8 +78,8 @@ var (
 	AccessKey          = os.Getenv("ACCESSKEYID")
 	AccessSecret       = os.Getenv("ACCESSKEYSECRET")
 	Region             = os.Getenv("RegionID")
-	TestImageName      = "" //default: registry.cn-qingdao.aliyuncs.com/sealer-io/kubernetes:v1.19.8
-	TestNydusImageName = "" //default: registry.cn-qingdao.aliyuncs.com/sealer-io/kubernetes-nydus:v1.19.8
+	TestImageName      = "registry.cn-qingdao.aliyuncs.com/sealer-io/kubernetes:v1.19.8.test" //default: registry.cn-qingdao.aliyuncs.com/sealer-io/kubernetes:v1.19.8


change it back before merging.

justadogistaken · 2022-08-13T12:06:15Z

related issues will be fixed.
#1581
#1538

allencloud · 2022-08-15T02:28:35Z

@justadogistaken Conflict happens. Please update this ASAP. And we will merge this with high priority.

justadogistaken · 2022-08-15T03:39:19Z

/test apply

allencloud · 2022-08-15T04:40:10Z

apply/driver/local.go

+		if err := applier.ImageEngine.RemoveContainer(&imagecommon.RemoveContainerOptions{
+			All: true,
+		}); err != nil {
+			logrus.Warnf("failed to clean the buildah contaienrs: %v", err)


typo, s/contaienrs/containers

allencloud · 2022-08-15T04:43:07Z

cmd/sealer/cmd/build.go

@@ -60,48 +69,114 @@ build with args:
 	sealer build -f Kubefile -t my-kubernetes:1.19.8 --build-arg MY_ARG=abc,PASSWORD=Sealer123 .
 `,
 	RunE: func(cmd *cobra.Command, args []string) error {
-		buildContext := args[0]
+		return buildSealerImage()


Here you directly ignore the args, because you have not passed args into function buildSealerImage(). Then I am wondering if we could update Args: cobra.MaximumNArgs(1), to be Args: cobra.NoArgs,.

allencloud · 2022-08-15T04:53:10Z

cmd/sealer/cmd/tag.go

-	Example: `sealer tag kubernetes:v1.19.8 registry.cn-qingdao.aliyuncs.com/sealer-apps/kubernetes:v1.19.8`,
-	Args:    cobra.ExactArgs(2),
+	Use:     "tag",
+	Short:   "create a new tag that refers to a local ClusterImage",


create one or more tags? Since you allow to input multiple tags.

allencloud · 2022-08-15T04:57:00Z

pkg/guest/guest.go

 	)

 	// application image: if cluster cmd not nil, use cluster cmd directly
 	if imageType == common.AppImage {
 		if len(clusterCmd) != 0 {
 			return clusterCmd
 		}
-		return image.Spec.ImageConfig.Cmd.Current
+		return clusterCmd


if len(clusterCmd) != 0 { return clusterCmd } return clusterCmd

I think the condition is useless. You can directly return clusterCmd.

allencloud · 2022-08-15T05:03:47Z

pkg/imageengine/buildah/build.go

+	return options, kubefiles, nil
+}
+
+func (engine *Engine) build(cxt context.Context, kubefiles []string, options define.BuildOptions) (id string, err error) {


I found that this function always returns a nil-error. Is it reasonable? @justadogistaken

allencloud · 2022-08-15T05:09:25Z

pkg/imageengine/buildah/commit.go

+				return err
+			}
+			if len(candidates) == 0 {
+				return errors.Errorf("error parsing target image name %q", image)


I suggest that you could print the detailed error reason, like xxx candidate name is empty?
to add error message readability?

allencloud · 2022-08-15T05:13:43Z

pkg/imageengine/buildah/common.go

+}
+
+func openBuilder(ctx context.Context, store storage.Store, name string) (builder *buildah.Builder, err error) {
+	if name != "" {


What will happen if name is empty string? The code logic seems to implicit when name is empty string.

Will "return nil, errors.Errorf("error finding build container")"

allencloud · 2022-08-15T05:17:02Z

pkg/imageengine/buildah/common.go

+	return context.TODO()
+}
+
+func getFormat(format string) (string, error) {


How about naming this function getImageType()?

allencloud · 2022-08-15T05:17:51Z

pkg/imageengine/buildah/copy.go

+		return errors.Errorf("src must be specified")
+	}
+	if len(opts.DestinationInContainer) == 0 {
+		return errors.Errorf("destination in container must be specifuled")


s/specifuled/specified

allencloud · 2022-08-15T05:22:54Z

pkg/imageengine/buildah/from.go

+	return nil
+}
+
+func onBuild(builder *buildah.Builder, quiet bool) error {


I suggest that we define this function like the following way:

func (builder *buildah.Builder) Build(quiet bool) error

This function if copied from buildah.

allencloud · 2022-08-15T05:26:01Z

pkg/imageengine/buildah/from.go

+			size := len(args)
+			if size > 1 {
+				dest = args[size-1]
+				args = args[:size-1]


try to use another name, like srcs = args[:size-1] ? although the original one has no incorrectness, different naming may bring more convenience.

allencloud · 2022-08-15T05:35:06Z

pkg/imageengine/buildah/from.go

+		case "WORKINGDIR":
+			builder.SetWorkDir(strings.Join(args, " "))
+		default:
+			logrus.Errorf("unknown OnBuild command %q; ignored", onBuildSpec)


I think we should return an error, since unexpected command is input. And we should consider it as illegal behaviour.

allencloud · 2022-08-15T05:41:09Z

pkg/imageengine/buildah/init.go

+)
+
+// TODO do we have an util or unified local storage accessing pattern?
+func writeFileIfNotExist(path string, content []byte) error {


Quite strange for the function definition. We usually create one if it does not exist, but we seldom write it it the path does not exist. Maybe we could separate the function into two parts, createFileIfNotExist and writefile.

allencloud · 2022-08-15T05:43:10Z

pkg/imageengine/buildah/manifest.go

+		case "v2s2", "docker":
+			manifestType = manifest.DockerV2Schema2MediaType
+		default:
+			return errors.Errorf("unknown format %q. Choose on of the supported formats: 'oci' or 'v2s2'", opts.Format)


allencloud · 2022-08-15T05:43:37Z

pkg/imageengine/buildah/mount.go

+func (engine *Engine) Mount(opts *options.MountOptions) ([]options.JSONMount, error) {
+	containers := opts.Containers
+	if len(containers) == 0 {
+		return []options.JSONMount{}, errors.Errorf("id/name of containers mube be specifiled")


s/specifiled/specified
s/mube/must

allencloud · 2022-08-15T05:46:47Z

pkg/imageengine/buildah/push.go

+		case "v2s2", "docker":
+			manifestType = manifest.DockerV2Schema2MediaType
+		default:
+			return errors.Errorf("unknown format %q. Choose on of the supported formats: 'oci', 'v2s1', or 'v2s2'", opts.Format)


Signed-off-by: david.bao <[email protected]>

allencloud · 2022-08-15T07:13:05Z

LGTM, great work 👍🏻

github-actions bot added area/doc Clusterfile ImageBuilding related to all staff with image building labels Jul 28, 2022

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch 2 times, most recently from b89013c to 0068f7a Compare July 28, 2022 10:32

allencloud reviewed Jul 29, 2022

View reviewed changes

cmd/sealer/cmd/build.go Outdated Show resolved Hide resolved

allencloud reviewed Jul 29, 2022

View reviewed changes

cmd/sealer/cmd/build.go Outdated Show resolved Hide resolved

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch from 0068f7a to 9103c74 Compare August 1, 2022 13:18

justadogistaken changed the title ~~[WIP]Feature: build cluster image with buildah~~ Feature: build cluster image with buildah Aug 3, 2022

justadogistaken commented Aug 5, 2022

View reviewed changes

build/buildinstruction/utils.go

}

return generateDigest(tmp)

return "", nil

Copy link

Member Author

justadogistaken Aug 5, 2022

Choose a reason for hiding this comment

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

Ignore modification of this file.

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch 4 times, most recently from f9f99d0 to 34a7eee Compare August 10, 2022 06:00

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch 5 times, most recently from 6e538a6 to 7000139 Compare August 11, 2022 03:11

github-actions bot added the e2e-test it needs to run e2e test label Aug 11, 2022

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch 3 times, most recently from 8983c23 to f1e76ff Compare August 11, 2022 05:43

justadogistaken commented Aug 11, 2022

View reviewed changes

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch 3 times, most recently from 9cf1600 to fbebb19 Compare August 12, 2022 02:18

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch from 9250043 to e6590e4 Compare August 13, 2022 12:04

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch 2 times, most recently from 131b45b to c55919f Compare August 14, 2022 05:09

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch 3 times, most recently from cbbe78f to fa5c8ba Compare August 15, 2022 02:53

allencloud reviewed Aug 15, 2022

View reviewed changes

integrate buildah

2dd7a0e

Signed-off-by: david.bao <[email protected]>

justadogistaken force-pushed the feature/build-cluster-image-with-buildah branch from fa5c8ba to 2dd7a0e Compare August 15, 2022 06:34

allencloud approved these changes Aug 15, 2022

View reviewed changes

allencloud merged commit 3cfab69 into sealerio:main Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: build cluster image with buildah #1611

Feature: build cluster image with buildah #1611

justadogistaken commented Jul 28, 2022 •

edited

Loading

justadogistaken commented Jul 29, 2022 •

edited

Loading

justadogistaken Aug 5, 2022

justadogistaken commented Aug 9, 2022

codecov-commenter commented Aug 10, 2022 •

edited

Loading

justadogistaken Aug 11, 2022

justadogistaken commented Aug 13, 2022

allencloud commented Aug 15, 2022

justadogistaken commented Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022 •

edited

Loading

allencloud Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022

justadogistaken Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022

justadogistaken Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022

allencloud Aug 15, 2022 •

edited

Loading

allencloud Aug 15, 2022

allencloud commented Aug 15, 2022

Feature: build cluster image with buildah #1611

Feature: build cluster image with buildah #1611

Conversation

justadogistaken commented Jul 28, 2022 • edited Loading

Describe what this PR does / why we need it

justadogistaken commented Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

justadogistaken commented Aug 9, 2022

codecov-commenter commented Aug 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

justadogistaken commented Aug 13, 2022

allencloud commented Aug 15, 2022

justadogistaken commented Aug 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Aug 15, 2022

justadogistaken commented Jul 28, 2022 •

edited

Loading

justadogistaken commented Jul 29, 2022 •

edited

Loading

codecov-commenter commented Aug 10, 2022 •

edited

Loading

allencloud Aug 15, 2022 •

edited

Loading

allencloud Aug 15, 2022 •

edited

Loading