-
Notifications
You must be signed in to change notification settings - Fork 77
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
CLOUPD-193335: Deletion Protection Serverless Private Endpoints #1093
Conversation
4439b5c
to
aafeb2d
Compare
Still a draft as it should go after bugfix #1082 |
da2ed29
to
4911f95
Compare
2fe9b4e
to
35045d4
Compare
24cc966
to
6fd303c
Compare
debugging |
6fd303c
to
f593f2a
Compare
Even after a rebase the issues persist. Seems |
f593f2a
to
88c6e6b
Compare
Debugging some panics in CI tests. |
88c6e6b
to
c7e1aaa
Compare
I think there are more usages of |
c7e1aaa
to
838e7ae
Compare
@igor-karpukhin gave me the right hint here. if deployment.IsLegacyDeployment() {
if err := ConvertLegacyDeployment(&deployment.Spec); err != nil {
result = workflow.Terminate(workflow.Internal, err.Error())
log.Errorw("failed to convert legacy deployment", "error", err)
return result.ReconcileResult(), nil
}
deployment.Spec.DeploymentSpec = nil
} But the key point is that conversion must never be seen in Kubernetes. It is ONLY used in memory for convenience so we always handle only serverless deployments and advanced. aAs old deployments get converted to their advanced equivalent. My code somewhere must be saving the converted spec to Kubernetes, so the rest of the test code and even Helm fail as they assume a default configuration which is missing, as it was converted to advanced. This conversion is wrong, it should not have happened. |
838e7ae
to
58560b7
Compare
Seems all issues, including the helm error and the panics were due to The fix is to actually make sure |
2 errors remain that might not be flaky:
Debugging those now. |
58560b7
to
dab8a24
Compare
The problem was due to func handleAutoscaling(ctx *workflow.Context, desiredDeployment *mdbv1.AdvancedDeploymentSpec, currentDeployment *mongodbatlas.AdvancedCluster) error {
...
syncInstanceSize := func(s *mdbv1.Specs, as *mdbv1.AdvancedAutoScalingSpec) error {
if s != nil {
...
if isInstanceSizeTheSame(currentDeployment, size) {
size = ""
}
s.InstanceSize = size Those changes were being saved to the Kubernetes state when updating the deployment after (re)setting the previous config here: func ApplyLastConfigApplied(ctx context.Context, resource mdbv1.AtlasCustomResource, k8sClient client.Client) error {
...
return k8sClient.Update(ctx, resource, &client.UpdateOptions{})
} This was only happening for advanced cluster, because I was NOT making a copy of the Kubernetes deployment structure unless the conversion was actually needed. The fix was to simply always make the converted deployment variable a separate copy. That way, the original deployment will be unchanged, and no changes in the reconcile loop will be reflected back into the Kubernetes etcd status. A separate matter is whether or not those ephemeral changes to the deployment struct make actual sense, but that is out of the scope of this PR. |
Current persistent failures are in encryption-at-rest a well know flaky test that, AFAIK, is totally unrelated to the changes here:
Will be debugging it now. |
Created a follow up issue for that. Now the test passed, we are ready for review. |
@josvazg this is done because of the way we operate with autoscaling. For example, if you set instanceSize to be M10 and enabled autoscaling from M10 to M40, later, after cluster is autoscaled to M30, if you try to modify the |
Thanks for the explanation. Then it means that ephemeral changes to the k8s struct is like an intermediate representation to ease syncing with Atlas. Makes sense. |
dab8a24
to
8dabad8
Compare
Rebased |
a3dee69
to
f1977e4
Compare
f1977e4
to
a9a544f
Compare
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 noticed something strange with your rebase. Changes already in the main branch are showing as new in your PR, I pointed out 2 but there are more, even entire files.
Thanks for the heads up, checking... |
Signed-off-by: Jose Vazquez <[email protected]>
a9a544f
to
1a5c121
Compare
All Submissions:
Putcloses #XXXX
in your comment to auto-close the issue that your PR fixes (if there is one).Update docs/release-notes/release-notes.md if your changes should be included in the release notes for the next release.