[WIP][DNM][RFC] Create a unified binary and container image #273

phlogistonjohn · 2019-03-20T19:26:32Z

Creating a unified binary and container image has come up a few times in conversations I've been involved with (generally in-person). I confirmed this was still something on the table with @travisn but it might be nice if you could confirm @rootfs.

Regardless, the PR is the start of a single image and binary. Currently, this only adds the new unified version and leaves the current items as-is. This is mainly to start the discussion and the final version would remove them.

Some points I'd like to discuss:

Currently a cli switch must be provided to select between rbd and cephfs. I was wondering it it would be nicer to select mode (also) by "argv 0" and have symlinks to the binary under the previous names such that the new binary can behave similarly to the old split binaries
Since this is early its a good time to "bikeshed" any of the naming
Does the "CEPH_VERSION" env var in the Dockerfile do anything? If not should it be removed?

Makefile

rootfs · 2019-03-21T00:56:34Z

cmd/cephcsi.go

@@ -0,0 +1,113 @@
+/*
+Copyright 2018 The Kubernetes Authors.


rootfs · 2019-03-21T00:58:35Z

cmd/cephcsi.go

+
+func getDriverName() string {
+	// was explicitly passed a driver name
+	if *driverName != "" {


if driver != nil && len(*drivername) != 0

I'll make these changes, but I'll like to understand why the preference of len(str) vs. direct comparision to "". Golang should optimize checking for the empty string, no?
The nil check I have no issues with.

rootfs · 2019-03-21T00:59:34Z

cmd/cephcsi.go

+		return *driverName
+	}
+	// select driver name based on volume type
+	switch *vtype {


if vtype == nil { return nil }

rootfs · 2019-03-21T01:00:03Z

cmd/cephcsi.go

+}
+
+func main() {
+	if *vtype == "" {


if vtype == nil || len(*vtype) == 0

rootfs · 2019-03-21T01:01:07Z

cmd/cephcsi.go

+		klog.Fatalln(err) // calls exit
+	}
+
+	switch *vtype {


switch dnam makes more sense based on the context

dname can be user specified as well, hence switching on the same may not be the right thing to do.

rootfs · 2019-03-21T01:03:34Z

also need to add the image to deploy.sh

leseb · 2019-03-21T03:47:49Z

deploy/cephcsi/image/Dockerfile

@@ -0,0 +1,17 @@
+
+FROM centos:7


Why not using ceph/ceph image instead? See: https://hub.docker.com/r/ceph/ceph/tags

I would be happy to make that change if other ceph-csi experts agree. @rootfs @ShyamsundarR @gman0 ?

@Lseb I like the general idea, but an unaware if the ceph/ceph image is unduly bigger than what we require in this case.

This image needs "ceph-common kmod ceph-fuse attr e2fsprogs xfsprogs rbd-nbd" as in the yum install below.

The ceph/ceph image seems to contain iSCSI, NFS Ganesha etc, which are not needed in this case.

Also, the image size is ~228MB as compared to the current ceph-csi images running at ~150MB (just pointing it out, not attempting to save bits and bytes at these sizes yet).

The ceph image is better from an operational standpoint, as ensuring it has the right dependencies is done by the image itself, hence more apt to use the same.

The above is what I notice, @leseb your thoughts on these and if they matter much?

@ShyamsundarR the image size, as well as the unnecessary packages, are a valid point. However, it doesn't hurt much, I'm just trying to avoid using home-made images that could use wrong sources. The images we build on ceph/ceph are Ceph's official image, supported and maintained by the Ceph team.

I just realized that our current image does not contain rbd-nbd and ceph-fuse, this can be easily fixed but I don't want to hold to PR for that. We can still fix this later.

Btw why do we need rbd-nbd, AFAIK we don't use NBD but KRBD, can someone clarify? Thanks!

@ShyamsundarR the image size, as well as the unnecessary packages, are a valid point. However, it doesn't hurt much, I'm just trying to avoid using home-made images that could use wrong sources. The images we build on ceph/ceph are Ceph's official image, supported and maintained by the Ceph team.

I agree, the extra size does not really bother me, also staying on the official image is the right option.

I just realized that our current image does not contain rbd-nbd and ceph-fuse, this can be easily fixed but I don't want to hold to PR for that. We can still fix this later.

Btw why do we need rbd-nbd, AFAIK we don't use NBD but KRBD, can someone clarify? Thanks!

NBD can be used via a mounter option provided, and so we support it. I am not sure about why we do and if we should carry it forward, but possibly a different discussion. As you have started the process of including it in the Ceph official image, it should be good for now.

ceph/ceph-container#1337 has just recently merged, which should allow us to use ceph/ceph as a base.

Not yet, we haven't built the image yet.

deploy/cephcsi/image/Dockerfile

ShyamsundarR

Broadly, I am fine with the naming and changes.

You did mention that based on the binary name the appropriate mode can be selected, I wonder how we could do that when the docker file expects an ENTRYPOINT anyway, and that is going to be common. I like the idea, but am possibly missing how we would achieve the same.

ShyamsundarR · 2019-03-27T17:28:01Z

cmd/cephcsi.go

+		klog.Fatalln(err) // calls exit
+	}
+
+	switch *vtype {


dname can be user specified as well, hence switching on the same may not be the right thing to do.

phlogistonjohn · 2019-03-27T17:46:54Z

Broadly, I am fine with the naming and changes.

You did mention that based on the binary name the appropriate mode can be selected, I wonder how we could do that when the docker file expects an ENTRYPOINT anyway, and that is going to be common. I like the idea, but am possibly missing how we would achieve the same.

I didn't actually implement that in this version but I'll do so to demonstrate what I mean and then I'll remove it if no-one likes that version. As for providing both... I think as long as our order of precedence is clear it shouldn't be much of an issue.

phlogistonjohn · 2019-03-27T18:18:11Z

I've updated the series to include switching on the name of the binary as well as most of the other suggestions made. To demonstrate, I now create symlinks within the unified container image and copy the binary to the "old" names in the split images.

I didn't change the base of the image as @leseb suggested yet because I wanted to get some buy-in from others before I did that. It would be a good change IMO though.

@leseb @Madhu-1 @rootfs @ShyamsundarR PTAL

deploy/cephcsi/image/Dockerfile

humblec · 2019-04-03T12:37:50Z

@phlogistonjohn @leseb IMO, we should not remove individual targets like rbdplugin or cephfsplugin from makefile , rather keep it as it is for someone who want to make a target separately for his requirement. It could be that, one want to only experiment one of it. Its good to have a seperate target in makefile for common binary which can be used for achieving the goal mentioned in the PR.. Thoughts ?

ktdreyer · 2019-04-03T14:13:29Z

@humblec , I think that would mean extra "experimental" code that could rot over time, because it will not be in the path that QE tests regularly.

Madhu-1 · 2019-04-03T14:23:31Z

cmd/cephcsi.go

+
+var (
+	// common flags
+	vtype           = flag.String("type", "", "driver type [rbd|cephfs]")


I think we need to update the deployment templates for this one, and also the files in deploy folder for new flags

is this a breaking change? because once PR is merged new image will be pushed.

Eventually yes, but I didn't plan to immediately deprecate the "split" images. Only new templates would be needed after the image was ready. I wasn't going to do that in this PR.

Create a single binary that can start ceph-csi in either rbd or cephfs mode. Signed-off-by: John Mulligan <jmulligan@redhat.com>

leseb

One last thing :), the image has the packages too now.

deploy/cephcsi/image/Dockerfile

ShyamsundarR

Looks clean (other than DockerFile comments by @leseb ).

Just some queries, implementation looks fine.

ShyamsundarR · 2019-04-09T13:37:26Z

Makefile

-cephfsplugin:
-	if [ ! -d ./vendor ]; then dep ensure -vendor-only; fi
-	CGO_ENABLED=0 GOOS=linux go build -a -ldflags '-extldflags "-static"' -o  _output/cephfsplugin ./cmd/cephfs
+image-rbdplugin: cephcsi


(query) I would assume the targets image-rbdplugin and image-cephfsplugin would go away eventually, as noted here

ShyamsundarR · 2019-04-09T13:44:51Z

cmd/cephcsi.go

+
+func getType() string {
+	if vtype == nil || len(*vtype) == 0 {
+		a0 := path.Base(os.Args[0])


(query) In the end the command to execute maybe set using the container:command directive in kubernetes say, would that be right?

That's one way, there's also the args parameter. These changes "don't care" how the command is invoked, just that if it is invoked as the rbd or cephfs variant it uses that type as default.

Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add rules and variables to the Makefile so that the unified binary and container image can be built. Signed-off-by: John Mulligan <jmulligan@redhat.com>

phlogistonjohn · 2019-04-09T17:59:17Z

Updated version pushed to incorporate @leseb latest comment.

Folks, please help me get a checklist of what's an absolute blocker for this PR and what's nice to have. Once I resolve the blockers I can move this out of RFC state and I can move forward with additional follow-up prs for the nice-to-haves.
@ShyamsundarR @rootfs @Madhu-1

ShyamsundarR · 2019-04-09T18:44:30Z

Updated version pushed to incorporate @leseb latest comment.

Folks, please help me get a checklist of what's an absolute blocker for this PR and what's nice to have. Once I resolve the blockers I can move this out of RFC state and I can move forward with additional follow-up prs for the nice-to-haves.
@ShyamsundarR @rootfs @Madhu-1

I have none, I am good as long as @leseb is good with the DockerFile

leseb · 2019-04-10T08:05:21Z

Thanks @phlogistonjohn for your patience 👍

Madhu-1 · 2019-04-10T08:06:12Z

@leseb someone needs to pick this to master branch

leseb · 2019-04-10T09:21:50Z

@phlogistonjohn can you please cherry-pick this into the master branch?

phlogistonjohn · 2019-04-10T13:15:33Z

@phlogistonjohn can you please cherry-pick this into the master branch?

Happy to do the work but not sure what needs to be done. Cherry-pick changes on top of master and then file a new PR for those?

leseb · 2019-04-10T14:03:07Z

@phlogistonjohn yes, checkout master, checkout a new branch, cherry-pick, send a PR against master.

Syncing latest changes from upstream devel for ceph-csi

rootfs reviewed Mar 21, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

rootfs reviewed Mar 21, 2019

View reviewed changes

cmd/cephcsi.go Outdated

@@ -0,0 +1,113 @@

/*

Copyright 2018 The Kubernetes Authors.

Copy link

Member

rootfs Mar 21, 2019

Choose a reason for hiding this comment

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

2019

rootfs reviewed Mar 21, 2019

View reviewed changes

cmd/cephcsi.go Outdated

}

func main() {

if *vtype == "" {

Copy link

Member

rootfs Mar 21, 2019

Choose a reason for hiding this comment

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

if vtype == nil || len(*vtype) == 0

rootfs reviewed Mar 21, 2019

View reviewed changes

leseb suggested changes Mar 21, 2019

View reviewed changes

ShyamsundarR reviewed Mar 27, 2019

View reviewed changes

phlogistonjohn force-pushed the jjm-unified-build branch from 9980d6c to e5904b4 Compare March 27, 2019 18:13

leseb suggested changes Mar 28, 2019

View reviewed changes

deploy/cephcsi/image/Dockerfile Outdated Show resolved Hide resolved

Madhu-1 reviewed Apr 3, 2019

View reviewed changes

phlogistonjohn force-pushed the jjm-unified-build branch from e5904b4 to 20fd4e3 Compare April 3, 2019 18:13

cmd: create a unified cephcsi binary

3fcb6cf

Create a single binary that can start ceph-csi in either rbd or cephfs mode. Signed-off-by: John Mulligan <jmulligan@redhat.com>

phlogistonjohn force-pushed the jjm-unified-build branch from 20fd4e3 to 66a6597 Compare April 3, 2019 18:20

leseb added the DNM DO NOT MERGE label Apr 9, 2019

leseb suggested changes Apr 9, 2019

View reviewed changes

deploy/cephcsi/image/Dockerfile Outdated Show resolved Hide resolved

ShyamsundarR reviewed Apr 9, 2019

View reviewed changes

phlogistonjohn added 2 commits April 9, 2019 13:55

deploy: create a new Dockerfile for unified cephcsi image

b8cc51f

Signed-off-by: John Mulligan <jmulligan@redhat.com>

Makefile: add initial build rules for combined binary and image

9cf82c2

Add rules and variables to the Makefile so that the unified binary and container image can be built. Signed-off-by: John Mulligan <jmulligan@redhat.com>

phlogistonjohn force-pushed the jjm-unified-build branch from 66a6597 to 9cf82c2 Compare April 9, 2019 17:55

ShyamsundarR approved these changes Apr 9, 2019

View reviewed changes

leseb approved these changes Apr 10, 2019

View reviewed changes

leseb removed the DNM DO NOT MERGE label Apr 10, 2019

mergify bot merged commit de94cb6 into ceph:csi-v1.0 Apr 10, 2019

phlogistonjohn mentioned this pull request Apr 10, 2019

Create a unified binary and container image (master) #316

Merged

ShyamsundarR mentioned this pull request May 3, 2019

Update images with ceph container as the base image #346

Merged

Madhu-1 mentioned this pull request Sep 4, 2019

Fix issue in mon accessing #586

Merged

phlogistonjohn deleted the jjm-unified-build branch April 23, 2020 15:32

nixpanic pushed a commit to nixpanic/ceph-csi that referenced this pull request Mar 27, 2024

Merge pull request ceph#273 from red-hat-storage/sync_us--devel

de17a0d

Syncing latest changes from upstream devel for ceph-csi

[WIP][DNM][RFC] Create a unified binary and container image #273

[WIP][DNM][RFC] Create a unified binary and container image #273

Conversation

phlogistonjohn commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlogistonjohn Mar 27, 2019 • 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

rootfs commented Mar 21, 2019

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

ShyamsundarR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlogistonjohn commented Mar 27, 2019

phlogistonjohn commented Mar 27, 2019

humblec commented Apr 3, 2019

ktdreyer commented Apr 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leseb left a comment

Choose a reason for hiding this comment

ShyamsundarR left a 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

phlogistonjohn commented Apr 9, 2019 • edited Loading

ShyamsundarR commented Apr 9, 2019

leseb commented Apr 10, 2019

Madhu-1 commented Apr 10, 2019

leseb commented Apr 10, 2019

phlogistonjohn commented Apr 10, 2019

leseb commented Apr 10, 2019

phlogistonjohn Mar 27, 2019 •

edited

Loading

phlogistonjohn commented Apr 9, 2019 •

edited

Loading