Adding statefulset functionality #44

SteveKMin · 2019-04-04T16:06:38Z

This is my first pass at adding statefulsets to wave. Most of the code mirrors the deployment controller, including the tests.

Some tests in the core pkg run against a deployment object, I was not sure if I should create another with a stateful set object.

pusher-ci · 2019-04-04T16:06:47Z

Hi @SteveKMin. Thanks for your PR.

I'm waiting for a pusher member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

JoelSpeed · 2019-04-04T16:15:38Z

I've had an initial scan over this and it looks great! 😄
I'm going to check it out tomorrow and have a proper read through and will get back to you once I've done so.

/ok-to-test

JoelSpeed

@SteveKMin Looks mostly good, got a few changes to request though mainly around consistency with the rest of the codebase and a hint on fixing the tests 😄

Let me know if you have any questions

JoelSpeed · 2019-04-05T14:46:36Z

pkg/controller/statefulset/stateful_controller_test.go

@@ -0,0 +1,361 @@
+package statefulset


This file is misnamed, should be statefulset_controller_test.go.

Could you also please add the license header to this file.

JoelSpeed · 2019-04-05T14:48:25Z

pkg/controller/statefulset/stateful_controller_test.go

+	var c client.Client
+	var m utils.Matcher
+
+	var sts *appsv1.StatefulSet


I'd prefer if this variable were called statefulset rather than sts, then it is consistent with the deployment controller 😃

JoelSpeed · 2019-04-05T14:50:00Z

config/rbac/manager_role.yaml

+  - create
+  - update
+  - patch
+  - delete


This should have a new line at the end, can you try running make manifests and committing the result?

pkg/controller/statefulset/statefulset_controller.go

JoelSpeed · 2019-04-05T14:52:13Z

pkg/controller/statefulset/statefulset_controller_suite_test.go

+
+func TestMain(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Wave Controller Suite")


The tests won't work right now as we recently changed this line to include JUnit reporters

Suggested change

RunSpecs(t, "Wave Controller Suite")

RunSpecsWithDefaultAndCustomReporters(t, "Wave Controller Suite", reporters.Reporters())

You'll also need to import "github.com/pusher/wave/test/reporters"

JoelSpeed · 2019-04-05T14:54:10Z

pkg/core/types.go

@@ -82,3 +82,23 @@ func (d *deployment) SetPodTemplate(template *corev1.PodTemplateSpec) {
 func (d *deployment) DeepCopy() podController {
 	return &deployment{d.Deployment.DeepCopy()}
 }
+


It would be nice to have some comments through the podController stuff that's been added recently but we don't have any right now so can leave them til later

SteveKMin · 2019-04-09T15:47:53Z

I think I've applied the changes you've requested. I did not comment podcontroller because I could not figure out a good wording for describing it 🙁.

I ran the make manifest command, but for whatever reason, I need to replace every instance of $(GO) with go and likewise for the other commands in the makefile to run locally.

Edit: A coworker has helped me resolve my local environment, I can test locally now.

Edit2: I see I will have to work on some of the tests failing as they assume deployment objects. I will work on those now.

Edit3:
I'm getting some errors that I'm not quite sure how to solve. I'm guessing it has something to do with the testing suite and how the deployment/statefulset objects are being handled?
Getting this error once

    <[]v1.OwnerReference | len:1, cap:1>: [
        {
            APIVersion: "apps/v1",
            Kind: "Deployment",
            Name: "example",
            UID: "decd8fb1-5ae2-11e9-830a-0a580a101331",
            Controller: false,
            BlockOwnerDeletion: true,
        },
    ]
to contain element matching
    <v1.OwnerReference>: {
        APIVersion: "apps/v1",
        Kind: "StatefulSet",
        Name: "example",
        UID: "decd8fb1-5ae2-11e9-830a-0a580a101331",
        Controller: false,
        BlockOwnerDeletion: true,
    }

I'm getting this error case for 4 of the tests

object is being deleted: statefulsets.apps "example" already exists

JoelSpeed · 2019-04-10T11:03:05Z

For your OwnerReferences problem, I think this is because the code here does not handle the different types of objects https://github.com/pusher/wave/blob/819439c729036887a3da70c683307b387bfe3651/pkg/core/owner_references.go#L122-L134

Could try adding a switch on the type to set the right Kind (maybe even inside kindOf if you feel like it)?

// kindOf returns the Kind of the given object as a string
func kindOf(obj Object) string {
	switch obj.(type) {
	case *corev1.ConfigMap:
		return "ConfigMap"
	case *corev1.Secret:
		return "Secret"
	case *deployment:
		return "Deployment"
	case *statefulset:
		return "StatefulSet"
	default:
		return "Unknown"
	}
}

I quickly tested that change and it makes the rest of the tests pass too, which is interesting and kind of sad 😢

SteveKMin · 2019-04-10T15:27:06Z

Ah that makes sense. I've done a 2 case switch statement to handle the getOwnerReference(obj podController). I wasn't sure to make it an if or a switch, will there be more PodController objects?

In addition, I'm seeing inconsistent test behavior now. Locally I was getting different test failures every time, I was thinking it was just an error with my environment. Here a couple of the different failures I saw (all of these are from different runs on Go 1.12).

        Expected
            <[]v1.OwnerReference | len:1, cap:1>: [
                {
                    APIVersion: "apps/v1",
                    Kind: "Deployment",
                    Name: "example",
                    UID: "850a2f40-5ba2-11e9-9120-88e9fe7a0a04",
                    Controller: false,
                    BlockOwnerDeletion: true,
                },
            ]
        not to contain element matching
            <v1.OwnerReference>: {
                APIVersion: "apps/v1",
                Kind: "Deployment",
                Name: "example",
                UID: "850a2f40-5ba2-11e9-9120-88e9fe7a0a04",
                Controller: false,
                BlockOwnerDeletion: true,
            }

        Expected success, but got an error:
            <*errors.StatusError | 0xc0003f2d80>: {
                ErrStatus: {
                    TypeMeta: {Kind: "", APIVersion: ""},
                    ListMeta: {SelfLink: "", ResourceVersion: "", Continue: ""},
                    Status: "Failure",
                    Message: "Operation cannot be fulfilled on deployments.apps \"example\": the object has been modified; please apply your changes to the latest version and try again",
                    Reason: "Conflict",
                    Details: {Name: "example", Group: "apps", Kind: "deployments", UID: "", Causes: nil, RetryAfterSeconds: 0},
                    Code: 409,
                },
            }
            Operation cannot be fulfilled on deployments.apps "example": the object has been modified; please apply your changes to the latest version and try again

JoelSpeed

The testing inconsistencies are interesting. The latter is a sporadic thing that happens every so often where the CPU is a bit too slow for the test timeouts, I haven't worked out a good way to fix that yet apart from --flake-attempts as a flag to ginkgo.

The former doesn't seem to happen on my machine, but I'll try running the tests a few times and see if I can find anything, there are a few oddities in these tests at present, they need a little work

JoelSpeed · 2019-04-11T09:32:03Z

pkg/core/owner_references.go

+	case "Deployment":
+		return metav1.OwnerReference{
+			APIVersion:         "apps/v1",
+			Kind:               "Deployment",


Any reason not to just substitute the string here for kindOf(obj), then this switch would be unnecessary right?

Suggested change

Kind: "Deployment",

Kind: kindOf(obj),

JoelSpeed · 2019-04-11T09:33:22Z

test/utils/matchers.go

@@ -167,12 +167,16 @@ func WithOwnerReferences(matcher gtypes.GomegaMatcher) gtypes.GomegaMatcher {
 	}, matcher)
 }

-// WithPodTemplateAnnotations returns the deployments PodTemplate's annotations
+// WithPodTemplateAnnotations returns the PodTemplate's annotations
 func WithPodTemplateAnnotations(matcher gtypes.GomegaMatcher) gtypes.GomegaMatcher {
 	return gomega.WithTransform(func(obj Object) map[string]string {
 		dep, ok := obj.(*appsv1.Deployment)


Could this not cast to the podController interface? Would get rid of the need for this double if-else?

Or better still just make the anonymous function accept a podController instead of Object and then the tests will panic by themselves if the incorrect interface is passed

podController is defined as a private interface. Do you want it to be public so it can be used in this way? We may have to refactor the tests to send a pod controller instead of deployment and statefulset types.

Yeah, I think that would probably be better to do. Let's make PodController public and I think the tests should just work anyway? As in I think we are already passing PodControllers in in most places. Lets give it a go and see what happens?

Are you thinking something like this?

func WithPodTemplateAnnotations(matcher gtypes.GomegaMatcher) gtypes.GomegaMatcher { return gomega.WithTransform(func(obj core.PodController) map[string]string { return obj.GetPodTemplate().GetAnnotations() }, matcher) }

If so

Error: Transform function expects 'core.PodController' but we have '*v1.Deployment'

happens on about ~20 times when the eventually function uses utils.WithPodTemplateAnnotations(HaveKey(core.ConfigHashAnnotation))

Ok, I've had a deeper look at this, definitely not worth trying to use podController here, it's going to cause so many issues

Instead can we make it a switch as below? I think it will look cleaner, WDYT?

return gomega.WithTransform(func(obj Object) map[string]string { switch obj.(type) { case *appsv1.Deployment: return obj.(*appsv1.Deployment).Spec.Template.GetAnnotations() case *appsv1.StatefulSet: return obj.(*appsv1.StatefulSet).Spec.Template.GetAnnotations() default: panic("unknown pod template type") } }, matcher)

Yeah, thats much easier to read. I've updated the PR.

JoelSpeed

I'm happy now with these changes, thanks for all your work on this.

Just going to retest the 1.11 suite as it failed for some reason and then I'll merge
/test 1.11

Would you like to rebase and squash the commits at all before we merge? Otherwise I'll just squash it as I merge

JoelSpeed · 2019-04-23T09:30:42Z

/retest

JoelSpeed · 2019-04-23T09:34:55Z

/retest

JoelSpeed · 2019-04-23T09:55:19Z

/retest

JoelSpeed · 2019-04-23T10:12:54Z

/retest

JoelSpeed · 2019-04-24T08:14:09Z

/retest

pusher-ci · 2019-04-24T08:18:18Z

@SteveKMin: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-wave-test-ginkgo	`a8b8799`	link	`/test ginkgo`
pull-wave-test-1.11	`8fbaa64`	link	`/test 1.11`

Full PR test history

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

JoelSpeed · 2019-04-24T08:42:15Z

There is something not right with these tests. The Deployment controller tests don't flake in the same way as the StatefulSet ones. I'm going to spend some time this morning investigating this to see if I can work out why

JoelSpeed · 2019-04-24T09:31:12Z

/build docker

I'm working on the flaking tests and am thinking it's probably best to follow up with a separate PR for fixing those, for the meantime I'm going to build a docker image and manually test this on one of our clusters

JoelSpeed · 2019-04-24T09:32:45Z

/build docker

JoelSpeed

Im happy enough with this that it's working, will get it merged!

pusher-ci added the needs-ok-to-test label Apr 4, 2019

pusher-ci added the size/XL label Apr 4, 2019

JoelSpeed requested a review from a team April 4, 2019 16:08

pusher-ci added ok-to-test and removed needs-ok-to-test labels Apr 4, 2019

JoelSpeed reviewed Apr 5, 2019

View reviewed changes

mcristina422 mentioned this pull request Apr 9, 2019

Improve local development #46

Closed

JoelSpeed requested changes Apr 11, 2019

View reviewed changes

JoelSpeed previously approved these changes Apr 23, 2019

View reviewed changes

Statefulset functionality with tests

8fbaa64

SteveKMin dismissed JoelSpeed’s stale review via 8fbaa64 April 23, 2019 15:31

SteveKMin force-pushed the stateful branch from aa5d73b to 8fbaa64 Compare April 23, 2019 15:31

JoelSpeed mentioned this pull request Apr 24, 2019

Attempt to fix flaking tests in Deployment and StatefulSet controllers #49

Merged

JoelSpeed approved these changes Apr 24, 2019

View reviewed changes

JoelSpeed merged commit b03cb69 into wave-k8s:master Apr 24, 2019

JoelSpeed mentioned this pull request Apr 24, 2019

Support for StatefulSets #22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding statefulset functionality #44

Adding statefulset functionality #44

SteveKMin commented Apr 4, 2019 •

edited

Loading

pusher-ci commented Apr 4, 2019

JoelSpeed commented Apr 4, 2019

JoelSpeed left a comment

JoelSpeed Apr 5, 2019

JoelSpeed Apr 5, 2019

JoelSpeed Apr 5, 2019

JoelSpeed Apr 5, 2019

JoelSpeed Apr 5, 2019

SteveKMin commented Apr 9, 2019 •

edited

Loading

JoelSpeed commented Apr 10, 2019 •

edited

Loading

SteveKMin commented Apr 10, 2019 •

edited

Loading

JoelSpeed left a comment

JoelSpeed Apr 11, 2019

JoelSpeed Apr 11, 2019

JoelSpeed Apr 11, 2019

SteveKMin Apr 16, 2019

JoelSpeed Apr 16, 2019

SteveKMin Apr 16, 2019

JoelSpeed Apr 18, 2019

SteveKMin Apr 18, 2019

JoelSpeed left a comment

JoelSpeed commented Apr 23, 2019

JoelSpeed commented Apr 23, 2019

JoelSpeed commented Apr 23, 2019

JoelSpeed commented Apr 23, 2019

JoelSpeed commented Apr 24, 2019

pusher-ci commented Apr 24, 2019

JoelSpeed commented Apr 24, 2019

JoelSpeed commented Apr 24, 2019

JoelSpeed commented Apr 24, 2019

JoelSpeed left a comment

	RunSpecs(t, "Wave Controller Suite")
	RunSpecsWithDefaultAndCustomReporters(t, "Wave Controller Suite", reporters.Reporters())

Adding statefulset functionality #44

Adding statefulset functionality #44

Conversation

SteveKMin commented Apr 4, 2019 • edited Loading

pusher-ci commented Apr 4, 2019

JoelSpeed commented Apr 4, 2019

JoelSpeed 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

Choose a reason for hiding this comment

SteveKMin commented Apr 9, 2019 • edited Loading

JoelSpeed commented Apr 10, 2019 • edited Loading

SteveKMin commented Apr 10, 2019 • edited Loading

JoelSpeed 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

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

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed commented Apr 23, 2019

JoelSpeed commented Apr 23, 2019

JoelSpeed commented Apr 23, 2019

JoelSpeed commented Apr 23, 2019

JoelSpeed commented Apr 24, 2019

pusher-ci commented Apr 24, 2019

JoelSpeed commented Apr 24, 2019

JoelSpeed commented Apr 24, 2019

JoelSpeed commented Apr 24, 2019

JoelSpeed left a comment

Choose a reason for hiding this comment

SteveKMin commented Apr 4, 2019 •

edited

Loading

SteveKMin commented Apr 9, 2019 •

edited

Loading

JoelSpeed commented Apr 10, 2019 •

edited

Loading

SteveKMin commented Apr 10, 2019 •

edited

Loading