Create a headless service so that other nodes can lookup the seed provider #128
Conversation
|
/test all |
wallrj
changed the title
|
/test all |
|
/retest |
|
/test all |
wallrj
changed the title
wallrj
changed the title
|
/test all |
|
/test e2e |
|
Logs show: Perhaps I just need to give the second node longer to connect to the first. |
|
/test e2e |
hack/e2e.sh
Outdated
| kubectl describe apiservice | ||
| kubectl logs -c apiserver -l app=navigator,component=apiserver | ||
| kubectl logs -c controller -l app=navigator,component=controller | ||
| kubectl logs -c etcd -l app=navigator,component=apiserver |
There was a problem hiding this comment.
Has all this moved somewhere else? Can't see it elsewhere in the PR.
There was a problem hiding this comment.
I added kubectl cluster-info dump --all-namespaces which appears to provide all this information and more. For example, it gives me the logs of all the cassandra pods which is how I was able to see that the test wasn't allowing long enough for the second node to join the cluster.
| func(t *testing.T) { | ||
| f := newFixture(t) | ||
| f.Run() | ||
| f.AssertServicesLength(1) |
There was a problem hiding this comment.
Can we perform any checks to ensure the created service has the required ports/type etc. set? Or is this done elsewhere? 😄
There was a problem hiding this comment.
There are e2e checks for the service ports.
| createService ServiceCreator, | ||
| updateService ServiceUpdater, | ||
| ) error { | ||
| svc := createService(cluster) |
There was a problem hiding this comment.
Do we need a creator as well as an updater? In my mind, calling updateService with an &apiv1.Service{} should have the same effect as calling createService?
There was a problem hiding this comment.
Ah - looks like that's what is always happening, but then we pass a Creator/Updater func to this function. Is there any reason for this? Do we behave differently ever? It'd be ideal to simplify it if we can.
There was a problem hiding this comment.
I need the create function for generating services for the unit tests. But you're right, it can probably be simplified in a future branch.
| func CqlServiceName(c *v1alpha1.CassandraCluster) string { | ||
| return fmt.Sprintf("%s-cql", ResourceBaseName(c)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Note (but not required to be fixed in this PR, as I think it's a wider problem) - I've run into problems in cert-manager where resource names become too long as we just keep appending to things. We should try and work out a 'grand unified way' to generate resource names predictably.
|
Could you squash this into 1 commit before merge? 😄 |
|
(then all good! I don't want to say the magic phrase for fear this will auto merge... 🙄 ) |
wallrj
force-pushed
the
127-seed-provider
branch
2 times, most recently
from
28a8ab5
to
fa31cb5
Compare
@munnerz Done. Thanks for the review. |
* Create a headless service so that other nodes can lookup the seed provider * Create a separate CQL service which loadbalances across all the C* nodes. * Refactor the service tests so that they can be re-used for both service controllers. Fixes: jetstack#127 **Release note**: ```release-note NONE ```
wallrj
force-pushed
the
127-seed-provider
branch
from
fa31cb5
to
3947940
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: munnerz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
1 similar comment
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
Fixes: #127
Release note: