feat: Adds abra app move #605
Reference in New Issue
Block a user
No description provided.
Delete Branch "p4u1/abra:abra-app-move"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
See #586 for discussion
This is a very wip implementation to move an app from one server to another.
goals:
non goals:
implementation:
Usage:
abra app move gitea.example.com serverb.example.com
abra app deploy gitea.example.com
I successfully moved a forgejo app with this, but couldn't get a moved nextcloud app running, because of some weird permission problems...
also the code is a total mess right now
3be23783fe
tod2c8aa5659
I cleaned up code and added fancy move overview:
Would love to get some feedback on the general direction. @decentral1se especially you, since you raised concerns about adding this to abra directly (which I totally get). If we decide not to add this to abra directly maybe we can add a new cmd to this project? similar to kadabra
@p4u1 looking good, i'm all for it! let me know when you want a code review and I can take a pass. from what i can tell, it's all looking pretty "standard" / "maintainable" and it would be great to support it well. I am not sure how we can test this but maybe you have ideas. I imagine validating that all required binaries/system executables are present needs some extra work. Also, some graceful handling for error cases will be tricky. I agree with the separation to not deploy the new moved app, that's for the operator to figure out (probably more chaos 🙃).
The only special thing is, that it's not only using the dockerclient but also running ssh and scp, wich has not been done before in Abra I think.
I will clean the code some more but should be ready for review soon.
One thing that troubles me is the file permission problem when moving a next cloud app. All other apps I moved worked, but when moving nextcloud it does not start because a test to create a file does not work. And I'm not quite sure what to try next because I already set the flags on scp to keep permission and ownership. If you have any ideas, please let me know!
Regarding testing I will try to come up with something, but we will probably need a second server
d2c8aa5659
tob8dbc5d65d
@decentral1se I fixed the premission problem (I had to create the volume before copying the files, otherwise docker wouls reset the folder ownership). I also cleaned to code a bit and added some info at the end.
So this is ready for a review :)
b8dbc5d65d
to126acee958
WIP feat: Adds abra app moveto feat: Adds abra app moveGreat work 👏👏👏
@ -0,0 +97,4 @@
secretName := strings.Join(sname[:len(sname)-1], "_")
data := resources.Secrets[secretName]
if err := client.StoreSecret(cl2, s.Spec.Name, data); err != nil {
log.Infof("creating secret: %s", s.Spec.Name)
Is this
log.Infof
in the right place? Seems like it should log earlier in the success case?@ -0,0 +109,4 @@
// docker creates a new volume it set the folder permissions to
// root, which might be wrong. This ensures we always have the
// correct folder permissions inside the volume.
log.Debug("creating volume: %s", v.Name)
log.Debugf
? (there are more cases of this below)@ -0,0 +174,4 @@
fmt.Println("Run the following command to deploy the app", app.Name, newServer)
fmt.Println(" abra app deploy --no-domain-checks", app.Domain)
fmt.Println()
fmt.Println("And don't forget to update you DNS record. And don't panic, as it might take a bit for the dust to settle. Traefik for example might fail to obtain the lets encrypt certificate for a while.", app.Domain)
I like this but I think the DNS record part should be in the main
--help
description. I wouldn't mention Traefik in the case in the future that other proxies are used (I think Caddy is already being used...) and this is out-of-date / potentially confusing.@ -0,0 +176,4 @@
fmt.Println()
fmt.Println("And don't forget to update you DNS record. And don't panic, as it might take a bit for the dust to settle. Traefik for example might fail to obtain the lets encrypt certificate for a while.", app.Domain)
fmt.Println()
fmt.Println("If anything goes wrong, you can always move the app config file to the original server and deploy it there again. There was no data removed on the old server")
I would add this really centrally to the main
--help
output instead so people know beforehand!@ -0,0 +262,4 @@
targetContainer, err := containerPkg.GetContainer(context.Background(), cl, f, true)
if err != nil {
log.Error(err)
continue
This code used to be in a for loop and now isn't?
continue
looks suspect here.@ -0,0 +276,4 @@
}
log.Debugf("extracting secret %s", secretName)
out, err := exec.Command("ssh", app.Server, "-tt", fmt.Sprintf("sudo cat /var/lib/docker/containers/%s/mounts/secrets/%s", targetContainer.ID, secretID)).Output()
I think it should mention that
tar
/cat
are required on the target server in the main--help
output. You might also consider doing a check up-front at the start of the command execution? It will be slower but less painful if things are missing...@ -143,0 +162,4 @@
{"DOMAIN", domain},
{"RECIPE", app.Recipe.Name},
{"OLD SERVER", server},
{"New SERVER", newServer},
NEW
@ -77,3 +77,3 @@
}
log.Info("polling undeploy status")
log.Debug("polling undeploy status")
That seems like a somewhat significant "off-topic" change to the undeploy screen for this PR. I have no strong feelings about it but we regularly see reported issues when the output of
deploy
/undeploy
changes. If you are happy to deal with that, then I'm fine with it 🙃Hmmmm that sounds a bit wasteful and also awkward. The integration test suite is already quite overwhelming to get into running for the everyday hacker and this would complicate things. Is there a way to test the underlying logic in the unit tests? Is there a way to make this logic more testable? I did think of
docker:dind
to mock a remote server but that could be quite chaotic...I will be away until end of September, if anyone wants to pick this up feel free. Otherwise I will finish it when I'm back :)
#636
Pull request closed