[spec][backwards incompatible] drop support for v1beta1 #331

surajssd · 2018-08-08T12:31:09Z

This commit removes the support for v1beta1 resource type. The
only supported version is only v1beta2.

krnowak · 2018-08-09T11:09:56Z

There seem to be a misunderstanding what this task was about. And the fact that we have this customVersion hack does not really help here.

So, so far our CRD has version habitat.sh/v1beta1. At some point we wanted to make an incompatible change (we switched from using Deployments to StatefulSets), so we wanted to introduce version habitat.sh/v1beta2. That didn't work - k8s at the time didn't have support for multiple versions of the CRD. It means that registering multiple versions failed and we needed to have it, because we still wanted to support the old version (with Deployments).

That's why we came up with the customVersion thing. It is a toplevel field which is our hacky take on CRD versioning. So if the field is absent in the manifest or if it is equal to v1beta1 then we expect yamls like:

apiVersion: habitat.sh/v1beta1
…
customVersion: v1beta1 # or unspecified
spec:
  image: habitat/redis-hab
  count: 1
  …

But if the customVersion field is equal to v1beta2 then we expect something like:

apiVersion: habitat.sh/v1beta2
…
customVersion: v1beta2
spec:
  v1beta2:
    image: habitat/redis-hab
    count: 1
    …

I suppose if we wanted to make another incompatible change in the manifest, we would add v1beta3 as a possible value for customVersion and v1beta3 field under spec, just like for v1beta2.

At some point we declared v1beta1 customVersion as deprecated and now we want to remove it. This currently means that customVersion field must be specified and must be equal to v1beta2 (until we introduce something like v1beta3 then this value would also be valid). So unspecified customVersion field or customVersion = v1beta1 are not supported anymore. This also means that the official CRD version is still v1beta1.

This customVersion hack is a thing we will need to keep until all versions of k8s the operator supports support CRD versioning. Basic form of CRD versioning arrived to k8s 1.11, not sure if it is in any way usable for us - I haven't yet investigated that. So you can see that this hack will stay with us a little while longer.

So about your changes: there is no need to change the CRD version. Keeping the CRD version intact should drop the need for renaming the directories, fixing the examples (the v1beta1 example still needs to be dropped though), resource names in tests, import paths in source files and some paths in bash scripts.

Oh, and thanks for the tests, they are always useful!

surajssd · 2018-08-10T10:45:01Z

@krnowak made changes as you requested PTAL!

krnowak · 2018-08-13T08:25:32Z

cmd/habitat-operator/main.go

 	if err := v1beta2(ctx, &wg, cSets, logger); err != nil {
 		level.Error(logger).Log("msg", err)
+		cancelFunc()


Not sure what's the point of this cancel function invocation as returning immediately after that will all os.Exit(…) which in turn just kills all the running goroutines. So what we can do here is one of the following:

just drop cancelFunc() here

otherwise it feels to be pointless, as other goroutines will likely have no time to respond to the cancellation

keep it

it probably won't do any harm anyway

keep it and add a wait on wg.Wait()

that probably would be ideal from the "cleanup" point of view, but it may hang if:

the operator failed to register the CRD for some other reason than the CRD already being registered, or if

the operator failed to create a controller

we probably could add wg.Done() in the v1beta2 function in the error paths, but that would basically be the same as keeping only the cancelFunc invocation from goroutines point of view, so no good

after writing all of the above I had a closer look at the v1beta2 function and I think that calling cancelFunc() is pointless for another reason - both error paths in v1beta happen before ctx is passed anywhere, so if an error happens then nothing listens on the ctx.Done(), so calling cancelFunc() here has no effects.

So I'd say, just drop it, please.

I agree that program fails so all the resources will be reclaimed, but go vet keeps complaining since it does not find cancelFunc being called in all of it's exit paths.

Added defer cancelFunc(), we can call cancelFunc() multiple times without a problem. Here IMO we could follow good practices of golang so even if the code is moved or refactored the reclaimation still exists.

Hmph, go vet go shmet. Alright, let's keep it like that.

krnowak · 2018-08-13T08:35:08Z

examples/standalone/habitat.yml

@@ -5,7 +5,6 @@ metadata:
  labels:
    source: operator-example
    app: standalone-habitat
-customVersion: v1beta2


This needs to stay as is - with dropping the customVersion for v1beta1, this field became mandatory. Lack of this field had the same meaning as it being specified to v1beta1. Now that we are dropping v1beta1 custom version, the customVersion field basically stops being optional.

We don't want to make an unspecified customVersion field the same as specifying it as v1beta2, because all the valid (but deprecated) v1beta1 manifests will suddenly be interpreted as v1beta2 which is just wrong.

krnowak · 2018-08-13T08:37:01Z

pkg/apis/habitat/v1beta1/types.go

@@ -45,21 +45,12 @@ type Habitat struct {
 	// multiple versions of a CRD.  It encodes the actual version of the type, so
 	// that controllers can decide whether to discard an object if the version
 	// doesn't match.
-	// When absent, it defaults to v1beta1.
+	// When absent, it defaults to v1beta2.


Please drop this line and the one below (+optional). The field became mandatory. It needs to stay as a pointer though, unfortunately, but that's hacks for us.

krnowak · 2018-08-13T08:42:43Z

pkg/controller/v1beta2/controller.go

-	if err := checkCustomVersionMatch(hab); err != nil {
-		level.Debug(hc.logger).Log("msg", err)
+	if err := checkCustomVersionMatch(hab.CustomVersion); err != nil {
+		level.Error(hc.logger).Log("msg", err)


I'm not sure if this should be an error, but it certainly should be something more that debug. Info maybe? The reason it shouldn't be an error is that if we add another customVersion (say v1beta3) then we don't want v1beta2 controller spewing errors every time we send a valid v1beta3 manifest to the cluster.

I guess it would be nice to have a single way of catching wrong customVersion fields that would print the errors, but that is outside of the scope of this PR.

Also, could you reword a bit a message of the commit that has those changes? It is not really only adding the tests to changed functions. It makes some functions easier to test too, so you added those tests as well.

When we add support for v1beta3 won't the code of checkCustomVersionMatch change?

Also this is user facing error which should be shown to user, but this rather gets dumped in the operator logs. I have summarized some of my thoughts here #335

Also, could you reword a bit a message of the commit that ...

Yes would take care of it.

we add support for v1beta3 won't the code of checkCustomVersionMatch change?

Unlikely, but who knows? So far, when we added a new custom version we basically duplicated the controller and then made some changes relevant for the new custom version. We did so because we wanted to avoid unintended changes in the older controller when changing something in the newer one.

As of user facing errors - maybe resolving this issue would help: #185

krnowak · 2018-08-13T08:44:12Z

pkg/controller/v1beta2/controller_test.go

+	"k8s.io/apimachinery/pkg/types"
+)
+
+func Test_habitatKeyFromLabeledResource(t *testing.T) {


Just TestHabitatKeyFromLabeledResource.

krnowak · 2018-08-13T08:53:34Z

pkg/controller/v1beta2/utils_test.go

+	}{
+		{
+			name:    "nothing specified in the version",
+			wantErr: false,


This should be true. And consider specifying arg as nil explicitly.

krnowak · 2018-08-13T09:22:57Z

pkg/controller/v1beta2/utils_test.go

+
+import "testing"
+
+func Test_checkCustomVersionMatch(t *testing.T) {


TestCheckCustomVersionMatch.

krnowak · 2018-08-13T09:26:15Z

test/e2e/v1beta1/operator_test.go

@@ -283,8 +283,8 @@ func TestPersistentStorage(t *testing.T) {
 	}
 }

-func TestV1beta1(t *testing.T) {
-	h, err := utils.ConvertHabitat("resources/v1beta1/habitat.yml")
+func TestV1beta2(t *testing.T) {


I think we can drop this test altogether. The v1beta2 is rather tested extensively in other test cases.

krnowak · 2018-08-13T09:26:53Z

test/e2e/v1beta1/resources/v1beta2/habitat.yml

@@ -0,0 +1,17 @@
+apiVersion: habitat.sh/v1beta2


If we decide to drop TestV1beta1 test instead of renaming it to TestV1beta1 then this file will be unnecessary and can be dropped too.

krnowak · 2018-08-13T09:29:19Z

pkg/controller/v1beta2/controller.go

@@ -79,9 +79,7 @@ const (
 	// Event messages.
 	messageValidationFailed = "Failed validating Habitat"
 	messageCMCreated        = "Created peer IP ConfigMap"
-	messageCMUpdated        = "Updated peer IP ConfigMap"


Could you figure out why these are unused? Maybe we should use them somewhere instead of dropping those? Maybe there will be some notes in some PR that introduced those fields. CC @kosyfrances

Also, this change could be done in a separate commit.

This commit removes the support for `v1beta1` resource type. The only supported version is only `v1beta2` inside spec. Also updates the function definition of `checkCustomVersionMatch` to accept string pointer, which is used to verify the supported version of the CR definition spec. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd · 2018-08-14T12:44:01Z

@krnowak all changes done!

krnowak · 2018-08-15T07:11:00Z

LFAD, thanks!

indradhanush

A late review. More like open questions than a review really.

indradhanush · 2018-08-17T05:36:58Z

doc/design.md

-offer support for persistence.
-
-Because this is a backwards-incompatible change, it was decided to implement it
-under a different custom version.


Would it have made sense to keep the message about customVersion and why it was implemented in the first place for historical reasons? Ofcourse the difference between v1beta1 and v1beta2 is irrelevant now. :)

I don't know if we should add that back again, but if you recommend so, I can do that? Not sure if current design doc should be a place to keep it though. I can add a line which talks about only customVersion is hack since the versions <=1.10 don't support versioning of CRDs, and to get around it customVersion was added?

indradhanush · 2018-08-17T05:37:36Z

examples/v1beta1/README.md

-time, we defined the `customVersion` field, and let controllers decide which
-objects to act upon based on its value (e.g. the `v1beta2` controller will
-ignore `Habitat` objects whose `customVersion` field is not `v1beta2`.
-


More like this would have been the part worth preserving probably.

removed since, it is not relevant to user in current state. But if we had mechanism like kubernetes docs website has where you can select the version of kubernetes and then docs reflect that version, that would have been great.

surajssd changed the title ~~[spec][backwards incompatible] drop support for v1beta1~~ [WIP][spec][backwards incompatible] drop support for v1beta1 Aug 8, 2018

surajssd force-pushed the surajssd/remove-v1beta1 branch 2 times, most recently from fc5bd73 to f83742a Compare August 9, 2018 06:07

surajssd changed the title ~~[WIP][spec][backwards incompatible] drop support for v1beta1~~ [spec][backwards incompatible] drop support for v1beta1 Aug 9, 2018

surajssd force-pushed the surajssd/remove-v1beta1 branch 3 times, most recently from 051f9c9 to f4dd7ee Compare August 10, 2018 10:23

krnowak suggested changes Aug 13, 2018

View reviewed changes

surajssd force-pushed the surajssd/remove-v1beta1 branch from f4dd7ee to 9fd7110 Compare August 13, 2018 15:47

surajssd force-pushed the surajssd/remove-v1beta1 branch from 9fd7110 to 6f60c35 Compare August 14, 2018 12:43

krnowak approved these changes Aug 15, 2018

View reviewed changes

krnowak merged commit 495365e into habitat-sh:master Aug 15, 2018

krnowak deleted the surajssd/remove-v1beta1 branch August 15, 2018 07:11

indradhanush reviewed Aug 17, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[spec][backwards incompatible] drop support for v1beta1 #331

[spec][backwards incompatible] drop support for v1beta1 #331

surajssd commented Aug 8, 2018

krnowak commented Aug 9, 2018

surajssd commented Aug 10, 2018

krnowak Aug 13, 2018

surajssd Aug 13, 2018

krnowak Aug 14, 2018

krnowak Aug 13, 2018

surajssd Aug 13, 2018 •

edited

Loading

krnowak Aug 13, 2018

surajssd Aug 13, 2018 •

edited

Loading

krnowak Aug 13, 2018

krnowak Aug 13, 2018

surajssd Aug 13, 2018

surajssd Aug 13, 2018 •

edited

Loading

krnowak Aug 14, 2018

krnowak Aug 13, 2018

surajssd Aug 13, 2018

krnowak Aug 13, 2018

surajssd Aug 13, 2018

krnowak Aug 13, 2018

surajssd Aug 13, 2018

krnowak Aug 13, 2018

surajssd Aug 13, 2018

krnowak Aug 13, 2018

surajssd Aug 13, 2018

krnowak Aug 13, 2018

surajssd Aug 14, 2018

surajssd commented Aug 14, 2018

krnowak commented Aug 15, 2018

indradhanush left a comment

indradhanush Aug 17, 2018

surajssd Aug 17, 2018

indradhanush Aug 17, 2018

surajssd Aug 17, 2018


		import "testing"

		func Test_checkCustomVersionMatch(t *testing.T) {

[spec][backwards incompatible] drop support for v1beta1 #331

[spec][backwards incompatible] drop support for v1beta1 #331

Conversation

surajssd commented Aug 8, 2018

krnowak commented Aug 9, 2018

surajssd commented Aug 10, 2018

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

surajssd Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd Aug 13, 2018 • edited Loading

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

surajssd Aug 13, 2018 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd commented Aug 14, 2018

krnowak commented Aug 15, 2018

indradhanush 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

Choose a reason for hiding this comment

surajssd Aug 13, 2018 •

edited

Loading

surajssd Aug 13, 2018 •

edited

Loading

surajssd Aug 13, 2018 •

edited

Loading