feat: Adds abra app move #605
Reference in New Issue
Block a user
No description provided.
Delete Branch "(deleted):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
3be23783fetod2c8aa5659I 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
d2c8aa5659tob8dbc5d65d@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 :)
b8dbc5d65dto126acee958WIP 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.Infofin 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
--helpdescription. 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
--helpoutput instead so people know beforehand!@ -0,0 +262,4 @@targetContainer, err := containerPkg.GetContainer(context.Background(), cl, f, true)if err != nil {log.Error(err)continueThis code used to be in a for loop and now isn't?
continuelooks 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/catare required on the target server in the main--helpoutput. 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/undeploychanges. 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:dindto 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