implemented router id blacklist

[danielkucera]

fixes #32 and #26

[raffaelespazzoli]

@danielkucera thanks for this contribution. we have other people also asking for this. We will test it as soon as possible.

[David-Igou]

Please regenerate/add the CRD (deploy/crds/redhatcop.redhat.io_keepalivedgroups_crd.yaml) using operator-sdk generate crds (This should be added to the README) and change the username on your commit to your Github username (it is currently root)

Additionally, when spec.blacklistRouterIDs is updated after being created, no updates are propagated by the operator. For example, I created a keepalivedgroup with spec.blacklistRouterIDs: [1] and then updated the value to [1, 3, 3, 2, 5] the operator didn't regenerate configmap/{keepalivedgroup.name} according to the new blacklist:

apiVersion: redhatcop.redhat.io/v1alpha1
kind: KeepalivedGroup
metadata:
  creationTimestamp: "2020-09-14T23:06:06Z"
  generation: 4
  name: keepalivedgroup-test
  namespace: keepalived-operator
  selfLink: /apis/redhatcop.redhat.io/v1alpha1/namespaces/keepalived-operator/keepalivedgroups/keepalivedgroup-test
spec:
  blacklistRouterIDs:
  - 1
  - 3
  - 3
  - 2
  - 5
  interface: ens5
  nodeSelector:
    node-role.kubernetes.io/worker: ""
status:
  conditions:
  - lastTransitionTime: "2020-09-14T23:24:51Z"
    message: Awaiting next reconciliation
    reason: Successful
    status: "True"
    type: ReconcileSuccess
  routerIDs:
    test-keepalived-operator/django-psql-example: 2

[danielkucera]

@David-Igou changes applied, please review.

[David-Igou]

@danielkucera acknowledged, trying to find the time

[danielkucera]

I'm done with this PR, if it is still not okay for you feel free to close or modify as you wish.

[raffaelespazzoli]

@danielkucera I am terribly sorry that we made this process unpleasant for you. This was due to poor communication on our side in what were asking of you. I have been on your side of the fence on other projects and I know how you feel. To try to simplify this process I took your PR and fixed a few things. Surprisingly I was also able to push the changes to your repo. So now the changes are here. I'm good to accept the PR. Could you or @David-Igou run a last test with it?

[raffaelespazzoli]

@danielkucera done.

On Wed, Sep 23, 2020 at 11:22 AM Daniel Kucera notifications@github.com wrote:

@danielkucera commented on this pull request.

In pkg/controller/keepalivedgroup/keepalivedgroup_controller.go https://github.com/redhat-cop/keepalived-operator/pull/33#discussion_r493682009 :

@@ -232,18 +234,30 @@ func (r *ReconcileKeepalivedGroup) Reconcile(request reconcile.Request) (reconci return r.ManageSuccess(instance) }

-func getNamespaceNameKey(obj metav1.Object) string {

• return obj.GetNamespace() + "/" + obj.GetName() -} +// func getNamespaceNameKey(obj metav1.Object) string { +// return obj.GetNamespace() + "/" + obj.GetName() +// }

Please leave this out. Commented out code is not necessary.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/redhat-cop/keepalived-operator/pull/33#pullrequestreview-494779208, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPERXCVQTMHQ365QXLLAZ3SHIHDZANCNFSM4RLJ3UCA .

-- ciao/bye Raffaele

[David-Igou]

@raffaelespazzoli LGTM

[danielkucera]

I'm sorry to inform you that the code doesn't work.

[David-Igou]

@danielkucera can you describe what you are seeing?

[danielkucera]

@David-Igou that it doesn't work for example, note the router IDs and blacklist IDs:

Name:         shz
Namespace:    keepalived-operator
Labels:       <none>
Annotations:  kubectl.kubernetes.io/last-applied-configuration:
                {"apiVersion":"redhatcop.redhat.io/v1alpha1","kind":"KeepalivedGroup","metadata":{"annotations":{},"name":"shz","namespace":"keepalived-op...
API Version:  redhatcop.redhat.io/v1alpha1
Kind:         KeepalivedGroup
Metadata:
  Creation Timestamp:  2020-09-15T07:44:27Z
  Generation:          8
  Resource Version:    91491390
  Self Link:           /apis/redhatcop.redhat.io/v1alpha1/namespaces/keepalived-operator/keepalivedgroups/shz
  UID:                 b04b7f57-1085-4fab-8831-97a151c8aa24
Spec:
  Blacklist Router I Ds:
    1
    2
    6
    223
  Image:      ***/openshift4/ose-keepalived-ipfailover
  Interface:  ens192
  Node Selector:
    failure-domain.beta.kubernetes.io/zone:  ***
Status:
  Conditions:
    Last Transition Time:  2020-09-28T10:41:55Z
    Message:               Awaiting next reconciliation
    Reason:                Successful
    Status:                True
    Type:                  ReconcileSuccess
  Router I Ds:
    kucera/kad:  2
Events:          <none>

[danielkucera]

can you explain why this was removed? https://github.com/redhat-cop/keepalived-operator/pull/33/commits/5eec0ada8c1b67edeb227fb99cfa7c4bd03c9d21#diff-74a1a8a53b344f8d4f297f314f302ed7L285-R301

[danielkucera]

Can you explain how on earth is this supposed to work?

    for key, value := range ids {
        if key < (value - 1) {
            return key, nil
        }
    }

[danielkucera]

👏 https://play.golang.org/p/YUvpPYO9yik

[raffaelespazzoli]

@danielkucera apologies again for the various misconprehensions. the code you contributed was slightly changed for readability reasons. this is what it looks like now:

func findNextAvailableID(ids []int) (int, error) {
    if len(ids) == 0 {
        return 1, nil
    }
    usedSet := iset.New(ids...)
    for i := 1; i <= 255; i++ {
        used := false
        if usedSet.Has(i) {
            used = true
        }
        if !used {
            return i, nil
        }
    }
    return 0, errors.New("cannot allocate more than 255 ids in one keepalived group")
}

let us know if you think there is a problem with it. This code was released yesterday. The release also contained the other feature you were asking for about allowing the daemonset to run on tainted nodes. Please keep us sending us your feedback and your contributions when you feel like.

[danielkucera]

@raffaelespazzoli , I was testing this with master right after the merge, you could have mentioned you were fixing this in following few commits. Latest master works fine. Thanks.