Skip to content
This repository has been archived by the owner on Jul 16, 2024. It is now read-only.

Check if ETCD is already installed, before attempting to install #178

Closed
wants to merge 0 commits into from

Conversation

ntaylor1781
Copy link

In situations where access to the internet is blocked, it may be needed/easiest to install an upstream ETCD package. This creates the situation where ETCD is installed, but etcdadm will fail to move forward because it can't download the binary. We can check if it is installed first, and move onto the old logic if it is not.

@k8s-ci-robot
Copy link
Contributor

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: [email protected]

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 k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 13, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @ntaylor1781!

It looks like this is your first PR to kubernetes-sigs/etcdadm 🎉. 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/etcdadm 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 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 Oct 13, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @ntaylor1781. 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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 13, 2020
@ntaylor1781
Copy link
Author

/assign @dlipovetsky

@ntaylor1781
Copy link
Author

/check-cla

1 similar comment
@ntaylor1781
Copy link
Author

/check-cla

@ntaylor1781
Copy link
Author

It does not seem to be registering the CLA as signed. The task says commit missing GitHub user, but I see it as associated.

@ntaylor1781
Copy link
Author

Attempted reverting my change, and adding it with a fixed author. The CLA shows as authorized. Let me know if there is a desired way to clean this up.

@dlipovetsky
Copy link
Contributor

Thanks for the PR @ntaylor1781! Looking.

@dlipovetsky
Copy link
Contributor

BTW, to appease the CLA Bot, I think you'll need to remove the initial commit and revert commit and force-push to your GitHub branch.

@dlipovetsky
Copy link
Contributor

In situations where access to the internet is blocked, it may be needed/easiest to install an upstream ETCD package.

Can you help me understand this use case better, please? By "upstream ETCD package," do you mean a package maintained by a Linux distro, e.g., an rpm, deb, etc?

etcdadm is designed to support the "air-gapped" use case, but it asks the user to fetch an etcd tarball published on the etcd GitHub releases page, and place it into the cache directory.

I agree that, if the both the correct etcd and etcdctl binaries are already installed, then etcdadm should not install them again from the cache.

However, I think that works only under the assumption that etcdadm "manages" the lifecycle of those binaries.

For example, if you install etcd/etcdctl binaries using a system package, but later run etcdadm reset, etcdadm will remove the binaries, and you won't be able to run etcdadm init/join until you install the binaries again.

If etcdadm could delegate installation of the binaries to a system package manager, that would work. But it's not trivial to integrate etcdadm with various system package managers, so I'd like to understand why the "download the tarball and put it in the cache" approach does not work well for you.

@ntaylor1781
Copy link
Author

By upstream ETCD package, I did mean a Linux distro maintained package. Using the cache directory is valid, and something I initially did as a workaround. The package was an easy way out. It mainly came down to not wanting to have a tarball in the bootstrap/config-management git, and not wanting to stand up a logical place for it.

I should have read the code for reset, as I didn't know that it cleaned up the binaries on reset. It seemed like this would be a simple change that could allow me to be lazy, but it appears that is not the case.

A flag could be added to not remove on reset, but I feel like the chance of error is pretty high (a user installs through package manager, but doesn't use the proper flag on reset).

With this new information it probably makes the most sense to close this.

@dlipovetsky
Copy link
Contributor

The package was an easy way out. It mainly came down to not wanting to have a tarball in the bootstrap/config-management git, and not wanting to stand up a logical place for it.

Can you share how (and when) you fetch the etcd system package?

(Also, do you know about etcdadm download, which populates the cache without installing etcd?)

@ntaylor1781
Copy link
Author

The process I was working on was to install etcd from an internal mirrored repo, and then have automation handle the etcdadm commands to start. The idea being that this change would allow it to see that it already existed, and not fail out.

I am aware of etcdadm download, and used it to create the cache directory structure (as the download failed to get to the internet).

A solution I can use would be to package the cache directory, and etcdadm, in an rpm. I can then easily install that from the internal mirrors. I initially added the change as it seemed like it would have no affect, and make it so that I didn't have to make an RPM (or provide some other system for grabbing the tar).

@dlipovetsky
Copy link
Contributor

A solution I can use would be to package the cache directory, and etcdadm, in an rpm

That would work.

If you don't need an rpm to get etcdadm on the machine, then it might be easier to create an cache-only rpm, e.g., by running etcdadm download in the rpm build root.

@dlipovetsky
Copy link
Contributor

It's worth contemplating making install/uninstall opt-in. Since etcdadm already verifies that the binaries are of the right version, I think it should be safe to delegate install/uninstall to something else.

etcdadm install
etcdadm init/join

..

etcdadm reset
etcdadm uninstall

Alternatively,

etcdadm init --install
etcdadm reset --uninstall

Do you want to join the next etcdadm office hours to talk about the trade-offs?

@ntaylor1781
Copy link
Author

Yea definitely. We could also look at implementing the phase concept kubeadm uses. This could be useful to run only specific steps, but would also make it easy to add a --skip-phases option to init and reset as well.

@dlipovetsky
Copy link
Contributor

@ntaylor1781 Could you file an issue for phases, please? If you think phases would meet your use case, we might be able to close this PR.

@ntaylor1781
Copy link
Author

I created #179 for tracking. This is a busy time of year, but I will see what I can do with it, if someone doesn't beat me to it. Would you like me to close this PR?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ntaylor1781
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2020
@ntaylor1781
Copy link
Author

Well guess cleaning up my fork to look at phases closed it anyone.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants