-
Notifications
You must be signed in to change notification settings - Fork 537
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 efs-utils.conf in-place instead of overwriting #988
Modify efs-utils.conf in-place instead of overwriting #988
Conversation
Hi @adammw. 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 Once the patch is verified, the new status will be reflected by the 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adammw The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
a98e716
to
53fd5b0
Compare
/ok-to-test |
/re-test |
/retest |
53fd5b0
to
b5bf9f4
Compare
b5bf9f4
to
5302323
Compare
5302323
to
70087e8
Compare
Tests passing, ready for review @Ashley-wenyizha |
efsCfgTemplate := template.Must(template.New("efs-utils-config").Parse(efsUtilsConfigTemplate)) | ||
f, err := os.Create(filepath.Join(w.efsUtilsCfgPath, efsUtilsConfigFileName)) | ||
filePath := filepath.Join(w.efsUtilsCfgPath, efsUtilsConfigFileName) | ||
cfg, err := ini.Load(filePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this throw an error if the file doesn't exist?
What about this change allows users to modify efs-utils.conf differently from how they would before? It seems like either way, users would still need to ssh into their containers and directly modify this file. I do not see any functional differences with this PR, but I might be misunderstanding the changes here. |
The difference is previously the binary would write out this file every time, making it impossible to customise before the binary has started. In contrast, users wishing to customise the config can now with this change either create a custom Dockerfile or entrypoint that modifies the config, and that modified config will be used rather than overwritten. Executing into the container and trying to modify the file after the fact doesn't work for mounts that have already been established before modification, therefore this was needed for us to debug issues in one of our persistent EFS mounts. |
PR needs rebase. 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. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
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. |
Is this a bug fix or adding new feature?
New Feature
What is this PR about? / Why do we need it?
Modify efs-utils.conf in-place instead of overwriting.
Expects the
/etc/amazon/efs-static-files/efs-utils.conf
file to exist and is copied over if the efs-utils.conf does not exist.Introduces
gopkg.in/ini.v1
ini parser to parse and rewrite the config file. Note that the rewritten file may have slightly different formatting but otherwise should be functionally identical.Allows users to customise efs-utils.conf to enable debug logging or changing other settings without the driver overwriting the entire file.
Fixes #737
What testing is done?
Tested on a fork of release-1.4 in our own clusters.