Skip to content
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

Wait for elasticsearch cluster to be up #242

Merged
merged 3 commits into from
Feb 28, 2019

Conversation

pavolloffay
Copy link
Member

Resolves #216

Signed-off-by: Pavol Loffay [email protected]

@jpkrohling
Copy link
Contributor

This change is Reviewable

@pavolloffay
Copy link
Member Author

There is one minor is deployment.go deletes ES deployment once it's up, not sure why I will have to dig into it

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

Merging #242 into master will decrease coverage by 0.28%.
The diff coverage is 61.29%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #242      +/-   ##
=========================================
- Coverage   89.69%   89.4%   -0.29%     
=========================================
  Files          62      62              
  Lines        2717    2738      +21     
=========================================
+ Hits         2437    2448      +11     
- Misses        181     187       +6     
- Partials       99     103       +4
Impacted Files Coverage Δ
pkg/controller/jaeger/jaeger_controller.go 33.33% <0%> (ø) ⬆️
pkg/storage/elasticsearch.go 79.81% <100%> (-0.19%) ⬇️
pkg/controller/jaeger/elasticsearch.go 59.09% <58.33%> (-4.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9302c08...2906cb6. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pavolloffay)

a discussion (no related file):
:lgtm:

One side-effect to this PR is that the images for Jaeger components no longer happen in parallel to the bootstrap of Elasticsearch. It's not a big deal, as it makes a difference only for the first time each image is used, but it might be worth opening an issue to investigate if we can pull the images in advance, before we declare them in a deployment.



pkg/controller/jaeger/elasticsearch.go, line 71 at r2 (raw file):

			}
		}
		return available == expectedSize, nil

Could you add a Debug() message with the current state? When debugging issues, I found it helpful to know that it's waiting for (N) nodes to become ready. We have something like this in the deployment:

		if d.Status.ReadyReplicas != d.Status.Replicas {
			log.WithFields(log.Fields{
				"namespace": dep.Namespace,
				"name":      dep.Name,
				"ready":     d.Status.ReadyReplicas,
				"desired":   d.Status.Replicas,
			}).Debug("Waiting for deployment to estabilize")
			return false, nil
		}

pkg/storage/elasticsearch.go, line 114 at r2 (raw file):

				"app.kubernetes.io/part-of":   "jaeger",
				// we cannot use jaeger-operator because our component controllers removes all objects
				// created by ES operator

Do you mean that our Operator removes the Elasticsearch stateful sets?

@pavolloffay
Copy link
Member Author

Do you mean that our Operator removes the Elasticsearch stateful sets?

There are no SS but yes. These objects would be queried by our controllers - which should not be bc they are managed by ES operator

Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay pavolloffay merged commit fc10364 into jaegertracing:master Feb 28, 2019

"github.com/pkg/errors"
"github.com/sirupsen/logrus"
Copy link
Contributor

Choose a reason for hiding this comment

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

Across the code base, we are now using log as alias to this package.

available++
}
}
logrus.WithFields(logrus.Fields{
Copy link
Contributor

Choose a reason for hiding this comment

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

s/logrus/log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants