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

Updated directory structure and have versioned names #45

Merged
merged 1 commit into from Aug 8, 2018

Conversation

puneetguptanitj
Copy link
Contributor

@puneetguptanitj puneetguptanitj commented Aug 4, 2018

Updated /var/cache to have a consistent format.
Updated directory names to have versions, to allow multiple installations to coexist
Have /opt/bin to have actual binaries and not symlinks (as we now have directories with versions in /var/cache)

Also change the directory names for other binaries to follow the same pattern of
/var/cache/<parent-tool>/<tool>/<tool-version>/<tool-binary> (by parent tool we mean the one which uses the tool, e.g. ssh-provider is the parent tool for nodeadm, while nodeadm is the parent tool for kubernetes)

contents of /var/cache

coreos-puneet-199-10-105-16-14platform9 cache # find nodeadm/
nodeadm/
nodeadm/kubernetes
nodeadm/kubernetes/v1.10.4
nodeadm/kubernetes/v1.10.4/10-kubeadm.conf
nodeadm/kubernetes/v1.10.4/kubeadm
nodeadm/kubernetes/v1.10.4/kubelet.service
nodeadm/kubernetes/v1.10.4/kubectl
nodeadm/kubernetes/v1.10.4/kubelet
nodeadm/cni
nodeadm/cni/v0.6.0
nodeadm/cni/v0.6.0/cni-plugins-amd64-v0.6.0.tgz
nodeadm/images
nodeadm/images/80cc5ea4b547abe174d7550b82825ace40769e977cde90495df3427b3a0f4e75.tar
nodeadm/images/c2ce1ffb51ed60c54057f53b8756231f5b4b792ce04113c6755339a1beb25943.tar
...
nodeadm/flannel
nodeadm/flannel/v0.10.0
nodeadm/flannel/v0.10.0/kube-flannel.yml

coreos-puneet-199-10-105-16-14platform9 cache # find etcdadm/
etcdadm/
etcdadm/etcd
etcdadm/etcd/v3.3.8
etcdadm/etcd/v3.3.8/etcd-v3.3.8-linux-amd64.tar.gz

coreos-puneet-199-10-105-16-14platform9 cache # find ssh-provider/
ssh-provider/
ssh-provider/nodeadm
ssh-provider/nodeadm/v.0.0.1-alpha
ssh-provider/nodeadm/v.0.0.1-alpha/nodeadm
ssh-provider/etcdadm
ssh-provider/etcdadm/v.0.0.1-alpha
ssh-provider/etcdadm/v.0.0.1-alpha/etcdadm

contents of /opt/bin

coreos-puneet-199-10-105-16-14platform9 bin # ls
cctl  conf  etcd  etcdadm  etcdctl  exporter  kubeadm  kubectl  kubelet  nodeadm

binary/etcd.go Outdated
return err
}
return util.CreateSymLink(etcdctlBinaryPath, etcdctlSymLinkPath)
return util.CopyFile(etcdctlSrcPath, etcdctlDestPath)
}

// DeleteSymLinks deletes symlinks created for etcd binaires
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Change comment to reflect delete binaries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Arun, fixed it

cmd/reset.go Outdated
}
// Remove symlinks
if err = binary.DeleteSymLinks(constants.DefaultInstallBaseDir); err != nil {
if err = binary.DeleteBinaries(constants.DefaultInstallBaseDir); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If InstallBaseDir is now renamed to InstallDir, should DefaultInstallBaseDir be renamed to DefaultInstallDir ?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated it

binary/etcd.go Outdated

if err := util.CreateSymLink(etcdBinaryPath, etcdSymLinkPath); err != nil {
if err := util.CopyFile(etcdSrcPath, etcdDestPath); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since CopyFile does not force copy a file cp -f, should there be a FileExists check to see if the binary already exists, and remove the binary if it does exist before copying the binary over to the destDir? (Idempotent operation)

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that's already handled when we call IsInstalled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks updated to cp -f

binary/etcd.go Outdated
etcdSymLinkPath := filepath.Join(symLinkDir, "etcd")
etcdctlSymLinkPath := filepath.Join(symLinkDir, "etcdctl")
if err := util.RemoveFile(etcdSymLinkPath); err != nil {
func DeleteBinaries(installDir string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI: I added an Uninstall to complement InstallFromCache in platform9@46866b8. Once that PR merges, this will get called by Uninstall, and won't need to be exported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Daniel, updated to use Uninstall

@@ -42,3 +43,11 @@ func CreateSymLink(srcFile, linkFile string) error {
RemoveFile(linkFile)
return os.Symlink(srcFile, linkFile)
}

// Copy file from src to dest
func CopyFile(srcFile, destFile string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's true that the standard library doesn't provide a "copyfile" function (here's why). Do you prefer calling cp to ioutil.ReadFile + ioutil.WriteFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i also tried to look at some external libraries e.g. https://github.com/spf13/afero but that also doesn't have a copy utility. I felt that using cp was much more concise and had other features like automatically retaining the file permissions..

@@ -42,3 +43,11 @@ func CreateSymLink(srcFile, linkFile string) error {
RemoveFile(linkFile)
return os.Symlink(srcFile, linkFile)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: The other functions (and this file) were removed in #46. You'll need to add the file back. My mistake. The file was not removed, but all functions apart from FileExists were.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, thanks Daniel

@puneetguptanitj puneetguptanitj force-pushed the private/versioned-tools-dir branch 2 times, most recently from 397795f to 4628381 Compare August 8, 2018 17:36
@vannrt
Copy link
Contributor

vannrt commented Aug 8, 2018

Can you make sure this question is resolved https://github.com/platform9/isv-tests/pull/26#pullrequestreview-144584751?

@vannrt vannrt self-requested a review August 8, 2018 22:44
@puneetguptanitj puneetguptanitj merged commit 8b4b59a into master Aug 8, 2018
@puneetguptanitj puneetguptanitj deleted the private/versioned-tools-dir branch August 8, 2018 23:43
justinsb pushed a commit to justinsb/etcdadm that referenced this pull request Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants