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

Adding etcdctl and k9s install on demand script to make usage easier #102

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

mimiteto
Copy link
Contributor

@mimiteto mimiteto commented Mar 11, 2024

What this PR does / why we need it:
This PR should make a bit easier for persons who troubleshoot to install etcdctl.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
I mostly copied/adapted things based on the existing install script and the same script for wireguard.
To check if the modification works as expected I did a local build of the image and tried installing and using etdctl.

Release note:

`etcdctl` command is now available within the `ops-toolbelt` image. Upon first invocation it will be installed and will be fully usable.
`etcdctl` is also trying to guess the parameter values for connectivity to the `etcd` instance.
`k9s` command is now available within the `ops-toolbelt` image. Upon first invocation it will be installed and will be fully usable.
On demand installations now work with any user within the image.

@mimiteto mimiteto requested a review from a team as a code owner March 11, 2024 16:06
@gardener-robot gardener-robot added needs/review Needs review size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Mar 11, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 11, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 11, 2024
@petersutter petersutter added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 11, 2024
@petersutter
Copy link
Member

Why don't we just install the latest version instead of asking the user?

@mimiteto
Copy link
Contributor Author

I just took two current scripts - dotfiles/.install_on_demand/.wireguard and hacks/install_etcdctl.
In the hacks script there is an option for the version, that's why it's transformed to a question (that's how it's handled in the other scripts).
Just a note, after discussing with @plkokanov - current PR will not be actually that useful.
If you decide to use the ops-toolbelt with non-root user you actually don't have access to dotfiles, so additional change is needed (for example, loading dotfiles in /etc/bashrc).
I still haven't gotten around to check this approach.

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 13, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 13, 2024
@petersutter
Copy link
Member

In the hacks script there is an option for the version, that's why it's transformed to a question (that's how it's handled in the other scripts).

I think we should just install the latest version, if the user runs etcdctl or wg. If they want to have a specific version, they should run the install script and pass the version as argument

@petersutter
Copy link
Member

btw. I have also created #104 for k9s as we have an install script for this as well but does not yet support to install it on demand

@mimiteto
Copy link
Contributor Author

If they want to have a specific version, they should run the install script and pass the version as argument

Arguments are passed directly to the binaries after they are installed, so I am not sure if that's a good idea.

IMO having the option to specify version can come in handy and first invocation of those commands will anyway break whatever script (because of the y/n part) so I don't see a reason to currently remove versions.

It would make sense if we decide to change it so when an operator invokes, for example, wg if it's not installed to automatically install, no questions asked. In that case I agree having the option to specify a version is pointless.

@petersutter
Copy link
Member

What about the following:

  • The install script should not invoke the binary. It should just install it. If you pass a version as argument it should take it, otherwise it should install the latest (or maybe a hardcoded version, depends on how we want it to behave).
  • Running the binary, like etcdctl should install it on demand in case it is not already installed, no questions asked to the user. We could redirect any output of the install script to stderr, which could make it more compatible with scripts calling it for the first time..

@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 18, 2024
@gardener-robot gardener-robot added the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Mar 18, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 18, 2024
@gardener-robot gardener-robot removed the size/s Size of pull request is small (see gardener-robot robot/bots/size.py) label Mar 18, 2024
hacks/install_etcdctl Outdated Show resolved Hide resolved
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 18, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 18, 2024
@mimiteto
Copy link
Contributor Author

I made the on-demand binaries get installed when executed without any questions asked.
I also changed the etcdctl install script to consider current user (root goes to the system path, non-root uses dir relevant to the current user).
In addition I made the etcdctl alias add arguments about certificates in the default paths when etcdctl is executed within debug pod.
PR is currently incomplete, as it still needs to change the way dotfiles are made available

hacks/install_etcdctl Outdated Show resolved Hide resolved
@petersutter
Copy link
Member

PR is currently incomplete

do you want to change it to draft then?

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 18, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 4, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 4, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 5, 2024
@mimiteto
Copy link
Contributor Author

mimiteto commented Apr 5, 2024

After a sync with @petersutter:

  • I've reverted the ops-pod script
  • I've dropped the image pull policy - that way we will default to Always if no image is specified, because default image in the script has latest tag. Otherwise we will default to the IfNotPresent

Copy link
Member

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/rebase Needs git rebase needs/review Needs review needs/second-opinion Needs second review by someone else labels Apr 5, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 5, 2024
Copy link
Contributor

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

Just a few more minor suggestions. Rest looks very good, I think.

hacks/install_k9s Outdated Show resolved Hide resolved
hacks/start_chrooted Outdated Show resolved Hide resolved
hacks/ops-pod Show resolved Hide resolved
hacks/install_etcdctl Outdated Show resolved Hide resolved
hacks/start_chrooted Outdated Show resolved Hide resolved
install_on_demand/.etcdctl Show resolved Hide resolved
install_on_demand/.k9s Show resolved Hide resolved
install_on_demand/.shrc Show resolved Hide resolved
install_on_demand/.table Show resolved Hide resolved
install_on_demand/.wireguard Show resolved Hide resolved
@gardener-robot gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Apr 9, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 9, 2024
Copy link
Contributor

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/second-opinion Needs second review by someone else labels Apr 9, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 9, 2024
Introduce `etcdctl` and `k9s` on demand scripts.
Automatically generate `etcdctl` connectivity parameters when `wrapper` runs.
@gardener-robot gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Apr 9, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 9, 2024
@petersutter petersutter merged commit 80c162e into gardener:master Apr 10, 2024
8 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/second-opinion Needs second review by someone else size/l Size of pull request is large (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants