feat: Adds abra app move #605
Closed
p4u1
wants to merge 1 commits from
(deleted):abra-app-move into main
pull from: (deleted):abra-app-move
merge into: :main
:main
:renovate/golang-1.27
:renovate/github.com-charmbracelet-bubbletea-2.x
:fix/492
:local-integration-testing
:renovate/github.com-charmbracelet-lipgloss-2.x
:renovate/otel-weaver-0.x
:renovate/codespell-2.x
:renovate/tonistiigi-xx-1.x
:renovate/alpine-3.x
:renovate/github.com-charmbracelet-log-2.x
:chore-deps
:fix/deps
:fix/613
Labels
Clear labels
bug
build
ci/cd
critical fix
design
documentation
duplicate
easy-first-issue
enhancement
help wanted
i10n
i18n
installer
invalid
question
release
release-candidate
security
tech-debt
test
wontfix
Something is not working
go build related issues
Building things with CI/CD
https://docs.coopcloud.tech/federation/resolutions/passed/010/
UI/UX
Documenting all the things
This issue or pull request already exists
Something for new people to get stuck into. We hope it's easy!
New feature
Need some help
Everything to do with localisation
Everything to do with internationalisation
Everything to do with the install script.
Something is wrong
More information is needed
Release management
Related to the new release candidate
Security related
Unit/integration testing
This won't be fixed
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Abra v0.11.x
Done
Assignees
3wordchant
aadil (Aadil Ayub)
abra-bot (Abra Bot)
ammaratef45
amras (Sarma)
Apfelwurm
BornDeleuze
Brooke
carla
cas (Cassowary)
coopcloud
cyrnel
decentral1se (d1)
dede
devydave
fauno (fauno)
iexos
jade (Jade Ambrose)
jjsfunhouse
jmakdah2 (Jackie Makdah)
joe-irving (Joe Irving)
kawaiipunk (KawaiiPunk)
knoflook
kolaente
lambdabundesverband
linnealovespie (April)
moosemower
moritz
notplants
oxaliq (sorrel)
p4u1
pharaohgraphy (Andrew 🐦🔥❤️🔥✴️)
renovate-bot (Comrade Renovate Bot)
ripclap
simon
sixsmith (Sixsmith)
stevensting
trav (Trav Fryer)
val (val (he/him))
yksflip
Clear assignees
No Assignees
p4u1
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: toolshed/abra#605
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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