-
Notifications
You must be signed in to change notification settings - Fork 103
Update backup agent default image type to unified #2351
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you adjust the failing test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to know if you have the time to finish the work?
@@ -3925,7 +3925,7 @@ var _ = Describe("pod_models", func() { | |||
).To(Equal("foundationdb/foundationdb:dev")) | |||
Expect( | |||
deployment.Spec.Template.Spec.InitContainers[0].Image, | |||
).To(Equal("foundationdb/foundationdb-kubernetes-sidecar:dev-1")) | |||
).To(Equal("foundationdb/foundationdb:dev")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seemed a bit suspect to me -- shouldn't the init container be the fdb-kubernetes-monitor? The failure message indicated that it was actually the same image as the main container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test the image config is changed and the main image config is set to foundationdb/foundationdb:dev
. Since the default image is now the unified image for the backup pods, the init image and the main image will be the same, in this case foundationdb/foundationdb:dev
and the sidecar image config will be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Even though this is valid from the operator's perspective, this would not actually work right, because the foundationdb image can't be used as the init container image?
@@ -3963,22 +3963,22 @@ var _ = Describe("pod_models", func() { | |||
}) | |||
}) | |||
|
|||
When("using the unified image", func() { | |||
When("using the split image", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that we still want to test the split image behavior, so test swaps with pod_models when getting the backup deployment with a basic deployment the init container [It] should set the image and command for the container
Sorry! We got sidetracked with something else. I've gone and fixed all the tests now. One slightly unrelated question -- I noticed that the build docker image action failed, it seems like cross compilation of the arm64 image fails. We encountered the same issue in our build system and had to use an arm64 machine to do the compilation. Did you get cross compilation to work at any point? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One slightly unrelated question -- I noticed that the build docker image action failed, it seems like cross compilation of the arm64 image fails. We encountered the same issue in our build system and had to use an arm64 machine to do the compilation. Did you get cross compilation to work at any point?
Most of the time the arm64 image builds fine but sometimes it is failing in the GitHub action worker, we haven't invested too much time debugging the issue as a retry usually fix the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the code is not in the expected format, could you run make fmt lint
?
Result of fdb-kubernetes-operator-pr on Linux RHEL 9
|
Fixed the formatting, please take a look? |
Description
See #2349
Type of change
Please select one of the options below.
I think I would consider this a breaking change since customizations to the FoundationDbBackup resource will need to take into account the fact that the unified image has a slightly different pod spec.
Discussion
Are there any design details that you would like to discuss further?
Testing
I ran unit tests, but am not yet sure how to get e2e tests to run.
Documentation
Did you update relevant documentation within this repository?
If this change is adding new functionality, do we need to describe it in our user manual?
If this change is adding or removing subreconcilers, have we updated the core technical design doc to reflect that?
If this change is adding new safety checks or new potential failure modes, have we documented and how to debug potential issues?
Follow-up
Are there any follow-up issues that we should pursue in the future?
Does this introduce new defaults that we should re-evaluate in the future?