Separate the humio client from cluster methods, move the kubernetes-related methods to a separate package and add initial unit tests #4

jswoods · 2020-03-10T03:48:59Z

No description provided.

kubernetes-related methods to a separate package and add initial unit tests

jswoods · 2020-03-10T03:54:07Z

There's not a ton here yet, and this is wip, but the important files are pkg/humio/client.go, pkg/humio/client_test.go, pkg/humio/cluster.go and pkg/humio/cluster_test.go. This is how I was thinking we lay out the client so it is mockable when we write the reconcile tests (as well as any unit tests for the cluster controller). I'd like to also do a similar pattern with the k8s client. Let me know if you had different ideas or if this aligns with what you were thinking as well.

SaaldjorMike · 2020-03-10T12:14:56Z

Looks good to me, but I have basically have no experience with writing tests in the Go world (yet). My main thoughts are:

We should probably at some point write tests within our API package in our CLI repo, like you started here: Add basic integration tests cli#9. I assume there will be some tests that are better suited to be part of the CLI project rather than within our operator project. In the CLI project, It would also be super cool if we could autogenerate the Go types instead of us having to manually maintain them. Not sure if we have a good approach to do this.
Going forward we probably want to have a way to support/define what Humio versions are supported by the operator. I'm thinking that Humio's GraphQL/REST API may change over time, and at some point we probably need to figure out how to support this going forward. For now, it is probably not super important though.
We could probably set up github-actions to run the tests on PR's now that the first couple of tests have been added.
Do we want to use a mocking framework at some point? Not sure if it would make sense for us to use one right away? As someone that have yet to dig into the whole world of writing tests in Go, I thought I'd ask as I've stumbled upon a ton of places where such frameworks are used.

jswoods · 2020-03-10T16:55:59Z

Good points. I agree on all fronts.

I have not done a great job about prioritizing the cli tests.. I spent some time adding json outputs so we can use those to run the integration tests but there is still quite a bit of work left.

I had not thought about the different humio versions. This is a really good point and while it may not be needed immediately, we likely will want this. A project that comes to mind where they seem to do a good job with this is the cluster autoscaler, see https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler#releases. They have a table that maps the k8s version with the CA version.

+1 on github actions

For the mocking framework, I think may be uses for this where it's hard or a pain to write our own mocks (for example, calling dependent libraries). For the situations where we only have a handfull of calls that we need to mock, I like the idea of using interfaces as it almost forces good composition. I don't always have the discipline of writing nicely composed code when given the luxury of stubs :) Also I think calling methods in tests exactly how they would be called in the real code helps a lot with documenting how they methods are intended to be used.

… humio api

jswoods · 2020-03-12T00:46:56Z

@SaaldjorMike I think this should be reviewed/merged now before I get too carried away :). I have been slowly pulling in code from the POC and there is now a successful reconcile test that creates humio pods similar to how it was done in the POC. There are separate unit tests for the humio code as well. There are no examples yet of using the mock humio client inside the reconcile test, but I think that can come in the round of changes as we add more to the reconciliation.

jswoods · 2020-03-12T00:48:24Z

pkg/controller/humiocluster/humiocluster_controller.go

+	}
+}
+
+func constructEnvVarList(nodeID int) *[]corev1.EnvVar {


I think we'll want to refactor this in an upcoming PR so we can merge env vars that are set in the spec (e.g. for things like saml, etc)

Sounds good to me!

SaaldjorMike · 2020-03-12T08:37:26Z

pkg/humio/cluster.go

+func generateStoragePartitionSchemeCandidate(storageNodeIDs []int, partitionCount, targetReplication int) ([]humioapi.StoragePartitionInput, error) {
+	replicas := targetReplication
+	if targetReplication > len(storageNodeIDs) {
+		replicas = len(storageNodeIDs)
+	}
+	if replicas == 0 {
+		return nil, fmt.Errorf("not possible to use replication factor 0")
+	}
+
+	var ps []humioapi.StoragePartitionInput
+
+	for p := 0; p < partitionCount; p++ {
+		var nodeIds []graphql.Int
+		for r := 0; r < replicas; r++ {
+			idx := (p + r) % len(storageNodeIDs)
+			nodeIds = append(nodeIds, graphql.Int(storageNodeIDs[idx]))
+		}
+		ps = append(ps, humioapi.StoragePartitionInput{ID: graphql.Int(p), NodeIDs: nodeIds})
+	}
+
+	return ps, nil
+}


I'm not 100% convinced that this functionality (as well as generating ingest partition scheme) should be within the operator - at least not in the long run. We hear customers now and then that are not super happy with the rather manual way of adjusting this right now, so we can probably move it to api package in the humioctl project at some point and direct customers (that does not use k8s) towards humioctl to help them spread partition across nodes. That being said, in order to really generate a good candidate for assigning partitions you'd need some rack/availabilityzone information for each of the Humio nodes. For customers running Humio outside k8s, I'm thinking there might be two ways for us to generate a good scheme.

Ensure Humio knows about the rack/AZ and can generate this scheme itself. Maybe just have a "generate and assign partitions" button from within the UI?

If Humio itself is not aware of rack/AZ: add method to Go API that requires you to specify which nodes are in which rack/AZ's.

Option 1 would probably be the best one in the long run, but I also see cases where it would make sense to use the Go API (in projects like this operator) and/or humioctl to generate and assign partitions to nodes.

Without rack/AZ information baked into Humio, I'm thinking that a subcommand of the CLI would be nice to also have where you're required to specify rack/AZ information. If we do decide to allow customers to specify rack/AZ directly in Humio's configurations somehow, we could probably relax this and only require customers to specify rack/AZ if Humio doesn't already have the information.

For now though, I'm good with just going with this. I just thought I'd at least share my thoughts 😄

Totally agree here. Would you rather start with putting zone awareness in the operator and then migrate it to the cli, or would you want to start with adding it to the cli first? I don't want to generate too much extra work with migrating it but also want to avoid getting too off-track of the operator work.

For getting the actual zones, we have the init container pattern used in the helm chart to pull the zone from the host where the pod is scheduled. I think we can probably reuse that here.

I filed #6 so feel free to put your opinion in there.

SaaldjorMike

LGTM! Feel free to merge it in whenever you're ready to do so.

SaaldjorMike · 2020-03-12T08:42:13Z

pkg/controller/humiocluster/humiocluster_controller.go

+			Volumes: []corev1.Volume{
+				{
+					Name: "humio-data",
+					VolumeSource: corev1.VolumeSource{
+						PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+							ClaimName: fmt.Sprintf("%s-core-%d", hc.Name, nodeID),
+						},
+					},
+				},
+			},


What do we want to do regarding Humio's data volume? We talked about focussing on our own use-case with ephemeral storage using hostPath, but maybe we'd also want a flag to maybe run with emptyDir when e.g. running it locally in kind/Minikube?

Good idea. I filed #5.

pkg/controller/humiocluster/defaults.go

Co-Authored-By: Mike Rostermund <mike@humio.com>

Separate the humio client from cluster methods, move the

3ef782c

kubernetes-related methods to a separate package and add initial unit tests

jswoods requested review from schofield and SaaldjorMike March 10, 2020 03:48

jswoods added 4 commits March 10, 2020 16:34

More tests

956607e

More tests, add storagePartitionsCount to spec

bf1887c

More tests, add digestPartitionsCount to spec

c481b88

Add more methods to the client interface for abstracting calls to the…

4e4e46f

… humio api

jswoods force-pushed the jestin/unit-tests branch from 49c005d to 2bafad1 Compare March 11, 2020 23:54

Start creating real humio pods and update tests

45af425

jswoods force-pushed the jestin/unit-tests branch from 2bafad1 to 45af425 Compare March 12, 2020 00:36

Cleanup

fe92315

jswoods marked this pull request as ready for review March 12, 2020 00:43

jswoods commented Mar 12, 2020

View reviewed changes

SaaldjorMike reviewed Mar 12, 2020

View reviewed changes

SaaldjorMike approved these changes Mar 12, 2020

View reviewed changes

SaaldjorMike reviewed Mar 12, 2020

View reviewed changes

pkg/controller/humiocluster/defaults.go Outdated Show resolved Hide resolved

Update pkg/controller/humiocluster/defaults.go

c29fca1

Co-Authored-By: Mike Rostermund <mike@humio.com>

This was referenced Mar 12, 2020

Support data volumes #5

Closed

Balance partitions across zones #6

Closed

Expose environment variables #7

Closed

jswoods merged commit f0c0dce into master Mar 12, 2020

jswoods deleted the jestin/unit-tests branch March 12, 2020 16:37

SaaldjorMike added a commit that referenced this pull request Oct 13, 2021

-nodes=1 #4

5919ac9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate the humio client from cluster methods, move the kubernetes-related methods to a separate package and add initial unit tests #4

Separate the humio client from cluster methods, move the kubernetes-related methods to a separate package and add initial unit tests #4

jswoods commented Mar 10, 2020

jswoods commented Mar 10, 2020

SaaldjorMike commented Mar 10, 2020

jswoods commented Mar 10, 2020

jswoods commented Mar 12, 2020

jswoods Mar 12, 2020

SaaldjorMike Mar 12, 2020

SaaldjorMike Mar 12, 2020

jswoods Mar 12, 2020

SaaldjorMike left a comment

SaaldjorMike Mar 12, 2020

jswoods Mar 12, 2020

Separate the humio client from cluster methods, move the kubernetes-related methods to a separate package and add initial unit tests #4

Separate the humio client from cluster methods, move the kubernetes-related methods to a separate package and add initial unit tests #4

Conversation

jswoods commented Mar 10, 2020

jswoods commented Mar 10, 2020

SaaldjorMike commented Mar 10, 2020

jswoods commented Mar 10, 2020

jswoods commented Mar 12, 2020

jswoods Mar 12, 2020

Choose a reason for hiding this comment

SaaldjorMike Mar 12, 2020

Choose a reason for hiding this comment

SaaldjorMike Mar 12, 2020

Choose a reason for hiding this comment

jswoods Mar 12, 2020

Choose a reason for hiding this comment

SaaldjorMike left a comment

Choose a reason for hiding this comment

SaaldjorMike Mar 12, 2020

Choose a reason for hiding this comment

jswoods Mar 12, 2020

Choose a reason for hiding this comment