Add 'version' field to ESC. Auto-generate Elasticsearch image details #205

munnerz · 2018-01-13T00:22:00Z

What this PR does / why we need it:

This PR adds a version field to an ElasticsearchCluster. It is used to auto-default the ElasticsearchImageSpec in its absence. If a user wants to override the image to be used and take manual control, they can specify their own fields as before.

The new version field should be required regardless whether the image is manually specified, as in future it can be used to help influence upgrade strategy. It will be up to the administrator to update the image and listed version in unison.

In future we should make the default image repository and uid be configurable.

Special notes for your reviewer:

Ensuring the spec.version field is set will be in a follow up after #199 is merged

Release note:

Add a new spec.version field to ElasticsearchCluster and auto-default the image version and repository to an official upstream image.

munnerz · 2018-01-15T18:54:03Z

/test e2e

munnerz · 2018-01-15T19:06:19Z

/test e2e

wallrj

Looks good, but similar questions as for #206

If I omit the ElasticSearch image spec when I create my cluster resource, what happens when I try and change the new Version attribute?
Do I then need to also provide the full image spec or will the appropriate default image spec be selected for that version?

wallrj · 2018-01-16T14:12:57Z

pkg/controllers/elasticsearch/nodepool/resources.go

+	if esImage.FsGroup == 0 &&
+		esImage.ImageSpec.PullPolicy == "" &&
+		esImage.ImageSpec.Repository == "" &&
+		esImage.ImageSpec.Tag == "" {


Why check for FsGroup here?

Could you instead compare esImage.ImageSpec to the zero value of ImageSpec?

What if I supply FsGroup, but omit the ImageSpec? It looks like the pod will fail to start.

Changing the version attribute is expected use. This should cause navigator to begin an upgrade/downgrade/report an error that upgrade/downgrade isn't possible.

Nope - provide the version and everything else is handled for you. As soon as you specify either FsGroup, PullPolicy, Repository or Tag however, you've gone into manual mode. Manual mode might be removed at some point in the future (as discussed today)

As above, fsGroup is checked because if specified, you've switched to 'manual mode' (and so all the others are required too). Perhaps this will make a bit more sense once we get the validation PR (Add basic ElasticsearchCluster resource validation #199) merged.

Yep potentially we could just check the zero values - or as you suggested earlier, make this field a pointer.

Yep you are correct. FsGroup = manual mode. Remember users never see imageSpec as a field (as it's json tag is: json:",inline")

jetstack-ci-bot · 2018-01-16T23:21:31Z

@munnerz PR needs rebase

munnerz · 2018-01-17T10:35:11Z

/test e2e

wallrj · 2018-01-17T11:05:33Z

pkg/controllers/elasticsearch/nodepool/util_test.go

+		},
+		// We don't error here - if the specified version is non semver but the
+		// user has specified a manual image, validation should detect the
+		// invalid semver and fail.


Shouldn't we always error with non-semver version?
Or does this comment mean that the non-semver version will be caught and rejected by resource validation in #199?

Precisely that - yep. It seems odd to return an error in this case. The function should be returning the image that should be used for the spec, and the image is explicitly defined in the manifest, so there's not really an error.

There is however an error with the overall state/spec of the cluster, and validation should be responsible for detecting this. I guess more gates isn't a bad thing, but I think the gate might need to be moved to some other function if we do add it. I'm working on validation now and will expand that PR to encompass new fields added here once merged.

wallrj

Thanks @munnerz

Looks great. I like your change to make ESImage a reference. And happy to see the unit tests. 👍

I've always been taught to convert to rich types as soon as possible, so
I'd be inclined to make the Version *semver.Version,
assuming that it's simple to register a JSON unmarshaller for that type.
That way we wouldn't have to parse the Semver everytime we need to use it.

Up to you. Merge when you're happy with everything.

wallrj · 2018-01-17T11:07:05Z

pkg/apis/navigator/types.go

@@ -89,10 +89,11 @@ type ElasticsearchClusterList struct {
 }

 type ElasticsearchClusterSpec struct {
+	Version   string


I'd be inclined to make this type *semver.Version ?

I'm not sure we can just make this some other struct. The Golang JSON parser would fail because string is not a structure, which it would expect the field to be.

That said, UnmarshalJSON etc. is defined so maybe it would. I'll take a look. Depending on external types in our apis can get pretty prickly though - if we update our dependency it could cause breaking API changes (and we don't 'control' the downstream code, which makes it harder to fix/manage)

munnerz · 2018-01-17T12:57:46Z

/test e2e

munnerz · 2018-01-17T13:12:33Z

@wallrj I've updated the PR to make the new Version field in our API into a semver.Version.

The unit tests obviously work with Golang structs (and pass), but they don't test the unmarshalling from a string. I've added a version parameter to the Elasticsearch Cluster test fixture for our e2e tests in order to verify this works correctly, and from what I can see it is passing happily.

Once #199 merges, I'll add further validation to this to stop invalid values being submitted to the API.

munnerz · 2018-01-17T18:37:00Z

I've added additional validation to this now, which ensures that version is specified.

Unmarshaling will error if an invalid string is provided, but it doesn't error if no string is provided (it returns nil). In order to handle this case, I check for an empty semver.Version struct during validation and throw a Required error in this case.

jetstack-ci-bot · 2018-01-17T23:25:03Z

@munnerz PR needs rebase

… and allow override.

…age.

wallrj

Thanks @munnerz

Just take another look at those error cases in the tests I think they need modifying.

Then merge when happy.

wallrj · 2018-01-18T10:17:44Z

pkg/apis/navigator/validation/elasticsearch_test.go

+			NodePools: []navigator.ElasticsearchClusterNodePool{validNodePool, validNodePool},
+			Pilot:     validPilotImage,
+			Image:     validESImage,
+		},


Version is missing from all the other error cases too.
So it's not clear whether they're failing because of version or minimummaster

wallrj

/lgtm

jetstack-ci-bot · 2018-01-18T12:38:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [wallrj]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jetstack-ci-bot · 2018-01-18T12:52:50Z

@munnerz PR needs rebase

jetstack-ci-bot · 2018-01-18T20:36:58Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @munnerz @wallrj

jetstack-ci-bot · 2018-01-18T20:43:59Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @munnerz @wallrj

jetstack-ci-bot · 2018-01-18T21:01:09Z

/test all [submit-queue is verifying that this PR is safe to merge]

jetstack-ci-bot · 2018-01-18T21:17:03Z

Automatic merge from submit-queue.

jetstack-bot added the release-note label Jan 13, 2018

jetstack-ci-bot assigned wallrj Jan 13, 2018

jetstack-bot added the size/M label Jan 13, 2018

munnerz added this to the v0.1 milestone Jan 13, 2018

munnerz force-pushed the add-es-version branch from 3d6d08b to c153a55 Compare January 13, 2018 00:36

wallrj suggested changes Jan 16, 2018

View reviewed changes

jetstack-ci-bot added the needs-rebase label Jan 16, 2018

jetstack-bot added size/L and removed size/M labels Jan 17, 2018

munnerz force-pushed the add-es-version branch from bde96d4 to b7037ae Compare January 17, 2018 10:34

jetstack-ci-bot removed the needs-rebase label Jan 17, 2018

wallrj reviewed Jan 17, 2018

View reviewed changes

wallrj approved these changes Jan 17, 2018

View reviewed changes

munnerz force-pushed the add-es-version branch from 3558b40 to fc7e786 Compare January 17, 2018 13:08

munnerz force-pushed the add-es-version branch from fc7e786 to 110fab4 Compare January 17, 2018 17:39

munnerz added the area/elasticsearch label Jan 17, 2018

jetstack-ci-bot added the needs-rebase label Jan 17, 2018

munnerz added 5 commits January 18, 2018 00:45

Add 'version' field to ESC. Auto-generate Elasticsearch image details…

40493bd

… and allow override.

Switch ElasticsearchImage field to a pointer

1de325d

Refactor for change to ES image field. Add unit test for computing im…

7299e27

…age.

Embed semver.Version within our API types

594f9e5

Add validation to require ES version specified

6fdfa3b

munnerz added 2 commits January 18, 2018 00:51

Update examples and testdata

9c24bcf

Update to use typed PullPolicy

f5382aa

munnerz force-pushed the add-es-version branch from 354149d to f5382aa Compare January 18, 2018 00:52

jetstack-ci-bot removed the needs-rebase label Jan 18, 2018

wallrj approved these changes Jan 18, 2018

View reviewed changes

munnerz added 2 commits January 18, 2018 10:29

Add Version field to errorCases in TestValidateElasticsearchClusterSpec

29d35da

Run dep ensure

77feb48

wallrj approved these changes Jan 18, 2018

View reviewed changes

jetstack-bot added the lgtm label Jan 18, 2018

jetstack-ci-bot added the approved label Jan 18, 2018

jetstack-ci-bot added the needs-rebase label Jan 18, 2018

Merge branch 'master' into add-es-version

e41762b

jetstack-ci-bot removed lgtm needs-rebase labels Jan 18, 2018

munnerz added the lgtm label Jan 18, 2018

Fix elasticsearch nodepool utils

cc380a7

jetstack-ci-bot removed the lgtm label Jan 18, 2018

munnerz added the lgtm label Jan 18, 2018

jetstack-ci-bot merged commit e7dc7a2 into jetstack:master Jan 18, 2018

wallrj mentioned this pull request Feb 1, 2018

Need a way to specify the CassandraCluster.Version #234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'version' field to ESC. Auto-generate Elasticsearch image details #205

Add 'version' field to ESC. Auto-generate Elasticsearch image details #205

munnerz commented Jan 13, 2018

munnerz commented Jan 15, 2018

munnerz commented Jan 15, 2018

wallrj left a comment

wallrj Jan 16, 2018

munnerz Jan 16, 2018

jetstack-ci-bot commented Jan 16, 2018

munnerz commented Jan 17, 2018

wallrj Jan 17, 2018

munnerz Jan 17, 2018

wallrj left a comment

wallrj Jan 17, 2018

munnerz Jan 17, 2018

munnerz Jan 17, 2018

munnerz commented Jan 17, 2018

munnerz commented Jan 17, 2018

munnerz commented Jan 17, 2018 •

edited

Loading

jetstack-ci-bot commented Jan 17, 2018

wallrj left a comment

wallrj Jan 18, 2018

wallrj left a comment

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

Add 'version' field to ESC. Auto-generate Elasticsearch image details #205

Add 'version' field to ESC. Auto-generate Elasticsearch image details #205

Conversation

munnerz commented Jan 13, 2018

munnerz commented Jan 15, 2018

munnerz commented Jan 15, 2018

wallrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetstack-ci-bot commented Jan 16, 2018

munnerz commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munnerz commented Jan 17, 2018

munnerz commented Jan 17, 2018

munnerz commented Jan 17, 2018 • edited Loading

jetstack-ci-bot commented Jan 17, 2018

wallrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

jetstack-ci-bot commented Jan 18, 2018

munnerz commented Jan 17, 2018 •

edited

Loading