Add StripVolumeRegex configuration property

[ten4o]

Add a StripVolumeRegex configuration property that will be used to strip some part of disk-id (aka volume ID) before device path resolve step, as proposed by the Working Group:

Create a configuration property for the bosh agent to configure a strip-volume-regex,
which would allow specifying a regex prefix per IaaS in the bosh stemcell builder here (in the case of Ali Cloud).

The potential benefit is that this could solve issue #310.

[beyhan]

I would prefer to have a function called like stripVolumeIfRequired which does everything and returns a striped ID or not if not required. The current implementation of stripDiskID function doesn't strip always the diskID which is not incorporated in the name. The function will need to return also error so that we fail in case the Compilation is not possible like it is implemented now.

[ten4o]

@beyhan Done. I've renamed the function made the requested changes.

[beyhan]

The change looks good. One open question is whether we need integration tests for it. I couldn't find any ITs for this kind of functionality.

[ramonskie]

as we currently only do bats testing on gcp. i don't know if this pr actually solves it for this.
but if so we can maby fit is somewhere here https://github.com/cloudfoundry/bosh-acceptance-tests/blob/master/spec/bat/bosh_helper_spec.rb#L29

[beyhan]

We want to make use of this only on AWS and Ali for now. On GCP stripping disk IDs isn't required. I was thinking for agent integration tests and not bosh.

[rkoster]

There are some integration test helpers like AttachDevice which could be helpful when adding an integration test.

[andinod]

Hi,

one question, is this change considering the Openstack Env?, because I have a similar issue described here #310 (comment) and I would like to know if I need to wait or there is a workaround that I have to look for.

Thanks in advance,

David

[beyhan]

Hi @andinod,
this change will enable us to configure a prefix which could be removed from the disk ID on the different IaaS providers. That means configuration like this will be required for an IaaS provider so that this can have an affect as discussed here.

[beyhan]

Regarding the IT discussion:
I think that it make sense to add a test case to the AssociateDisk tests in the linux_platform_test.go. The settings should look like this so that the right provider is selected here. Additionally, we should configure a strip value and verify that the ID resolution worked. @jpalermo, @rkoster what us your opinion on this? Are the latest agent integration tests executable locally because our agent dev setup is not clear about this and I didn't have time to try it?

[beyhan]

Please try to add the integration test as I described it in my previous comment.

[ten4o]

@beyhan After examining AssociateDisk tests, it seems that those are the unit tests of linux_platform.go - all test cases in that file are using FakeDevicePathResolver which will not call our code in GetRealDevicePath. It seems that there is not much value in hacking together additional test cases in AssociateDisk when we can already add similar test cases in identity_device_path_resolver_test.go.

[beyhan]

Ok, I looked into the wrong tests. Looking into the right tests I think an integration test will be challenging to implement here because the agent handles the resolution internally during the update_setttings action. The only way to validate from outside that the id_device_path_resolver.go provider was used, is via logs which is error prone.

[beyhan]

We agreed to go without integration tests.