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

Modify GCI mounter to enable NFSv3 #36610

Merged
merged 1 commit into from
Nov 23, 2016

Conversation

jingxu97
Copy link
Contributor

@jingxu97 jingxu97 commented Nov 10, 2016

In order to make NFSv3 work, mounter needs to start rpcbind daemon. This
change modify mounter's Dockerfile and mounter script to start the
rpcbind daemon if it is not running on the host.

After this change, need to make push the image and update the sha number in Changelog.


This change is Reviewable

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Nov 10, 2016
@grodrigues3 grodrigues3 removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@apelisse apelisse removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@@ -16,5 +16,5 @@ FROM ubuntu:xenial
MAINTAINER vishh@google.com

RUN apt-get update && apt-get install -y netbase nfs-common=1:1.2.8-9ubuntu12 glusterfs-client=3.7.6-1ubuntu1

ADD startmounter.sh /startmounter.sh
ENTRYPOINT ["/bin/mount"]
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to change the entrypoint

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 noticed this will be overwritten by rkt exec command. But I will change to make sure.

@@ -16,5 +16,5 @@ FROM ubuntu:xenial
MAINTAINER vishh@google.com

RUN apt-get update && apt-get install -y netbase nfs-common=1:1.2.8-9ubuntu12 glusterfs-client=3.7.6-1ubuntu1

ADD startmounter.sh /startmounter.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to update the -exec argument passed to rkt in the mounter script to point to this script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. Somehow it is not in the commit. I will add it.

@jbeda
Copy link
Contributor

jbeda commented Nov 15, 2016

@vishh -- looks like you got this. If you want my eyes let me know.

@vishh
Copy link
Contributor

vishh commented Nov 16, 2016

@jbeda Yup. I got this! Thanks!

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit f6f3eb6d522b77dfee0eef25e06e1e9f5c6ef2ea. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit f6f3eb6d522b77dfee0eef25e06e1e9f5c6ef2ea. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit f6f3eb6d522b77dfee0eef25e06e1e9f5c6ef2ea. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit f6f3eb6d522b77dfee0eef25e06e1e9f5c6ef2ea. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit f6f3eb6d522b77dfee0eef25e06e1e9f5c6ef2ea. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit f6f3eb6d522b77dfee0eef25e06e1e9f5c6ef2ea. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit f6f3eb6d522b77dfee0eef25e06e1e9f5c6ef2ea. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit f6f3eb6d522b77dfee0eef25e06e1e9f5c6ef2ea. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@jingxu97
Copy link
Contributor Author

@k8s-bot gci gke e2e test this

@jingxu97 jingxu97 force-pushed the Nov/nfsv3 branch 2 times, most recently from 68206a0 to 86fc77f Compare November 18, 2016 01:34
@jingxu97
Copy link
Contributor Author

@vishh @mtaufen I added NFSv3 test to this PR and it passed! PTAL.

echo "Starting rpcbind: /sbin/rpcbind -w"
/sbin/rpcbind -w
fi
echo `/etc/init.d/rpcbind status`
Copy link
Contributor

Choose a reason for hiding this comment

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

We will be running rpcbind even when we don''t mount nfs. Is that the intended behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this mounter script will be called only for nfs type or glusterfs, it should be ok in my option. Any concerns?

Copy link
Contributor

Choose a reason for hiding this comment

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

If glusterfs does not rely on rpcbind (I don't think it does, but would need to check) then ideally we would only start rpcbind when someone tries to mount nfs, correct? That said, if parsing the args and figuring out what fs type the user is trying to mount is too ugly, I can live with this as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Since we're pre-warming the mounter, this means we're always starting rpcbind on every node, which we shouldn't do)

Copy link
Contributor

Choose a reason for hiding this comment

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

wrt offline convo with @jingxu97 and @saad-ali we can get around the pre-warm issue by not starting rpcbind when the arg list is empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtaufen @saad-ali , modified the code to address this comment. PTAL

Copy link
Member

Choose a reason for hiding this comment

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

@mtaufen @saad-ali , modified the code to address this comment. PTAL

Am I missing something? It looks like RPC bind is started if it is not already started regardless of args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change for checking the empty input parameters is in cluster/gce/gci/mounter/mounter

config := VolumeTestConfig{
namespace: namespace.Name,
prefix: "nfs",
serverImage: "gcr.io/google_containers/volume-nfs:0.8",
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know this is testing v3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In line 421, I put /exports in the mount path, so this is testing v3. Also in serverPorts, I also have to put 20048 in serverports for v3 (which is not required by v4)

Copy link
Member

Choose a reason for hiding this comment

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

Does it use a different server image then the v4 test? Maybe we should maintain two seperate nfs-server bianries in gcr.io, one for each version?

@jingxu97 jingxu97 added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Nov 18, 2016
In order to make NFSv3 work, mounter needs to start rpcbind daemon. This
change modify mounter's Dockerfile and mounter script to start the
rpcbind daemon if it is not running on the host.

After this change, need to make push the image and update the sha number in Changelog.
@k8s-ci-robot
Copy link
Contributor

Jenkins Bazel Build failed for commit 2a8d89e. Full PR test history.

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 22, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 22, 2016
@jingxu97 jingxu97 added this to the v1.5 milestone Nov 22, 2016
echo "Starting rpcbind: /sbin/rpcbind -w"
/sbin/rpcbind -w
fi
echo `/etc/init.d/rpcbind status`
Copy link
Member

Choose a reason for hiding this comment

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

@mtaufen @saad-ali , modified the code to address this comment. PTAL

Am I missing something? It looks like RPC bind is started if it is not already started regardless of args?

config := VolumeTestConfig{
namespace: namespace.Name,
prefix: "nfs",
serverImage: "gcr.io/google_containers/volume-nfs:0.8",
Copy link
Member

Choose a reason for hiding this comment

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

Does it use a different server image then the v4 test? Maybe we should maintain two seperate nfs-server bianries in gcr.io, one for each version?

@@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

TAG=v2
TAG=v3
Copy link
Member

Choose a reason for hiding this comment

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

Did you already push this container to gcr.io?

Copy link
Member

Choose a reason for hiding this comment

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

Spoke offline. Already done.

@saad-ali saad-ali added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Nov 23, 2016
@saad-ali
Copy link
Member

LGTM

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants