-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implemented router id blacklist #33
Conversation
@danielkucera thanks for this contribution. we have other people also asking for this. We will test it as soon as possible. |
Please regenerate/add the CRD (deploy/crds/redhatcop.redhat.io_keepalivedgroups_crd.yaml) using Additionally, when 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 |
8167531
to
45024a2
Compare
@David-Igou changes applied, please review. |
@danielkucera acknowledged, trying to find the time |
This reverts commit 142a9f8.
I'm done with this PR, if it is still not okay for you feel free to close or modify as you wish. |
Signed-off-by: Raffaele Spazzoli <[email protected]>
@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. |
Signed-off-by: Raffaele Spazzoli <[email protected]>
*@danielkucera done.*
…On Wed, Sep 23, 2020 at 11:22 AM Daniel Kucera ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/controller/keepalivedgroup/keepalivedgroup_controller.go
<#33 (comment)>
:
> @@ -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
<#33 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABPERXCVQTMHQ365QXLLAZ3SHIHDZANCNFSM4RLJ3UCA>
.
--
ciao/bye
Raffaele
|
@raffaelespazzoli LGTM |
I'm sorry to inform you that the code doesn't work. |
@danielkucera can you describe what you are seeing? |
@David-Igou that it doesn't work for example, note the router IDs and blacklist IDs:
|
can you explain why this was removed? |
Can you explain how on earth is this supposed to work?
|
@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. |
@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. |
fixes #32 and #26