Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Add log to dump kv_str #1388

Merged
merged 5 commits into from
Jun 12, 2017
Merged

Add log to dump kv_str #1388

merged 5 commits into from
Jun 12, 2017

Conversation

lipingxue
Copy link
Contributor

This PR includes:

  1. add log to dump kv_str to help reproduce Metadata load error from volume KV during access update for the volume. #1371
  2. set "policyList" to empty in SetupTest for VsanTestSuite, which is the left over from my previous merged PR.

@@ -15,7 +15,7 @@
// The goal of this test suite is to verify read/write consistency on volumes
// in accordance with the access updates on the volume

// +build runonce
// +build runalways
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the default and hence need not be specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specified explicitly is more clear, and I found "basic_test.go" also specify "runalways".

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we do need to add "runalways" explicitly - @shuklanirdesh82 can you please explain the difference between default (no tag) and "runalways"?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a limitation of go test -tags <tag_name> command. There is no way to exclude any tag, as per -tags tests are picked up with passed value and run. Any test files not labeled with tag falls into default category and end up running multiple times hence tag is required.

Let me explain with following example. let's say we have 3 files a_test.go, b_test.go, c_test.go.

a_test.go => // +build runalways
b_test.go => // +build runonce
c_test.go => no label 

We have two targets runalways and runonce (https://github.com/vmware/docker-volume-vsphere/blob/master/vmdk_plugin/Makefile#L357).

targets runalways: includes and ends up running a_test.go & c_test.go
targets runonce: includes and ends up running b_test.go & c_test.go

c_test.go falls into default and ends up running multiple times hence the tag explicitly needed.

@@ -277,6 +277,8 @@ def load(volpath):
return None

try:
# Adding this log for DEBUG
logging.warning("kv_str from meta file is %s ", kv_str)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should print the json formatted text instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the log so that it only print when the "kv_str" is not a json file.

@@ -57,6 +57,10 @@ func (s *VsanTestSuite) SetUpSuite(c *C) {
}
}

func (s *VsanTestSuite) SetUpTest(c *C) {
s.policyList = []string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: As this is cleaning the list, this step should go to teardown.
You can empty this slice as s.policyList = s.policyList[:0]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it in the setup is to make sure each test is running from a clean state. I think either way works. So I will keep it as it is.

Copy link
Contributor

@shaominchen shaominchen left a comment

Choose a reason for hiding this comment

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

LGTM.

@lipingxue lipingxue merged commit f5dae96 into master Jun 12, 2017
@shuklanirdesh82 shuklanirdesh82 deleted the runci/load_meta.liping branch June 13, 2017 04:56
shuklanirdesh82 added a commit to shuklanirdesh82/vsphere-storage-for-docker that referenced this pull request Jun 17, 2017
1. Adding --rm flag while mounting volume so that containers is removed automatically
2. Test code cleanup while addressing #1
3. Reverting extra logging change added as part of vmware-archive#1388
shuklanirdesh82 added a commit to shuklanirdesh82/vsphere-storage-for-docker that referenced this pull request Jun 17, 2017
1. Adding --rm flag while mounting volume so that containers is removed automatically
2. Test code cleanup while addressing #1
3. Reverting extra logging change added as part of vmware-archive#1388
shuklanirdesh82 added a commit that referenced this pull request Jun 17, 2017
1. Adding --rm flag while mounting volume so that containers is removed automatically
2. Test code cleanup while addressing #1
3. Reverting extra logging change added as part of #1388
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants