change config dir location #286

webern · 2020-12-07T22:38:53Z

Is this a bug fix or adding new feature?

Fixes #246
Replaces #247

What is this PR about? / Why do we need it?

change config dir location

Before this change, the driver fails to work on Bottlerocket because it
tries to write files into the host directory /etc/amazon/efs. On
Bottlerocket that directory is not writable by the container.

We want to move the host mount point to /var/amazon/efs, but we need to
support backward compatibility with hosts that may already have configs
written to /etc/amazon/efs.

To solve this, we mount both host paths and check for the presence of
config files at container runtime. If we find files in the old location,
we symlink to that location, otherwise we symlink to the new location.

What testing is done?

I built my changes to a container image, pushed to a private ECR repo, and changed yaml files accordingly to use my changes. Throughout, I used a canary application from these instructions https://docs.aws.amazon.com/eks/latest/userguide/efs-csi.html which mounts an EFS volume and logs to it every five seconds.

First, I deployed the current production version to a cluster that had one AL2 node and one Bottlerocket node.
- I observed that the deployment failed on Bottlerocket and succeeded on AL2.
- I ssh'ed to the AL2 box and confirmed that files had been written to /etc/amazon/efs.
- I confirmed that the canary application was writing logs to the EFS mount, but only on AL2, Bottlerocket was broken.
Next I deployed my updated version of the efs driver.
- I observed that pods on both hosts were now healthy and writing to their EFS mounts.
- I ssh'ed to AL2 and found that /etc/amazon/efs was still being used and /var/amazon/efs was empty.
- I ssh'ed to Bottlerocket and found that /etc/amazon/efs was empty and `/var/amazon/efs/ was being used.
I rebooted both nodes and found that when the pods resumed, they were still able to write to their EFS mounts.
I also deployed my changed driver into a clean cluster and observed that both AL2 and Bottlerocket used /var/amazon/efs and that mounts were working.

k8s-ci-robot · 2020-12-07T22:38:56Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2020-12-07T22:39:01Z

Welcome @webern!

It looks like this is your first PR to kubernetes-sigs/aws-efs-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-efs-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-12-07T22:39:01Z

Hi @webern. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

webern · 2020-12-08T20:41:00Z

webern force-pushed the webern:bottlerocket branch from 78d281c to d31f20d

This push fixes the author and a couple of comments. I'm still figuring out the paperwork for the CLA.

webern · 2020-12-11T20:08:16Z

webern force-pushed the webern:bottlerocket branch from d31f20d to dcee1e2

Hopefully this push will get me passed the CLA?

wongma7 · 2020-12-11T21:50:09Z

/check-cla
/ok-to-test

wongma7 · 2020-12-11T22:07:58Z

/check-cla

webern · 2020-12-11T22:12:24Z

I signed it

webern · 2020-12-11T22:49:07Z

/check-cla

webern · 2020-12-11T22:49:26Z

I signed it

webern · 2020-12-11T23:44:28Z

/check-cla

webern · 2020-12-11T23:50:22Z

/retest

webern · 2020-12-12T01:50:40Z

Hmmm, OK, I may need some help understanding why the e2e tests are failing. Or maybe a walkthrough of what's actually happening. e.g. does a container with my Go changes get built and used in the test?

webern · 2020-12-14T22:00:00Z

webern force-pushed the webern:bottlerocket branch from dcee1e2 to 7fc9f51

This push brings the helm chart inline with the other podspec.

webern force-pushed the webern:bottlerocket branch from 7fc9f51 to e7cc126

This push raises code coverage to 100% for the new function.

webern · 2020-12-15T00:29:45Z

/retest

helm/templates/daemonset.yaml

wongma7 · 2021-01-04T21:02:40Z

pkg/driver/config_dir_test.go

+	}
+}
+
+// TestInitConfigDirPreferredError asserts that a symlink an error is returned when a symlink cannot be created to the


asserts that ~~a symlink~~ an error is returned...

deploy/kubernetes/base/node.yaml

wongma7 · 2021-01-06T05:38:26Z

Ok, got it, since the controller makes no reference to any config directories, the new function fails like this:

I0106 05:34:47.978762       1 config_dir.go:72] Creating symlink from '/etc/amazon/efs' to '/var/amazon/efs'
F0106 05:34:47.978839       1 main.go:60] config directory '/var/amazon/efs' does not exist: 'stat /var/amazon/efs: no such file or directory'

I guess it should be oky to remove the check and just create preferredDir /var/amazon/efs if it doesn't exist? In the node setting we can assume that it will be created by hostpath DirectoryOrCreate, in controller setting we don't care about persisting configs at all.

@kbasv since the controller should only briefly mount efs volumes to delete them is it necessary for its config and such to be persistent? I am saying here no, but could you please confirm.

kbasv · 2021-01-06T16:33:16Z

@wongma7 Controller doesn't require persistent configs

webern · 2021-01-06T19:22:12Z

I guess it should be oky to remove the check and just create preferredDir /var/amazon/efs if it doesn't exist? In the node setting we can assume that it will be created by hostpath DirectoryOrCreate, in controller setting we don't care about persisting configs at all.

OK, If I understand correctly, we also need to run in an environment where the mounts that I was expecting are not there. In that case we probably want the contents of /etc/amazon/efs-static-files to be available at /etc/amazon/efs. I think the simplest thing to do would be to create a symlink /etc/amazon/efs -> /etc/amazon/efs-static-files when neither of the mounted directories are present.

wongma7 · 2021-01-06T19:29:50Z

@webern yes that makes sense to me

webern · 2021-01-07T01:33:53Z

This has most of my changes. A simplification of the config dir function to account for runtimes where legacy and preferred host paths are not mounted:

webern force-pushed the webern:bottlerocket branch from 2e6f31a to 216e3f8 9 minutes ago

I missed a couple of refactor opportunities in the tests and added them in this push:

webern force-pushed the webern:bottlerocket branch from 216e3f8 to ec2d8df 2 minutes ago

I rebased and fixed conflicts in main.go:

webern force-pushed the webern:bottlerocket branch from ec2d8df to 5ccb581 2 minutes ago

webern · 2021-01-07T01:39:07Z

webern force-pushed the webern:bottlerocket branch from 5ccb581 to 45acb17 20 seconds ago

I can't make os.CreateDirAll fail reliably in any environment, so I deleted that test. Unfortunately I can't give you 100% coverage as that error return cannot reasonably be exercised.

webern · 2021-01-07T01:41:12Z

@webern yes that makes sense to me

Actually the right solution was to create an empty dir since the configs will be copied into it.

wongma7 · 2021-01-07T02:50:48Z

pkg/driver/config_dir_test.go

+	defer cleanup(t, dir)
+
+	// create legacy dir and a fake pre-existing conf file
+	// create an empty legacy dir


nit extra comment here

wongma7 · 2021-01-07T02:55:17Z

/lgtm

beautiful, thank you

Next steps for me:

Test the upgrade scenario end-to-end w/ both helm and kustomize. (cc Added e2e test for driver version upgrade #213)
Make a helm chart and release of the driver. Probably will do minor version bump.

Before this change, the driver failed to work on Bottlerocket because it tries to write files into the host directory /etc/amazon/efs. On Bottlerocket that directory is not writable by the container. We want to move the host mount point to /var/amazon/efs, but we need to support backward compatibility with hosts that may already have configs written to /etc/amazon/efs. To solve this, we mount both host paths and check for the presence of config files at container runtime. If we find files in the old location, we symlink to that location, otherwise we symlink to the new location.

webern · 2021-01-07T03:26:43Z

I fixed the incorrect comment. We needed a retest anyway. The 12 failed tests looked like timeouts, which is what I remember failed tests looking like in the past when a retest passed. Unfortunately fixing that comment removed the lgtm flag.

wongma7 · 2021-01-07T03:28:35Z

/lgtm

Yes, just checked logs too and this time the containers all became ready so I don't think it has anything to do with this PR. will repro locally tomorrow if happens again

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 7, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 7, 2020

k8s-ci-robot requested review from jqmichael and leakingtapan December 7, 2020 22:39

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 7, 2020

webern force-pushed the bottlerocket branch from 78d281c to d31f20d Compare December 8, 2020 20:39

webern force-pushed the bottlerocket branch from d31f20d to dcee1e2 Compare December 11, 2020 20:07

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 11, 2020

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 11, 2020

webern force-pushed the bottlerocket branch 2 times, most recently from 7fc9f51 to e7cc126 Compare December 14, 2020 21:56

wongma7 reviewed Jan 4, 2021

View reviewed changes

helm/templates/daemonset.yaml Show resolved Hide resolved

wongma7 reviewed Jan 4, 2021

View reviewed changes

k8s-ci-robot assigned wongma7 Jan 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 6, 2021

wongma7 reviewed Jan 6, 2021

View reviewed changes

deploy/kubernetes/base/node.yaml Show resolved Hide resolved

webern force-pushed the bottlerocket branch from 2e6f31a to 216e3f8 Compare January 7, 2021 01:23

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 7, 2021

webern force-pushed the bottlerocket branch 2 times, most recently from ec2d8df to 5ccb581 Compare January 7, 2021 01:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2021

webern force-pushed the bottlerocket branch from 5ccb581 to 45acb17 Compare January 7, 2021 01:37

wongma7 reviewed Jan 7, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2021

webern force-pushed the bottlerocket branch from 45acb17 to 6d4b96d Compare January 7, 2021 03:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2021

k8s-ci-robot merged commit 6ad12c1 into kubernetes-sigs:master Jan 7, 2021

webern mentioned this pull request Jan 7, 2021

[EKS] unable to deploy the aws-efs-csi-driver bottlerocket-os/bottlerocket#1111

Closed

wongma7 mentioned this pull request Jan 7, 2021

deploy/kubernetes/base/node.yaml: Fixing deployment on Bottlerocket. #247

Closed

webern mentioned this pull request May 27, 2021

Support NFS on EKS bottlerocket-os/bottlerocket#1225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change config dir location #286

change config dir location #286

webern commented Dec 7, 2020

k8s-ci-robot commented Dec 7, 2020

k8s-ci-robot commented Dec 7, 2020

k8s-ci-robot commented Dec 7, 2020

webern commented Dec 8, 2020

webern commented Dec 11, 2020

wongma7 commented Dec 11, 2020

wongma7 commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 12, 2020

webern commented Dec 14, 2020

webern commented Dec 15, 2020

wongma7 Jan 4, 2021

wongma7 commented Jan 6, 2021

kbasv commented Jan 6, 2021

webern commented Jan 6, 2021

wongma7 commented Jan 6, 2021

webern commented Jan 7, 2021

webern commented Jan 7, 2021

webern commented Jan 7, 2021

wongma7 Jan 7, 2021

wongma7 commented Jan 7, 2021

webern commented Jan 7, 2021

wongma7 commented Jan 7, 2021

change config dir location #286

change config dir location #286

Conversation

webern commented Dec 7, 2020

k8s-ci-robot commented Dec 7, 2020

k8s-ci-robot commented Dec 7, 2020

k8s-ci-robot commented Dec 7, 2020

webern commented Dec 8, 2020

webern commented Dec 11, 2020

wongma7 commented Dec 11, 2020

wongma7 commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 11, 2020

webern commented Dec 12, 2020

webern commented Dec 14, 2020

webern commented Dec 15, 2020

wongma7 Jan 4, 2021

Choose a reason for hiding this comment

wongma7 commented Jan 6, 2021

kbasv commented Jan 6, 2021

webern commented Jan 6, 2021

wongma7 commented Jan 6, 2021

webern commented Jan 7, 2021

webern commented Jan 7, 2021

webern commented Jan 7, 2021

wongma7 Jan 7, 2021

Choose a reason for hiding this comment

wongma7 commented Jan 7, 2021

webern commented Jan 7, 2021

wongma7 commented Jan 7, 2021