refector: refect the pouch's volume module #2379

rudyfly · 2018-10-29T09:13:02Z

Ⅰ. Describe what this PR did

don't return error when create volume is exist.
Refect the VolumeID to VolumeContext.
remove unuse field Selectors in VolumeContext.
set directory's quota id after check the limit of device.
get volume's size in Extra field.
add more options to set volume's size, you also can use "-o size=10g" or
"-o Size=10g" or "-o opt.Size=10g" to set volume's size.
merge volume's error into pkg/errtypes
remove unuse package pkg/serializer

Ⅱ. Does this pull request fix one issue?

NONE

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

Added TestToStringMap

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang rudyflyzhang@gmail.com

codecov · 2018-10-29T09:13:03Z

Codecov Report

Merging #2379 into master will increase coverage by 11.57%.
The diff coverage is 49.37%.

@@             Coverage Diff             @@
##           master    #2379       +/-   ##
===========================================
+ Coverage   57.26%   68.83%   +11.57%     
===========================================
  Files         277      276        -1     
  Lines       18227    18252       +25     
===========================================
+ Hits        10437    12564     +2127     
+ Misses       6582     4271     -2311     
- Partials     1208     1417      +209

Flag	Coverage Δ
#criv1alpha1test	`31.55% <3.34%> (-0.07%)`	⬇️
#criv1alpha2test	`35.53% <25.1%> (-0.21%)`	⬇️
#integrationtest	`40.1% <41%> (?)`
#nodee2etest	`32.98% <4.6%> (-0.14%)`	⬇️
#unittest	`26.58% <22.05%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
pkg/errtypes/errors.go	`100% <ø> (+38.09%)`	⬆️
storage/volume/types/meta/meta.go	`0% <ø> (ø)`	⬆️
storage/volume/driver/fake_driver.go	`50% <0%> (ø)`	⬆️
storage/volume/modules/tmpfs/tmpfs.go	`12.82% <0%> (-3.31%)`	⬇️
storage/quota/prjquota.go	`27.51% <0%> (ø)`	⬆️
storage/quota/grpquota.go	`0% <0%> (ø)`	⬆️
pkg/utils/utils.go	`83.82% <100%> (+4.03%)`	⬆️
storage/volume/types/volume_util.go	`85.71% <100%> (+23.21%)`	⬆️
storage/volume/modules/local/local.go	`76.38% <100%> (+12.75%)`	⬆️
storage/quota/quota.go	`45.04% <16.12%> (-1.91%)`	⬇️
... and 92 more

allencloud · 2018-10-29T11:49:15Z

daemon/mgr/container_storage.go

-				}
+			volume, err := mgr.VolumeMgr.Get(ctx, name)
+			if err != nil || volume == nil {
+				logrus.Errorf("failed to get volume: (%s), err: (%v)", name, err)


Could you use the same way to construct log and error message?
I found that in L178, you use logrus.Errorf("failed to bind volume(%s): %v", name, err), but here logrus.Errorf("failed to get volume: (%s), err: (%v)", name, err).
How about that?

weird... still not changed

allencloud · 2018-10-29T11:53:57Z

storage/volume/types/volume.go

@@ -193,21 +200,25 @@ func (v *Volume) CreateTime() string {

 // VolumeID use to define the volume's identity.
 type VolumeID struct {


Could we change the name of VolumeID？It is so strange.
Is this that this VolumeID will be stored in some legacy db?

I have change the name to VolumeContext.

allencloud · 2018-10-29T11:57:11Z

storage/quota/quota.go

+	}
+	id, err := GetNextQuotaID()
+	if err != nil {
+		return 0, errors.Wrapf(err, "failed to get file: (%s) quota id", dir)


please remove :

allencloud · 2018-10-29T11:57:21Z

storage/quota/quota.go

@@ -181,18 +196,20 @@ func GetDefaultQuota(quotas map[string]string) string {
 func SetRootfsDiskQuota(basefs, size string, quotaID uint32) (uint32, error) {
 	overlayMountInfo, err := getOverlayMountInfo(basefs)
 	if err != nil {
-		return 0, fmt.Errorf("failed to get overlay mount info: %v", err)
+		return 0, errors.Wrapf(err, "failed to get overlay:(%s) mount info", basefs)


please remove :
and could you fix that in all of the pull request?

allencloud · 2018-10-29T12:05:43Z

daemon/mgr/volume.go

@@ -83,6 +83,9 @@ func (vm *VolumeManager) Create(ctx context.Context, name, driver string, option

 	v, err := vm.core.CreateVolume(id)
 	if err != nil {
+		if e, ok := err.(*volerr.CoreError); ok && e.IsVolumeExisted() {


Could we have a way to combine pkg/errtypes and storage/volume/error into one?
Or extract the common part? @rudyfly

I have merged them.

allencloud · 2018-10-31T17:00:51Z

storage/volume/core.go

@@ -159,43 +159,43 @@ func (c *Core) getVolumeDriver(id types.VolumeID) (*types.Volume, driver.Driver,
 }

 // existVolume return 'true' if volume be found and not errors.
-func (c *Core) existVolume(id types.VolumeID) (bool, error) {
-	_, err := c.getVolume(id)
+func (c *Core) existVolume(id types.VolumeContext) (*types.Volume, bool, error) {


I am wondering if it is necessary to update the return parameter of this function. I saw that you added an instance of *types.Volume in the function.

When I searched the code, I found that this is used in core.CreateVolume, and this *types.Volume is only used when the input volume ID exists, then you returned the volume.

I think with the change, you break the primitives of this function.

As an encapsulated package, we should make the function to be focused as much as possible. when checking if it exists, the main part is the returned boolean, if the returned error is necessary, it is ok.

But when we return a Volume instance, it combines two actions into a single one, to break the purity of this function existVolume.

If the caller need the types.Volume instance when it is already exists, please let it use another function to get it, like core.GetVolumes(...).

WDYT? @rudyfly

In addition, would you mind changing function name into isVolumeExist?

good idea, I fix it.

allencloud · 2018-10-31T17:08:41Z

For user's input size, Size, opt.Size or others, could we first convert them into bytes which is a type of integer. So all the value dealing is all about integer rather rather string.

When to the lower part where we must to convert them into string, then we do the convertion.

Otherwise the string dealing all along the path may lead to some unreadability or some potential bugs. WDYT? @rudyfly

HusterWan · 2018-11-01T05:13:15Z

daemon/mgr/volume.go

-		Options:   map[string]string{},
-		Selectors: map[string]string{},
-		Labels:    map[string]string{},
+	id := types.VolumeContext{


change VolumeID to VolumeContext, feel better.

HusterWan · 2018-11-01T05:18:19Z

storage/volume/types/volume.go

 func (v *Volume) Size() string {
-	return v.Spec.Size
+	if v.Spec.Size != "" {


Can we add some verification for the Size in API interface?

No need 😄

zhuangqh · 2018-11-01T08:27:49Z

please also fix this issue #2403 thanks

rudyfly · 2018-11-02T03:10:57Z

@zhuangqh
fixed, please see this: https://github.com/alibaba/pouch/pull/2379/files#diff-10877fe186b33251a42f1b8719753b49R205

rudyfly · 2018-11-02T03:15:01Z

@allencloud It's hard to change the type of size now, because we have store it into boltdb.

1. don't return error when create volume is exist. 2. Refect the `VolumeID` to `VolumeContext`. 3. remove unuse field `Selectors` in `VolumeContext`. 4. set directory's quota id after check the limit of device. 5. get volume's size in `Extra` field. 6. add more options to set volume's size, you also can use "-o size=10g" or "-o Size=10g" or "-o opt.Size=10g" to set volume's size. 7. merge volume's error into `pkg/errtypes` 8. remove unuse package `pkg/serializer` Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>

HusterWan · 2018-11-07T07:46:42Z

pkg/utils/utils.go

+
+// ToStringMap changes the map[string]interface{} to map[string]string,
+// If the interface is not string, it will be ignore.
+func ToStringMap(in map[string]interface{}) map[string]string {


feel weird, enough though I can not figure out a more proper solution.

HusterWan · 2018-11-07T07:53:13Z

storage/volume/types/volume_util.go

@@ -45,9 +30,8 @@ func NewVolumeFromID(mountPath, size string, id VolumeID) *Volume {
 			ModifyTimestamp:   &now,
 		},
 		Spec: &VolumeSpec{
-			Backend:  id.Driver,
-			Extra:    id.Options,
-			Selector: make(Selector, 0),


Why delete the Selector?

HusterWan · 2018-11-07T10:12:48Z

Since I still have some comments, but I think them not stuck for merge this PR,
So LGTM, thanks for your works, Boss Ku !

HusterWan · 2018-11-07T10:13:16Z

lgtm

pouchrobot added areas/storage kind/bug This is bug report for project size/XL labels Oct 29, 2018

allencloud reviewed Oct 29, 2018

View reviewed changes

rudyfly force-pushed the master_remotevolume branch from 0fccb7f to 6823770 Compare October 30, 2018 13:51

pouchrobot added the conflict/needs-rebase label Oct 31, 2018

rudyfly force-pushed the master_remotevolume branch from 6823770 to 847943c Compare October 31, 2018 05:51

pouchrobot removed the conflict/needs-rebase label Oct 31, 2018

allencloud reviewed Oct 31, 2018

View reviewed changes

HusterWan reviewed Nov 1, 2018

View reviewed changes

rudyfly force-pushed the master_remotevolume branch 2 times, most recently from 606c718 to e2af333 Compare November 2, 2018 03:09

rudyfly force-pushed the master_remotevolume branch from e2af333 to 660386b Compare November 5, 2018 02:42

pouchrobot added size/XXL and removed size/XL labels Nov 5, 2018

rudyfly force-pushed the master_remotevolume branch from 660386b to ca90c83 Compare November 5, 2018 02:47

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Nov 5, 2018

rudyfly force-pushed the master_remotevolume branch from ca90c83 to 592572d Compare November 5, 2018 06:28

pouchrobot removed the conflict/needs-rebase label Nov 5, 2018

rudyfly force-pushed the master_remotevolume branch 6 times, most recently from 13088e5 to 1469d80 Compare November 7, 2018 07:41

rudyfly changed the title ~~bugfix: fix some volume bugs~~ refector: refect the pouch's volume module Nov 7, 2018

rudyfly force-pushed the master_remotevolume branch from 1469d80 to de46416 Compare November 7, 2018 07:55

HusterWan reviewed Nov 7, 2018

View reviewed changes

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 7, 2018

HusterWan merged commit c7a568e into AliyunContainerService:master Nov 7, 2018

pouchrobot mentioned this pull request Nov 9, 2018

WeeklyReport of PouchContainer from 2018-11-02 to 2018-11-09 #2457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refector: refect the pouch's volume module #2379

refector: refect the pouch's volume module #2379

rudyfly commented Oct 29, 2018 •

edited

Loading

codecov bot commented Oct 29, 2018 •

edited

Loading

allencloud Oct 29, 2018

rudyfly Nov 5, 2018

HusterWan Nov 7, 2018

allencloud Oct 29, 2018

rudyfly Oct 31, 2018

allencloud Oct 29, 2018

rudyfly Nov 5, 2018

allencloud Oct 29, 2018 •

edited

Loading

allencloud Oct 29, 2018

rudyfly Nov 7, 2018

allencloud Oct 31, 2018

rudyfly Nov 2, 2018

allencloud commented Oct 31, 2018

HusterWan Nov 1, 2018

rudyfly Nov 2, 2018

HusterWan Nov 1, 2018

rudyfly Nov 2, 2018

zhuangqh commented Nov 1, 2018

rudyfly commented Nov 2, 2018

rudyfly commented Nov 2, 2018

HusterWan Nov 7, 2018

HusterWan Nov 7, 2018

HusterWan commented Nov 7, 2018

HusterWan commented Nov 7, 2018

		@@ -193,21 +200,25 @@ func (v *Volume) CreateTime() string {

		// VolumeID use to define the volume's identity.
		type VolumeID struct {

refector: refect the pouch's volume module #2379

refector: refect the pouch's volume module #2379

Conversation

rudyfly commented Oct 29, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Oct 29, 2018 • edited Loading

Codecov Report

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 Oct 29, 2018 • 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

allencloud commented Oct 31, 2018

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

zhuangqh commented Nov 1, 2018

rudyfly commented Nov 2, 2018

rudyfly commented Nov 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HusterWan commented Nov 7, 2018

HusterWan commented Nov 7, 2018

rudyfly commented Oct 29, 2018 •

edited

Loading

codecov bot commented Oct 29, 2018 •

edited

Loading

allencloud Oct 29, 2018 •

edited

Loading