fyne-cross
fyne-cross copied to clipboard
fyne-io
Feature/kubernetes
Description:
This does add a Kubernetes engine to fyne-cross.
Fixes #98
Checklist:
- [ ] Tests included.
- [x] Lint and formatter run with no errors.
- [ ] Tests all pass.
Where applicable:
- [ ] Public APIs match existing style.
- [ ] Any breaking changes have a deprecation path or have been discussed.
- [ ] Updated the vendor folder (using
go mod vendor).
Can you address the test failures @Bluebugs , then we can do a full review?
Can you address the test failures @Bluebugs , then we can do a full review?
Yes, the issue is due to a dependency that doesn't support old go. Need to use a less nice API to work around it :-(
I just noticed something, I can't find the code for fyne-cross-s3. It somehow disappeared. Will look into it tomorrow.
Looking at the use of archiver and problems with Go 1.14 compilation, it might even make sense to not use it at all and just create the functionality ourselves. It is a big modules because it imports all of the code for the compression formats it support.
I have some tar compression code laying around in a private repo if you are interested.
@Jacalz I think it would be better not to, as that is technical debt that we have to carry and there is already a lot to care for.
Personally, I think that the technical debt would be worth it over importing support for a half a dozen different compression formats. I might want to start working on my compression library again. If I remember correctly, it was modular and avoided importing everything.
Some naming stuff stood out as weird. I am wondering though - why is it necessary to expose a new
fyne-cross-s3command, can the kubernetes container code not call the functions directly?
The fyne-cross-s3 command is inside the container and is driven by fyne-cross. Without that, there is no capacity to upload/download anything from or to the container.
But as fyne-cross sets up the container is it not able to perform file operations without forking new executables inside the container?
But as fyne-cross sets up the container is it not able to perform file operations without forking new executables inside the container?
fyne-cross never modify the filesystem of the container. In the case of local docker/podman, it can mount host endpoint into the container as it is a local scenario. In the local scenario, the hack we are doing is to modify then files on the host as they are reflected through those mounted end point directly. In the case of kubernetes, there is no file sharing layer that allow for mounting a file system in the container. So we can only modify files by actually executing command in the container, downloading and uploading them as needed.
The current github action are triggered for darwin, but we do not provide that image publicly due to Apple SDK license. So it is expected to fail. Should we disable that part of the github action or is the zig experiment going to solve this? Other than that, this is passing all other tests.
The current github action are triggered for darwin, but we do not provide that image publicly due to Apple SDK license. So it is expected to fail. Should we disable that part of the github action or is the zig experiment going to solve this? Other than that, this is passing all other tests.
I guess we could disable the darwin tests if they will always fail as you say. Let's not bet on zig to fix this, as the SDK is still Apple's, replacing the compiler does not remove the need for Apple's code SDK think...
This failure is not darwin related:
Error: internal/command/kubernetes.go:75:2: misuse of unbuffered os.Signal channel as argument to signal.Notify
Error: Process completed with exit code 2.
The current github action are triggered for darwin, but we do not provide that image publicly due to Apple SDK license. So it is expected to fail. Should we disable that part of the github action or is the zig experiment going to solve this? Other than that, this is passing all other tests.
I guess we could disable the darwin tests if they will always fail as you say. Let's not bet on zig to fix this, as the SDK is still Apple's, replacing the compiler does not remove the need for Apple's code SDK think...
I will look into disabling it for now.
This failure is not darwin related:
Error: internal/command/kubernetes.go:75:2: misuse of unbuffered os.Signal channel as argument to signal.Notify Error: Process completed with exit code 2.
Good catch from go vet.
@metal3d this PR might be of interest to you. If you do have time to look at it, that would be great.
This is mostly a feature useful for enterprise who have their own infrastructure which cost less to run than using github action for example. This way they can automatically leverage that infrastructure to use fyne-cross to do those build. You can look at setup that use drone deploy for example. I do agree that this has little direct value for community users as they usually do not have a cluster setup at home (and when they have it might be a bunch of rpi and we do not support build on arm at the moment with fyne-cross), but it does deliver value for enterprise user that can now use fyne-cross to do CI/CD as part of their existing infrastructure more easily and in a more cost effective way.
Regarding the size, I think it isn't so big of an issue. In my go/bin directory, the initial fyne-cross is an outlier as most binary are between 10MB and 60MB. We are not talking about impacting the size of the produced binary, just one of a developer tool in a minor way that get updated once in a while. The 10x larger doesn't really mean much when you grow by 30MB and it make fyne-cross more in the norm of go binary tools.
I honestly do not see any technical benefit in trying to save 30MB in 2022 for this case with the added complexity of having two separate repository that need to be released and kept in sync and the addition of a public API to have them communicate. This would be over engineered and likely be considered technical debt as soon as it is released.
I also completely forgot, but fyne-cross in itself is completely useless without the image it use. The smallest image will take 450MB (Linux x64) and the biggest one goes all the way to 2.4GB for Android. I think the discussion about size completely forgot the fact, that to use fyne-cross, you need to download GB of image.
Regarding the size, I think it isn't so big of an issue. In my go/bin directory, the initial fyne-cross is an outlier as most binary are between 10MB and 60MB.
I am not talking about the size of go/bin, I am talking about the size of the build cache (the other folders in the go folders). The cached compiled dependencies for this (k8s and such) are huge and each build of a different version saves more of them. My go folder was 2GB in size after less than a month of regular Fyne binary compilations (where each binary is about 20MB in size). The results of 45MB binaries is going to be massive.
With recent refactoring it would be much easier to add this as an "extended fyne-cross" or fyne-cross-k8 I would think, which could be considered.
If we are looking to merge this into the fyne command (as has been put on the 2.3 roadmap https://github.com/fyne-io/fyne/issues/3043) I would expect it could be overkill to pull all of kubernetes in as well...
The version being hold in the past is due to our requirement to support old go release. The security implication of such old dependencies are low as it is a client connection instead of a server and the server is supposed to be trusted enough in this setup to see all the code being uploaded. I do consider this not ideal, but also not problematic.
Regarding the binary size and its dependencies, I still think they are dwarfed by the size of the image being used by fyne-cross. In the context of migrating this in fyne, this might be something to improve upon.
From a technical and long term maintenance perspective, I would largely prefer this to stay in tree, go with a solution that provide type safety across the link, stay simple and work on all our supported platform. This rules out go plugin as it doesn't work on Windows. This rules out also go-plugin from hashicorp as it does provide a lot of non necessary feature for this case. This means, we are pretty much on our own. A solution that I like is what is used in https://github.com/bithavoc/hellogrpcstdin . Basically using stdin/stdout to pipe grpc command. This would significantly expand the complexity of this patch and I would prefer to have a new PR as a follow up work for this, especially because that PR will introduce a public API (those gRPC command).
This rules out go plugin as it doesn't work on Windows. This rules out also go-plugin from hashicorp as it does provide a lot of non necessary feature for this case. This means, we are pretty much on our own.
I don't think any of us meant to imply a plugin solution would be desirable - it is a real pain to work with. However with minor modifications it would be possible to allow fyne-cross to accept new providers so that a codebase that wraps fyne-cross could provide things like k8 as workers without needing plugins, large dependency list or change of scope to fyne_cross and also enable others to add new providers without further weighing down the main codebase.
This becomes more of a concern as we move to fyne as a commandline tool that helps build and package would feel weird depending on k8 or other large infrastructure projects.
I think there is two ways to do embedding. Either wrap the fyne-cross command in an upper tool, or wrap that tool in fyne-cross. In the second case, we can use grpc over stdin/stdout to connect them and implement the 8 API necessary for ContainerEngine and ContainerImage. In the first case, I can not even start counting the amount of API, but it would be massive as all the parameters needs to be available somehow, require duplication of cli and so on. If I could choose, I would go with the second option.
But if we really want to make kubernetes an optional things, why not just put it behind a build tags in tree? This answer the problem of dependencies download/build and at the same time does not increase complexity/technical debt/maintenance complexity.
It is worth noting that separate build tags still would pull in all of the dependencies in the vendor folder of fyne (when we move this into that place).
vendor directory are quite problematic. Their main issue is that all their content will always be built even when not used by the parent project. Some project do break when build within a vendor directory. It so means that building the same application with or without the vendor directory will lead to potentially different result. If we could get rid of it, that would be great.
Also with go starting in version 1.17, you have lazy loading of module. This means that if we do put the kubernetes part in a subdirectory with its own go.mod (still hosted in the same git), we should be able to make it so that it doesn't download those dependencies without the build tag. That solution would still compile with older version, just in a slightly more download. That solution would still be less complexity than the other (Still in that scenario, it require to expose the interface of ContainerEngine and ContainerImage).
Agreed, having the command package exposed is probably the ideal solution. At that point we'd have a library that interact with docker/podman with its dedicated life cycle. It comes with the downside to maintain a new library...
An hacky solution could be also to use the workspaces introduced in 1.18.
Never tested, but this could allow to have a cmd/fyne-cross-k8 with its own go.mod leaving the possibility to access the internal packages. The folder structure will look like:
├── cmd
│ └── fyne-cross-k8
│ ├── go.mod
│ ├── go.sum
│ ├── go.work
│ └── main.go
├── internal
I actually don't think we need workspace for the following layout:
├── go.mod
├── go.sum
└── internal
├── k8s
├── go.mod
├── go.sum
For that purpose, we should be able to just use replace in the top go.mod. When building without k8s tags, it will not download and build any of the internal/k8s content. This avoid exposing any new API and use standard go function call.
If keeping a vendor directory when adding fyne-cross to fyne is still necessary an additional change would be to make the communication between the k8s interface and the rest of fyne-cross using a grpc call over stdin/stdout. This could be done with just moving the k8s module to internal/cmd and remove the replace in the top go.mod.
I'm pretty sure that go.mod in folders under another go.mod breaks older versions of go.
Do you remember which version you had trouble with? According to https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories it is a supported configuration. There might have been bugs before go 1.14 as module wasn't "ready", but it seems to have been just bugs.
I don't recall sorry. If it is deemed as a good solution to try we can do more testing.