wave-k8s / wave

Kubernetes configuration tracking controller
Apache License 2.0
661 stars 81 forks source link

Adding statefulset functionality #44

Closed SteveKMin closed 5 years ago

SteveKMin commented 5 years ago

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 commented 5 years ago

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
JoelSpeed commented 5 years ago

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

SteveKMin commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

/retest

JoelSpeed commented 5 years ago

/retest

JoelSpeed commented 5 years ago

/retest

JoelSpeed commented 5 years ago

/retest

JoelSpeed commented 5 years ago

/retest

pusher-ci commented 5 years ago

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

Test name Commit Details Rerun command
pull-wave-test-ginkgo a8b87996a6a74576acc6c6d2a624f26425be7a27 link /test ginkgo
pull-wave-test-1.11 8fbaa640cf159e5205d8a8831b105424e32556e1 link /test 1.11

Full PR test history

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
JoelSpeed commented 5 years ago

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 commented 5 years ago

/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 commented 5 years ago

/build docker