#259 Support for node labels to set eniConfig #260
Conversation
|
Tested this PR with my EKS and it is awesome. probably for a while we going to use custom build for aws-node to ensure that we able to do automated cni configuration. |
|
|
||
| val, ok := label[eniConfigLabelDef] | ||
| if ok { | ||
| h.controller.eniLock.Lock() |
There was a problem hiding this comment.
There are three duplicated Lock, Unlock lines around this block. Would you like to refactor them?
For example, you can write under L106 as:
case *corev1.Node:
log.Infof("Handle corev1.Node: %s, %v, %v", o.GetName(), o.GetAnnotations(), o.GetLabels())
if h.controller.myNodeName == o.GetName() {
val, ok := o.GetAnnotations()[eniConfigAnnotationDef]
if !ok {
val, ok = o.GetLabels()[eniConfigLabelDef]
if !ok {
val = eniConfigDefault
}
}
h.controller.eniLock.Lock()
defer h.controller.eniLock.Unlock()
h.controller.myENI = val
log.Infof(" Setting myENI to: %s", val)
}
}
return nil
There was a problem hiding this comment.
I agree with @nak3, the code above is easier to read and less duplicated code.
| @@ -68,6 +68,33 @@ func updateNodeAnnotation(hdlr sdk.Handler, nodeName string, configName string, | |||
| hdlr.Handle(nil, event) | |||
| } | |||
|
|
|||
| func updateNodeLabel(hdlr sdk.Handler, nodeName string, configName string, toDelete bool) { | |||
There was a problem hiding this comment.
This function is almost same with updateNodeAnnotation(). It is better to combine them?
There was a problem hiding this comment.
Can we extract the strictly duplicate code. I think two methods is fine, if we remove the code that is the same toa createEvent function.
| @@ -144,3 +171,44 @@ func TestNodeENIConfig(t *testing.T) { | |||
| assert.Equal(t, defaultCfg, *outputCfg) | |||
|
|
|||
| } | |||
|
|
|||
| func TestNodeENIConfigLabel(t *testing.T) { | |||
There was a problem hiding this comment.
Same with above comment. Is it not possible to merge this test into TestNodeENIConfig()?
There was a problem hiding this comment.
I'm planning a bigger refactor of the test code so I am OK with this.
|
I am not an official reviewer so please disregard my comments if you disagree. |
|
when this can be merged? |
|
There is another PR with same functionality, more features and cleaner logic here 280. Hoping to see that one gets merged and you will also be able to set custom annotations and labels. |
|
Can we cancel this PR since #280 has been merged? |
#259
Added support for node labels and unit tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.