1
0
Fork 0
mirror of https://github.com/kubernetes-sigs/node-feature-discovery.git synced 2024-12-14 11:57:51 +00:00

nfd-master: use namespaced label and annotation names internally

For historical reasons the labels in the default nfd namespace have been
internally represented without the namespace part. I.e. instead of
"feature.node.kubernetes.io/foo" we just use "foo". NFD worker uses this
representation, too, both internally and over the gRPC requests. The
same scheme has been used for annotations.

This patch changes NFD master to use fully namespaced label and
annotation names internally. This hopefully makes the code a bit more
understandable. It also addresses some corner cases making the handling
of label names consistent, making it possible to use both "truncated"
and fully namespaced names over the gRPC interface (and in the
annotations).
This commit is contained in:
Markus Lehtonen 2020-08-12 21:28:21 +03:00
parent 7f3f9d7ed8
commit bb1e4c60fb
4 changed files with 123 additions and 101 deletions

View file

@ -122,7 +122,10 @@ func argsParse(argv []string) (master.Args, error) {
return args, fmt.Errorf("error parsing whitelist regex (%s): %s", arguments["--label-whitelist"], err)
}
args.VerifyNodeName = arguments["--verify-node-name"].(bool)
args.ExtraLabelNs = strings.Split(arguments["--extra-label-ns"].(string), ",")
args.ExtraLabelNs = map[string]struct{}{}
for _, n := range strings.Split(arguments["--extra-label-ns"].(string), ",") {
args.ExtraLabelNs[n] = struct{}{}
}
args.ResourceLabels = strings.Split(arguments["--resource-labels"].(string), ",")
args.Prune = arguments["--prune"].(bool)
args.Kubeconfig = arguments["--kubeconfig"].(string)

View file

@ -54,22 +54,22 @@ func newMockNode() *api.Node {
func TestUpdateNodeFeatures(t *testing.T) {
Convey("When I update the node using fake client", t, func() {
fakeFeatureLabels := map[string]string{"source-feature.1": "1", "source-feature.2": "2", "source-feature.3": "val3"}
fakeAnnotations := map[string]string{"version": version.Get()}
fakeExtResources := ExtendedResources{"source-feature.1": "", "source-feature.2": ""}
fakeFeatureLabels := map[string]string{LabelNs + "/source-feature.1": "1", LabelNs + "/source-feature.2": "2", LabelNs + "/source-feature.3": "val3"}
fakeAnnotations := map[string]string{AnnotationNs + "/version": version.Get()}
fakeExtResources := ExtendedResources{LabelNs + "/source-feature.1": "", LabelNs + "/source-feature.2": ""}
fakeFeatureLabelNames := make([]string, 0, len(fakeFeatureLabels))
for k := range fakeFeatureLabels {
fakeFeatureLabelNames = append(fakeFeatureLabelNames, k)
}
sort.Strings(fakeFeatureLabelNames)
fakeAnnotations["feature-labels"] = strings.Join(fakeFeatureLabelNames, ",")
fakeAnnotations[AnnotationNs+"/feature-labels"] = strings.Join(fakeFeatureLabelNames, ",")
mockAPIHelper := new(apihelper.MockAPIHelpers)
mockClient := &k8sclient.Clientset{}
// Mock node with old features
mockNode := newMockNode()
mockNode.Labels[LabelNs+"old-feature"] = "old-value"
mockNode.Annotations[AnnotationNs+"feature-labels"] = "old-feature"
mockNode.Labels[LabelNs+"/old-feature"] = "old-value"
mockNode.Annotations[AnnotationNs+"/feature-labels"] = "old-feature"
Convey("When I successfully update the node with feature labels", func() {
mockAPIHelper.On("GetClient").Return(mockClient, nil)
@ -84,11 +84,11 @@ func TestUpdateNodeFeatures(t *testing.T) {
Convey("Node object should have updated with labels and annotations", func() {
So(len(mockNode.Labels), ShouldEqual, len(fakeFeatureLabels))
for k, v := range fakeFeatureLabels {
So(mockNode.Labels[LabelNs+k], ShouldEqual, v)
So(mockNode.Labels[k], ShouldEqual, v)
}
So(len(mockNode.Annotations), ShouldEqual, len(fakeAnnotations))
for k, v := range fakeAnnotations {
So(mockNode.Annotations[AnnotationNs+k], ShouldEqual, v)
So(mockNode.Annotations[k], ShouldEqual, v)
}
})
})
@ -195,23 +195,23 @@ func TestAddingExtResources(t *testing.T) {
Convey("When there are matching labels", func() {
mockNode := newMockNode()
mockResourceLabels := ExtendedResources{"feature-1": "1", "feature-2": "2"}
mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1", LabelNs + "feature-2": "2"}
resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
So(len(resourceOps), ShouldBeGreaterThan, 0)
})
Convey("When the resource already exists", func() {
mockNode := newMockNode()
mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
mockResourceLabels := ExtendedResources{"feature-1": "1"}
mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1"}
resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
So(len(resourceOps), ShouldEqual, 0)
})
Convey("When the resource already exists but its capacity has changed", func() {
mockNode := newMockNode()
mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(2, resource.BinarySI)
mockResourceLabels := ExtendedResources{"feature-1": "1"}
mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(2, resource.BinarySI)
mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1"}
resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
So(len(resourceOps), ShouldBeGreaterThan, 0)
})
@ -222,28 +222,28 @@ func TestRemovingExtResources(t *testing.T) {
Convey("When removing extended resources", t, func() {
Convey("When none are removed", func() {
mockNode := newMockNode()
mockResourceLabels := ExtendedResources{"feature-1": "1", "feature-2": "2"}
mockNode.Annotations[AnnotationNs+"extended-resources"] = "feature-1,feature-2"
mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1", LabelNs + "/feature-2": "2"}
mockNode.Annotations[AnnotationNs+"/extended-resources"] = "feature-1,feature-2"
mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
So(len(resourceOps), ShouldEqual, 0)
})
Convey("When the related label is gone", func() {
mockNode := newMockNode()
mockResourceLabels := ExtendedResources{"feature-4": "", "feature-2": "2"}
mockNode.Annotations[AnnotationNs+"extended-resources"] = "feature-4,feature-2"
mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-4")] = *resource.NewQuantity(4, resource.BinarySI)
mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
mockResourceLabels := ExtendedResources{LabelNs + "/feature-4": "", LabelNs + "/feature-2": "2"}
mockNode.Annotations[AnnotationNs+"/extended-resources"] = "feature-4,feature-2"
mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-4")] = *resource.NewQuantity(4, resource.BinarySI)
mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
So(len(resourceOps), ShouldBeGreaterThan, 0)
})
Convey("When the extended resource is no longer wanted", func() {
mockNode := newMockNode()
mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
mockResourceLabels := ExtendedResources{"feature-2": "2"}
mockNode.Annotations[AnnotationNs+"extended-resources"] = "feature-1,feature-2"
mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
mockResourceLabels := ExtendedResources{LabelNs + "/feature-2": "2"}
mockNode.Annotations[AnnotationNs+"/extended-resources"] = "feature-1,feature-2"
resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
So(len(resourceOps), ShouldBeGreaterThan, 0)
})
@ -259,6 +259,7 @@ func TestSetLabels(t *testing.T) {
mockNode := newMockNode()
mockServer := labelerServer{args: Args{LabelWhiteList: regexp.MustCompile("")}, apiHelper: mockHelper}
mockCtx := context.Background()
// In the gRPC request the label names may omit the default ns
mockLabels := map[string]string{"feature-1": "val-1", "feature-2": "val-2", "feature-3": "val-3"}
mockReq := &labeler.SetLabelsRequest{NodeName: workerName, NfdVersion: workerVer, Labels: mockLabels}
@ -282,11 +283,11 @@ func TestSetLabels(t *testing.T) {
Convey("Node object should have updated with labels and annotations", func() {
So(len(mockNode.Labels), ShouldEqual, len(mockLabels))
for k, v := range mockLabels {
So(mockNode.Labels[LabelNs+k], ShouldEqual, v)
So(mockNode.Labels[LabelNs+"/"+k], ShouldEqual, v)
}
So(len(mockNode.Annotations), ShouldEqual, len(expectedAnnotations))
for k, v := range expectedAnnotations {
So(mockNode.Annotations[AnnotationNs+k], ShouldEqual, v)
So(mockNode.Annotations[AnnotationNs+"/"+k], ShouldEqual, v)
}
})
})
@ -302,19 +303,22 @@ func TestSetLabels(t *testing.T) {
})
Convey("Node object should only have whitelisted labels", func() {
So(len(mockNode.Labels), ShouldEqual, 1)
So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "feature-2": "val-2"})
So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "/feature-2": "val-2"})
a := map[string]string{AnnotationNs + "worker.version": workerVer, AnnotationNs + "feature-labels": "feature-2", AnnotationNs + "extended-resources": ""}
a := map[string]string{AnnotationNs + "/worker.version": workerVer,
AnnotationNs + "/feature-labels": "feature-2",
AnnotationNs + "/extended-resources": ""}
So(len(mockNode.Annotations), ShouldEqual, len(a))
So(mockNode.Annotations, ShouldResemble, a)
})
})
Convey("When --extra-label-ns is specified", func() {
mockServer.args.ExtraLabelNs = []string{"valid.ns"}
mockServer.args.ExtraLabelNs = map[string]struct{}{"valid.ns": struct{}{}}
mockHelper.On("GetClient").Return(mockClient, nil)
mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil)
mockHelper.On("UpdateNode", mockClient, mockNode).Return(nil)
// In the gRPC request the label names may omit the default ns
mockLabels := map[string]string{"feature-1": "val-1",
"valid.ns/feature-2": "val-2",
"invalid.ns/feature-3": "val-3"}
@ -325,9 +329,11 @@ func TestSetLabels(t *testing.T) {
})
Convey("Node object should only have allowed label namespaces", func() {
So(len(mockNode.Labels), ShouldEqual, 2)
So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "feature-1": "val-1", "valid.ns/feature-2": "val-2"})
So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "/feature-1": "val-1", "valid.ns/feature-2": "val-2"})
a := map[string]string{AnnotationNs + "worker.version": workerVer, AnnotationNs + "feature-labels": "feature-1,valid.ns/feature-2", AnnotationNs + "extended-resources": ""}
a := map[string]string{AnnotationNs + "/worker.version": workerVer,
AnnotationNs + "/feature-labels": "feature-1,valid.ns/feature-2",
AnnotationNs + "/extended-resources": ""}
So(len(mockNode.Annotations), ShouldEqual, len(a))
So(mockNode.Annotations, ShouldResemble, a)
})
@ -371,9 +377,9 @@ func TestAddLabels(t *testing.T) {
Convey("They should be added to the node.Labels", func() {
test1 := "test1"
labels[test1] = "true"
labels[LabelNs+"/"+test1] = "true"
addLabels(n, labels)
So(n.Labels, ShouldContainKey, LabelNs+test1)
So(n.Labels, ShouldContainKey, LabelNs+"/"+test1)
})
})
}

View file

@ -24,6 +24,7 @@ import (
"log"
"net"
"os"
"path/filepath"
"regexp"
"sort"
"strconv"
@ -42,10 +43,10 @@ import (
const (
// Namespace for feature labels
LabelNs = "feature.node.kubernetes.io/"
LabelNs = "feature.node.kubernetes.io"
// Namespace for all NFD-related annotations
AnnotationNs = "nfd.node.kubernetes.io/"
AnnotationNs = "nfd.node.kubernetes.io"
)
// package loggers
@ -68,7 +69,7 @@ type Annotations map[string]string
type Args struct {
CaFile string
CertFile string
ExtraLabelNs []string
ExtraLabelNs map[string]struct{}
KeyFile string
Kubeconfig string
LabelWhiteList *regexp.Regexp
@ -100,9 +101,6 @@ type statusOp struct {
}
func createStatusOp(verb string, resource string, path string, value string) statusOp {
if !strings.Contains(resource, "/") {
resource = LabelNs + resource
}
res := strings.ReplaceAll(resource, "/", "~1")
return statusOp{verb, "/status/" + path + "/" + res, value}
}
@ -260,7 +258,7 @@ func updateMasterNode(helper apihelper.APIHelpers) error {
}
// Advertise NFD version as an annotation
addAnnotations(node, Annotations{"master.version": version.Get()})
addAnnotations(node, Annotations{AnnotationNs + "/master.version": version.Get()})
err = helper.UpdateNode(cli, node)
if err != nil {
stderrLogger.Printf("can't update node: %s", err.Error())
@ -270,50 +268,52 @@ func updateMasterNode(helper apihelper.APIHelpers) error {
return nil
}
// Filter labels by namespace and name whitelist
func filterFeatureLabels(labels Labels, extraLabelNs []string, labelWhiteList *regexp.Regexp, extendedResourceNames []string) (Labels, ExtendedResources) {
for label := range labels {
split := strings.SplitN(label, "/", 2)
name := split[0]
// Filter labels by namespace and name whitelist, and, turn selected labels
// into extended resources. This function also handles proper namespacing of
// labels and ERs, i.e. adds the possibly missing default namespace for labels
// arriving through the gRPC API.
func filterFeatureLabels(labels Labels, extraLabelNs map[string]struct{}, labelWhiteList *regexp.Regexp, extendedResourceNames []string) (Labels, ExtendedResources) {
outLabels := Labels{}
// Check namespaced labels, filter out if ns is not whitelisted
if len(split) == 2 {
ns := split[0]
name = split[1]
for i, extraNs := range extraLabelNs {
if ns == extraNs {
break
} else if i == len(extraLabelNs)-1 {
for label, value := range labels {
// Add possibly missing default ns
label := addNs(label, LabelNs)
ns, name := splitNs(label)
// Check label namespace, filter out if ns is not whitelisted
if ns != LabelNs {
if _, ok := extraLabelNs[ns]; !ok {
stderrLogger.Printf("Namespace '%s' is not allowed. Ignoring label '%s'\n", ns, label)
delete(labels, label)
}
continue
}
}
// Skip if label doesn't match labelWhiteList
if !labelWhiteList.MatchString(name) {
stderrLogger.Printf("%s does not match the whitelist (%s) and will not be published.", name, labelWhiteList.String())
delete(labels, label)
stderrLogger.Printf("%s (%s) does not match the whitelist (%s) and will not be published.", name, label, labelWhiteList.String())
continue
}
outLabels[label] = value
}
// Remove labels which are intended to be extended resources
extendedResources := ExtendedResources{}
for _, extendedResourceName := range extendedResourceNames {
// remove possibly given default LabelNs to keep annotations shorter
extendedResourceName = strings.TrimPrefix(extendedResourceName, LabelNs)
if _, ok := labels[extendedResourceName]; ok {
if _, err := strconv.Atoi(labels[extendedResourceName]); err != nil {
stderrLogger.Printf("bad label value encountered for extended resource: %s", err.Error())
// Add possibly missing default ns
extendedResourceName = addNs(extendedResourceName, LabelNs)
if value, ok := outLabels[extendedResourceName]; ok {
if _, err := strconv.Atoi(value); err != nil {
stderrLogger.Printf("bad label value (%s: %s) encountered for extended resource: %s", extendedResourceName, value, err.Error())
continue // non-numeric label can't be used
}
extendedResources[extendedResourceName] = labels[extendedResourceName]
delete(labels, extendedResourceName)
extendedResources[extendedResourceName] = value
delete(outLabels, extendedResourceName)
}
}
return labels, extendedResources
return outLabels, extendedResources
}
// Implement LabelerServer
@ -355,19 +355,21 @@ func (s *labelerServer) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*p
// Advertise NFD worker version, label names and extended resources as annotations
labelKeys := make([]string, 0, len(labels))
for k := range labels {
labelKeys = append(labelKeys, k)
// Drop the ns part for labels in the default ns
labelKeys = append(labelKeys, strings.TrimPrefix(k, LabelNs+"/"))
}
sort.Strings(labelKeys)
extendedResourceKeys := make([]string, 0, len(extendedResources))
for key := range extendedResources {
extendedResourceKeys = append(extendedResourceKeys, key)
// Drop the ns part if in the default ns
extendedResourceKeys = append(extendedResourceKeys, strings.TrimPrefix(key, LabelNs+"/"))
}
sort.Strings(extendedResourceKeys)
annotations := Annotations{"worker.version": r.NfdVersion,
"feature-labels": strings.Join(labelKeys, ","),
"extended-resources": strings.Join(extendedResourceKeys, ","),
annotations := Annotations{AnnotationNs + "/worker.version": r.NfdVersion,
AnnotationNs + "/feature-labels": strings.Join(labelKeys, ","),
AnnotationNs + "/extended-resources": strings.Join(extendedResourceKeys, ","),
}
err := updateNodeFeatures(s.apiHelper, r.NodeName, labels, annotations, extendedResources)
@ -398,8 +400,12 @@ func updateNodeFeatures(helper apihelper.APIHelpers, nodeName string, labels Lab
statusOps := getExtendedResourceOps(node, extendedResources)
// Remove old labels
if l, ok := node.Annotations[AnnotationNs+"feature-labels"]; ok {
if l, ok := node.Annotations[AnnotationNs+"/feature-labels"]; ok {
oldLabels := strings.Split(l, ",")
for i, name := range oldLabels {
// Names in the annotation may omit the default ns
oldLabels[i] = addNs(name, LabelNs)
}
removeLabels(node, oldLabels)
}
@ -444,11 +450,7 @@ func removeLabelsWithPrefix(n *api.Node, search string) {
// Removes NFD labels from a Node object
func removeLabels(n *api.Node, labelNames []string) {
for _, l := range labelNames {
if strings.Contains(l, "/") {
delete(n.Labels, l)
} else {
delete(n.Labels, LabelNs+l)
}
}
}
@ -456,24 +458,30 @@ func removeLabels(n *api.Node, labelNames []string) {
func getExtendedResourceOps(n *api.Node, extendedResources ExtendedResources) []statusOp {
var statusOps []statusOp
oldResources := strings.Split(n.Annotations[AnnotationNs+"extended-resources"], ",")
// Form a list of namespaced resource names managed by us
if old, ok := n.Annotations[AnnotationNs+"/extended-resources"]; ok {
oldResources := strings.Split(old, ",")
for i, name := range oldResources {
// Names in the annotation may omit the default ns
oldResources[i] = addNs(name, LabelNs)
}
// figure out which resources to remove
for _, resource := range oldResources {
if _, ok := n.Status.Capacity[api.ResourceName(addNs(resource, LabelNs))]; ok {
if _, ok := n.Status.Capacity[api.ResourceName(resource)]; ok {
// check if the ext resource is still needed
_, extResNeeded := extendedResources[resource]
if !extResNeeded {
if _, extResNeeded := extendedResources[resource]; !extResNeeded {
statusOps = append(statusOps, createStatusOp("remove", resource, "capacity", ""))
statusOps = append(statusOps, createStatusOp("remove", resource, "allocatable", ""))
}
}
}
}
// figure out which resources to replace and which to add
for resource, value := range extendedResources {
// check if the extended resource already exists with the same capacity in the node
if quantity, ok := n.Status.Capacity[api.ResourceName(addNs(resource, LabelNs))]; ok {
if quantity, ok := n.Status.Capacity[api.ResourceName(resource)]; ok {
val, _ := quantity.AsInt64()
if strconv.FormatInt(val, 10) != value {
statusOps = append(statusOps, createStatusOp("replace", resource, "capacity", value))
@ -489,20 +497,16 @@ func getExtendedResourceOps(n *api.Node, extendedResources ExtendedResources) []
}
// Add NFD labels to a Node object.
func addLabels(n *api.Node, labels map[string]string) {
func addLabels(n *api.Node, labels Labels) {
for k, v := range labels {
if strings.Contains(k, "/") {
n.Labels[k] = v
} else {
n.Labels[LabelNs+k] = v
}
}
}
// Add Annotations to a Node object
func addAnnotations(n *api.Node, annotations map[string]string) {
func addAnnotations(n *api.Node, annotations Annotations) {
for k, v := range annotations {
n.Annotations[AnnotationNs+k] = v
n.Annotations[k] = v
}
}
@ -511,5 +515,14 @@ func addNs(src string, nsToAdd string) string {
if strings.Contains(src, "/") {
return src
}
return nsToAdd + src
return filepath.Join(nsToAdd, src)
}
// splitNs splits a name into its namespace and name parts
func splitNs(fullname string) (string, string) {
split := strings.SplitN(fullname, "/", 2)
if len(split) == 2 {
return split[0], split[1]
}
return "", fullname
}

View file

@ -458,9 +458,9 @@ var _ = framework.KubeDescribe("[NFD] Node Feature Discovery", func() {
ginkgo.It("it should decorate the node with the fake feature labels", func() {
fakeFeatureLabels := map[string]string{
master.LabelNs + "fake-fakefeature1": "true",
master.LabelNs + "fake-fakefeature2": "true",
master.LabelNs + "fake-fakefeature3": "true",
master.LabelNs + "/fake-fakefeature1": "true",
master.LabelNs + "/fake-fakefeature2": "true",
master.LabelNs + "/fake-fakefeature3": "true",
}
// Remove pre-existing stale annotations and labels