Bug 1795696: templates: etcd-member: setup environment variables needed for easy etcdctl execution #1429

retroflexer · 2020-02-01T21:16:51Z

Closes: #1795696

One can easily update .bashrc or .bash_profile using postStart hooks. However, oc rsh apparently doesn't use bash, so these are not getting automatically invoked.

openshift/origin#7514
openshift/origin#7496

openshift/origin#7519

However, standard shell (Bourne shell) uses .profile. But in RHCOS, /bin/sh is symbolically linked to /usr/bin/bash.

If bash is invoked with the name sh, it tries to mimic the startup behavior of historical versions of sh as closely as possible, while conforming to the POSIX standard as well. [...] When invoked as an interactive shell with the name sh, bash looks for the variable ENV, expands its value if it is defined, and uses the expanded value as the name of a file to read and execute. Since a shell invoked as sh does not attempt to read and execute commands from any other startup files, the --rcfile option has no effect.

- What I did
Added the environment variables needed for straightforward execution of etcdctl into /root/.profile, so that oc rsh has the ready-made environment.
- How to verify it

Install cluster
oc rsh -n openshift-etcd etcd-member-pod
Run etcdctl commands such as etcdctl member list

Should work right off the bat.

- Description for the changelog

Provide environment variables for straightfoward execution of etcdctl commands on pods.

openshift-ci-robot · 2020-02-01T21:16:57Z

@retroflexer: This pull request references Bugzilla bug 1795696, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1795696: templates: etcd-member: Write environment variables needed for etcdctl execution

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.

hexfusion · 2020-02-01T21:55:39Z

templates/master/00-master/_base/files/etc-kubernetes-manifests-etcd-member.yaml

+ command:
+ - /bin/sh
+ - -c
+ - echo 'export ETCDCTL_API=3 ETCDCTL_CACERT=/etc/ssl/etcd/ca.crt ETCDCTL_CERT=$(find /etc/ssl/ -name *peer*crt) ETCDCTL_KEY=$(find /etc/ssl/ -name *peer*key)' >> /root/.profile


we set API version in env below. we should do one or the other. does this work if we exec in as well?

Removed ETCDCTL_API. crictl exec works well too.

hexfusion · 2020-02-01T21:56:48Z

overall looks good, thanks @retroflexer

openshift-ci-robot · 2020-02-01T22:06:40Z

@retroflexer: This pull request references Bugzilla bug 1795696, which is valid.

In response to this:

Bug 1795696: templates: etcd-member: Write environment variables needed for etcdctl execution

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.

openshift-ci-robot · 2020-02-02T14:12:40Z

@retroflexer: This pull request references Bugzilla bug 1795696, which is valid.

In response to this:

Bug 1795696: templates: etcd-member: setup environment variables needed for easy etcdctl execution

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.

retroflexer · 2020-02-02T15:06:01Z

templates/master/00-master/_base/files/etc-kubernetes-manifests-etcd-member.yaml


 env:
 - name: ETCDCTL_API
 value: "3"
 - name: ETCD_DATA_DIR
 value: "/var/lib/etcd"
+ - name: ENV
+ value: "/root/.profile"


In RHCOS, /bin/sh is symbolically linked to bash, but it tries to mimic the behavior of historical bourne shell.

When bash is invoked as an interactive shell with the name sh, bash looks for the variable ENV, expands its value if it is defined, and uses the expanded value as the name of a file to read and execute. Since a shell invoked as sh does not attempt to read and execute commands from any other startup files (including .profile), the --rcfile option has no effect.
(from the man pages of bash)

@retroflexer thanks, for the crisp explanation. I wonder if it would be a good idea to create a secret/configmap for the env variables here and then mount the configmap? I think it would be lot cleaner and admins can configure the env variable independent of static pod yaml.

/cc @hexfusion

retroflexer · 2020-02-02T22:44:39Z

/test e2e-gcp-op

retroflexer · 2020-02-02T22:45:31Z

/test e2e-aws-disruptive

retroflexer · 2020-02-03T00:44:25Z

/test e2e-aws-disruptive

retroflexer · 2020-02-03T00:44:48Z

/test e2e-gcp-op

openshift-ci-robot · 2020-02-03T01:07:44Z

@retroflexer: This pull request references Bugzilla bug 1795696, which is valid.

In response to this:

Bug 1795696: templates: etcd-member: setup environment variables needed for easy etcdctl execution

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.

openshift-ci-robot · 2020-02-03T01:09:02Z

@retroflexer: This pull request references Bugzilla bug 1795696, which is valid.

In response to this:

Bug 1795696: templates: etcd-member: setup environment variables needed for easy etcdctl execution

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.

cgwalters · 2020-02-03T14:03:41Z

templates/master/00-master/_base/files/etc-kubernetes-manifests-etcd-member.yaml

+ command:
+ - /bin/sh
+ - -c
+ - echo 'export ETCDCTL_CACERT=/etc/ssl/etcd/ca.crt ETCDCTL_CERT=$(find /etc/ssl/ -name *peer*crt) ETCDCTL_KEY=$(find /etc/ssl/ -name *peer*key)' >> /root/.profile


I really don't think we should have starting any containers affect host content like this by default.

The etcd recovery tools are going to be containerized anyways, so let's just document this stuff over there?

The etcd recovery tools are going to be containerized in 4.5+. And it assumes cluster-etcd-operator to be available, which is not available prior to 4.4. That means, our current customers of 4.2 and 4.3 (and perhaps 4.4) will not have easy execution of etcdctl.

Also .profile is current not used by anyone (that file doesn't exist at all in most pod installations). So, I do not see a reason to oppose it. If you don't like the use of .profile, I can put it in a file named .etcdctl_env.

Sorry, I mistakenly thought these hooks ran on the host.

cgwalters · 2020-02-03T14:04:25Z

/hold
I really don't like this and think we should take another approach that doesn't involve changing root's .profile.

retroflexer · 2020-02-03T14:10:40Z

/hold
I really don't like this and think we should take another approach that doesn't involve changing root's .profile.

What other solution do you recommend, Colins?

Our customers and customer supporting staff are struggling with typing a mile long commands to achieve simple member list or endpoint status.

Michal thought this should be ported to 4.2 and 4.3 as well to help mitigate the pain for our customers.

Cc: @mfojtik, @hexfusion

mfojtik · 2020-02-04T13:14:36Z

@cgwalters i'm willing to accept this for 4.4, but revisit in 4.5 where we should make the ETCD_DNS_NAME in the cert names go away and one could just set normal pod level env vars that points to ETCDCTL_CERT/etc.
We made this hard to ourself by making the file names non-deterministic, lets drop that legacy and make them deterministic (use a pod name or something).

Also this is important when debugging broken clusters and also come up recently during one escalation call.

/cc @smarterclayton

alaypatel07 · 2020-02-04T18:41:33Z

@retroflexer @hexfusion can we maybe do this via the discovery container? for instance, we set the DNS name here[1], can we set the ETCDCTL_CERT* env variables here as well. It will be in the /run/etcd/environment and we source that anyway. The etcd container can then always have the env variable. Sorry for the late response, it hit me now.

machine-config-operator/cmd/setup-etcd-environment/run.go

Line 189 in 09fe53e

unexportedEnv["DNS_NAME"] = setupEnv.etcdDNS

alaypatel07 · 2020-02-05T01:32:28Z

we set the DNS name here[1], can we set the ETCDCTL_CERT* env variables here as well. It will be in the /run/etcd/environment and we source that anyway. The etcd container can then always have the env variable

Thanks @retroflexer for confirming over slack, this will only work if the user explicitly sources /run/etcd/environment after execing or rshing into the container, hence dropping the idea

cgwalters · 2020-02-05T20:33:15Z

/hold cancel
per #1429 (comment)

retroflexer · 2020-02-06T02:00:41Z

/test e2e-aws-disruptive

retroflexer · 2020-02-06T02:01:07Z

/test e2e-aws-scaleup-rhel7

hexfusion · 2020-02-06T12:00:32Z

The bottom line here is we are improving user experience. This is a simple and practical approach in my opinion. thanks @retroflexer

/lgtm

hexfusion · 2020-02-06T12:43:35Z

cc @cgwalters @runcom

hexfusion · 2020-02-06T12:45:53Z

/skip

cgwalters · 2020-02-10T14:59:13Z

/approve

openshift-ci-robot · 2020-02-10T15:00:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, hexfusion, retroflexer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

retroflexer · 2020-02-10T15:18:32Z

/cherrypick release-4.3

openshift-cherrypick-robot · 2020-02-10T15:18:33Z

@retroflexer: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.3

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.

openshift-ci-robot · 2020-02-10T16:44:38Z

@retroflexer: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-disruptive	`144c710`	link	`/test e2e-aws-disruptive`
ci/prow/e2e-aws-scaleup-rhel7	`144c710`	link	`/test e2e-aws-scaleup-rhel7`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

openshift-bot · 2020-02-10T16:55:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-02-10T17:08:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-02-10T18:37:23Z

@retroflexer: All pull requests linked via external trackers have merged. Bugzilla bug 1795696 has been moved to the MODIFIED state.

In response to this:

Bug 1795696: templates: etcd-member: setup environment variables needed for easy etcdctl execution

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.

openshift-cherrypick-robot · 2020-02-10T18:38:41Z

@retroflexer: new pull request created: #1452

In response to this:

/cherrypick release-4.3

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.

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 1, 2020

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 1, 2020

openshift-ci-robot requested review from ashcrow and kikisdeliveryservice February 1, 2020 21:17

hexfusion reviewed Feb 1, 2020

View reviewed changes

retroflexer changed the title ~~Bug 1795696: templates: etcd-member: Write environment variables needed for etcdctl execution~~ Bug 1795696: templates: etcd-member: setup environment variables needed for easy etcdctl execution Feb 2, 2020

retroflexer commented Feb 2, 2020

View reviewed changes

Write environment variables needed for etcdctl execution into ~/.profile

144c710

retroflexer force-pushed the for-easy-ectdctl-setup-env branch from b8a58a5 to 144c710 Compare February 2, 2020 20:45

openshift-ci-robot requested a review from hexfusion February 3, 2020 13:55

cgwalters reviewed Feb 3, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2020

openshift-ci-robot requested a review from smarterclayton February 4, 2020 13:14

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2020

openshift-ci-robot assigned hexfusion Feb 6, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2020

openshift-merge-robot merged commit d9dec38 into openshift:master Feb 10, 2020

openshift-cherrypick-robot mentioned this pull request Feb 10, 2020

[release-4.3] Bug 1801430: templates: etcd-member: setup environment variables needed for easy etcdctl execution #1452 #1452

Closed

Bug 1795696: templates: etcd-member: setup environment variables needed for easy etcdctl execution #1429

Bug 1795696: templates: etcd-member: setup environment variables needed for easy etcdctl execution #1429

Conversation

retroflexer commented Feb 1, 2020 • edited Loading

openshift-ci-robot commented Feb 1, 2020

hexfusion Feb 1, 2020 • edited Loading

Choose a reason for hiding this comment

retroflexer Feb 2, 2020

Choose a reason for hiding this comment

hexfusion commented Feb 1, 2020

openshift-ci-robot commented Feb 1, 2020

openshift-ci-robot commented Feb 2, 2020

retroflexer Feb 2, 2020 • edited Loading

Choose a reason for hiding this comment

alaypatel07 Feb 3, 2020

Choose a reason for hiding this comment

retroflexer commented Feb 2, 2020

retroflexer commented Feb 2, 2020

retroflexer commented Feb 3, 2020

retroflexer commented Feb 3, 2020

openshift-ci-robot commented Feb 3, 2020

openshift-ci-robot commented Feb 3, 2020

cgwalters Feb 3, 2020

Choose a reason for hiding this comment

retroflexer Feb 3, 2020

Choose a reason for hiding this comment

cgwalters Feb 5, 2020

Choose a reason for hiding this comment

cgwalters commented Feb 3, 2020

retroflexer commented Feb 3, 2020 • edited Loading

mfojtik commented Feb 4, 2020

alaypatel07 commented Feb 4, 2020

alaypatel07 commented Feb 5, 2020 • edited Loading

cgwalters commented Feb 5, 2020

retroflexer commented Feb 6, 2020

retroflexer commented Feb 6, 2020

hexfusion commented Feb 6, 2020

hexfusion commented Feb 6, 2020

hexfusion commented Feb 6, 2020

cgwalters commented Feb 10, 2020

openshift-ci-robot commented Feb 10, 2020

retroflexer commented Feb 10, 2020

openshift-cherrypick-robot commented Feb 10, 2020

openshift-ci-robot commented Feb 10, 2020 • edited Loading

openshift-bot commented Feb 10, 2020

openshift-bot commented Feb 10, 2020

openshift-ci-robot commented Feb 10, 2020

openshift-cherrypick-robot commented Feb 10, 2020

retroflexer commented Feb 1, 2020 •

edited

Loading

hexfusion Feb 1, 2020 •

edited

Loading

retroflexer Feb 2, 2020 •

edited

Loading

retroflexer commented Feb 3, 2020 •

edited

Loading

alaypatel07 commented Feb 5, 2020 •

edited

Loading

openshift-ci-robot commented Feb 10, 2020 •

edited

Loading