Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Add Params for external service #46

Merged
merged 27 commits into from
Feb 13, 2020
Merged

Add Params for external service #46

merged 27 commits into from
Feb 13, 2020

Conversation

ANeumann82
Copy link
Contributor

No description provided.

if err != nil {
log.Errorf("Error waiting for operator deploy to be in-progress: %s", err)
return err
if waitForDeploy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "wait for deploy complete" should also be inside the conditional block, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmmm, maybe. It only assures that the plan status is in 'COMPLETE', which is ok.

Maybe the parameter should be named waitForDeployInProgress, because that's what I tried to accomplish.

The problem with the "WaitForOperatorDeployInProgress" is that for an update to a ConfigMap, the "IN_PROGRESS" step is so short that the test misses it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of waiting first for the deploy to be "in-progress" and then waiting for "completed" is to follow the deploy state machine. A parameter update will trigger a deploy, which will cause the deploy status to go from "complete" -> "in-progress" -> "complete".

So the a parameter named "waitForDeploy" waiting for "in-progress" and then "complete" makes sense, because that's what one would want from a function that updates an instances parameters: change the parameter and wait for the parameter change to be reflected fully, i.e., for the deploy plan to complete.

The problem with the "WaitForOperatorDeployInProgress" is that for an update to a ConfigMap, the "IN_PROGRESS" step is so short that the test misses it.

Is this reproducible? Where did you see it happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, generally I would agree with the COMPLETE->IN_PROGRESS->COMPLETE flow.

I've noticed the problem in this step:
https://github.com/mesosphere/kudo-cassandra-operator/pull/46/files#diff-013e06c2e358068fd6cff2d5724007a7R31

In this update, only an additional service is deployed, without updating the Pods of the StatefulSet. I assume that the Apply of the Service is quick enough that the window for "IN_PROGRESS" is very short and not reliable enough to wait for it

Changed wait_for_deployment to use instance generation to check if a new plan is executed

Signed-off-by: Andreas Neumann <[email protected]>
Signed-off-by: Andreas Neumann <[email protected]>
Signed-off-by: Andreas Neumann <[email protected]>
@ANeumann82
Copy link
Contributor Author

This is mostly complete - It's still missing the deletion of the external service if the feature is disabled, I'll wait for the Toggle Feature support in KUDO.

@ANeumann82 ANeumann82 self-assigned this Feb 7, 2020
# Conflicts:
#	operator/operator.yaml
#	templates/operator/operator.yaml.template
#	tests/suites/sanity/sanity_test.go
Signed-off-by: Andreas Neumann <[email protected]>
# Conflicts:
#	tests/suites/sanity/sanity_test.go
#	tests/suites/tls/tls_test.go
#	tests/utils/k8s/k8s.go
#	tests/utils/kudo/kudo.go
Signed-off-by: Andreas Neumann <[email protected]>
Signed-off-by: Andreas Neumann <[email protected]>
Copy link
Contributor

@nfnt nfnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just some small comments on testing more conditions.

@ANeumann82 ANeumann82 merged commit 2262eb9 into master Feb 13, 2020
@ANeumann82 ANeumann82 deleted the an/add-external-service branch February 13, 2020 15:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants