Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA-135612: Add experimental support for NFSv4 to SM #109

Closed

Conversation

robertbreker
Copy link
Contributor

This enables the specification of the NFS version (3,4) in the device-config of
ISOSRs(type=nfs) and NFSSRs.
E.g. device-config:nfsversion=4

Signed-off-by: Robert Breker robert.breker@citrix.com

@siddharthv
Copy link
Contributor

Can one of the admins verify this patch?

@matelakat
Copy link
Contributor

retest this please

@matelakat
Copy link
Contributor

Please ignore the build result - the check job requires #151 to be merged first

@matelakat
Copy link
Contributor

retest this please

@coveralls
Copy link

Coverage Status

Coverage increased (+0.59%) when pulling 7bc1d14277516e916835994143a370f583107611 on robertbreker:experimentalnfsv4 into b890746 on xapi-project:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.59%) when pulling 8626c3c44162d4204f93988e59b3438bf02690e0 on robertbreker:experimentalnfsv4 into b890746 on xapi-project:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.59%) when pulling 0d4524d7c7ebc311675e2e2c63df79e02c9c2f76 on robertbreker:experimentalnfsv4 into b890746 on xapi-project:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.58%) when pulling 7b9a6898151e44d4927ecca68201b46359dbcfae on robertbreker:experimentalnfsv4 into b890746 on xapi-project:master.

@robertbreker robertbreker changed the title CA-135612: Add experimental support for NFSv4 to SM CA-135612: Add experimental support for NFSv4 to SM [WiP] Jun 22, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.58%) when pulling 9b35f8cb05532a114103f7f3b9dc21d52b47d434 on robertbreker:experimentalnfsv4 into b890746 on xapi-project:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.58%) when pulling dd40b04c6e81911d3e602f1cc5fc99efdff7ac3d on robertbreker:experimentalnfsv4 into b890746 on xapi-project:master.

@robertbreker robertbreker changed the title CA-135612: Add experimental support for NFSv4 to SM [WiP] CA-135612: Add experimental support for NFSv4 to SM Jul 28, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.58%) when pulling fa12687adc15b4101453d0aad8c0b145f040b249 on robertbreker:experimentalnfsv4 into bce4501 on xapi-project:master.

@robertbreker
Copy link
Contributor Author

Hi @matelakat,

this is now ready for review.

The code is vastly cherry-picked, but:

  • With minor modification to the nfs mount options in the unit tests, as trunk looks slightly different.
  • With an additional bugfix for CA-141409 - this PR doesn't make sense without fixing this bug on trunk.

Thanks,
Robert

@coveralls
Copy link

Coverage Status

Coverage increased (+0.58%) when pulling ff824f1 on robertbreker:experimentalnfsv4 into bce4501 on xapi-project:master.

@matelakat
Copy link
Contributor

@robertbreker - Thaks for this change, it's very useful, however, it would be better to be able to pick the changes as they are in xs64bit, without any adjustment. So I used your original commits, cherry picked them on top of master, and ran the unit tests. The unit tests failed of course, so I used them to drive my decisions, and identify which changes would need to be picked to make sure that the tests pass. This is how I came to the pull request #202

It means, that cherry picking your original changes on top of #202 will yield all tests to pass without any modifications.

So I would suggest, we first get #202 merged, then we can pick the original changes without any modification.

It's again a sign of how useful those unit tests can be!

@matelakat
Copy link
Contributor

@robertbreker - is there any chance you can create a separate pull request for the CA-141409 commits?

@robertbreker
Copy link
Contributor Author

@matelakat - I wish I could, but that's unfortunately not straight forward because of dependencies.

@matelakat
Copy link
Contributor

@robertbreker - I created a new PR with your changes to xs64bit: #205

@germanop
Copy link
Contributor

What should we do with this PR?
Why we opened another one instead of waiting for Robert to update this?
Pushing into master at present is not critical.

@siddharthv
Copy link
Contributor

@robertbreker Many of the changes related to NFSv4 have been ported from xs64bit to master.
Could you please create a pull request just for CA-141409 ??

@germanop
Copy link
Contributor

Why create a new pull request instead of updating this one?
I guess some of the patches coming from xs64bit are Robert's anyway.

@siddharthv
Copy link
Contributor

True, we could reuse the same pull request and update it with only what is required for master now!

@siddharthv
Copy link
Contributor

@robertbreker I'll import only the missing chunks from the pull request, resolve conflicts and merge them onto master. I'll also close this pull request when I do the same.

Thanks

Robert Breker added 2 commits February 18, 2015 09:10
We are unintentionally using nfsversion 4 on trunk.
This is because the command mount.nfs defaults to using nfsversion 4 instead
of version 3 in our environment on trunk.
This commit specifies the nfsversion additionally in the mount options, to make
sure that we default to nfsversion 3 again, and that the default setting can
be switched intentionally.

Signed-off-by: Robert Breker <robert.breker@citrix.com>
…on 3 again

Signed-off-by: Robert Breker <robert.breker@citrix.com>
@robertbreker
Copy link
Contributor Author

I updated this PR after discussing this with @siddharthv.

@robertbreker robertbreker deleted the experimentalnfsv4 branch June 27, 2015 19:30
andrey-podko pushed a commit to andrey-podko/sm that referenced this pull request Aug 16, 2022
GitHub: closes xapi-project#109 on xapi-project/sm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants