Skip to content

Commit

Permalink
CNF-13193: Node selector matching zero nodes is a valid use case
Browse files Browse the repository at this point in the history 
- It is allowed for a node selector to match zero nodes, this should not return an error
- Nodes can be added after the profile is created
- If no nodes are currently present it does limit the validation we can perform (No architecture validation without nodes present)
- Update profile validation and associated test cases with these changes
  • Loading branch information
@fontivan
fontivan committed Sep 9, 2024
1 parent 92e76c5 commit 3677c6f
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 63 deletions.
66 changes: 21 additions & 45 deletions pkg/apis/performanceprofile/v2/performanceprofile_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,15 +249,8 @@ func (r *PerformanceProfile) validateAllNodesAreSameCpuArchitecture(nodes corev1
var allErrs field.ErrorList
// First check if the node list has valid elements
if len(nodes.Items) == 0 {
allErrs = append(allErrs,
field.Invalid(
field.NewPath("spec.nodeSelector"),
r.Spec.NodeSelector,
"Failed to detect any nodes, unable to validate architecture",
),
)

// If we failed to detect any nodes we cannot do anything more here
// We are unable to validate this if we have no nodes
// But no nodes is still a valid profile so skip this validation
return allErrs
}

Expand Down Expand Up @@ -304,15 +297,8 @@ func (r *PerformanceProfile) validateAllNodesAreSameCpuCapacity(nodes corev1.Nod
var allErrs field.ErrorList
// First check if the node list has valid elements
if len(nodes.Items) == 0 {
allErrs = append(allErrs,
field.Invalid(
field.NewPath("spec.nodeSelector"),
r.Spec.NodeSelector,
"Failed to detect any nodes, unable to validate cpu capacity",
),
)

// If we failed to detect any nodes we cannot do anything more here
// We are unable to validate this if we have no nodes
// But no nodes is still a valid profile so skip this validation
return allErrs
}

Expand Down Expand Up @@ -362,26 +348,21 @@ func (r *PerformanceProfile) validateHugePages(nodes corev1.NodeList) field.Erro
return allErrs
}

// First check if the node list has valid elements
if len(nodes.Items) == 0 {
allErrs = append(allErrs,
field.Invalid(
field.NewPath("spec.nodeSelector"),
r.Spec.NodeSelector,
"Failed to detect any nodes, unable to validate hugepages",
),
)
// We can only partially validate this if we have no nodes
// We can check that the value used is legitimate but we cannot check
// whether it is supposed to be x86 or aarch64
x86 := false
aarch64 := false
combinedHugepagesSizes := append(x86ValidHugepagesSizes, aarch64ValidHugepagesSizes...)

// If we failed to detect any nodes we cannot do anything more here
return allErrs
if len(nodes.Items) > 0 {
// `validateHugePages` implicitly relies on `validateAllNodesAreSameCpuArchitecture` to have already been run
// Under that assumption we can return any node from the list since they should all be the same architecture
// However it is simple and easy to just return the first node
x86 = isX86(nodes.Items[0])
aarch64 = isAarch64(nodes.Items[0])
}

// `validateHugePages` implicitly relies on `validateAllNodesAreSameCpuArchitecture` to have already been run
// Under that assumption we can return any node from the list since they should all be the same architecture
// However it is simple and easy to just return the first node
x86 := isX86(nodes.Items[0])
aarch64 := isAarch64(nodes.Items[0])

if r.Spec.HugePages.DefaultHugePagesSize != nil {
defaultSize := *r.Spec.HugePages.DefaultHugePagesSize
errField := "spec.hugepages.defaultHugepagesSize"
Expand All @@ -405,13 +386,13 @@ func (r *PerformanceProfile) validateHugePages(nodes corev1.NodeList) field.Erro
fmt.Sprintf("%s %v", errMsg, aarch64ValidHugepagesSizes),
),
)
} else if !x86 && !aarch64 {
} else if !x86 && !aarch64 && !slices.Contains(combinedHugepagesSizes, string(defaultSize)){
allErrs = append(
allErrs,
field.Invalid(
field.NewPath(errField),
r.Spec.HugePages.DefaultHugePagesSize,
"Failed to detect architecture, unable to validate default hugepage size",
fmt.Sprintf("%s %v", errMsg, combinedHugepagesSizes),
),
)
}
Expand All @@ -438,13 +419,13 @@ func (r *PerformanceProfile) validateHugePages(nodes corev1.NodeList) field.Erro
fmt.Sprintf("%s %v", errMsg, aarch64ValidHugepagesSizes),
),
)
} else if !x86 && !aarch64 {
} else if !x86 && !aarch64 && !slices.Contains(combinedHugepagesSizes, string(page.Size)){
allErrs = append(
allErrs,
field.Invalid(
field.NewPath(errField),
r.Spec.HugePages.Pages,
"Failed to detect architecture, unable to validate hugepage size",
r.Spec.HugePages.DefaultHugePagesSize,
fmt.Sprintf("%s %v", errMsg, combinedHugepagesSizes),
),
)
}
Expand Down Expand Up @@ -618,10 +599,5 @@ func (r *PerformanceProfile) getNodesList() (corev1.NodeList, error) {
return corev1.NodeList{}, err
}

// If we have no nodes then consider this an error
if len(nodes.Items) == 0 {
return corev1.NodeList{}, fmt.Errorf("no nodes found with selector %s", selector)
}

return *nodes, nil
}
56 changes: 38 additions & 18 deletions pkg/apis/performanceprofile/v2/performanceprofile_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ var _ = Describe("PerformanceProfile", func() {
})

Describe("The getNodesList helper function", func() {
It("should not return any errors when at least one node is detected", func() {
It("should pass when at least one node is detected", func() {
// Get client with one node to test this case
nodeSpecs := []NodeSpecifications{}
nodeSpecs = append(nodeSpecs, NodeSpecifications{architecture: amd64, cpuCapacity: 1000, name: "node"})
Expand All @@ -383,15 +383,14 @@ var _ = Describe("PerformanceProfile", func() {
Expect(err).To(BeNil())
Expect(nodes.Items).ToNot(BeEmpty())
})
It("should return an error when nothing is detected", func() {
It("should pass when zero nodes is detected", func() {
// Get client with no nodes to test this case
nodeSpecs := []NodeSpecifications{}
validatorClient = GetFakeValidatorClient(nodeSpecs)

// There should be an empty node list and error present
nodes, err := profile.getNodesList()
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("no nodes found with selector"))
Expect(err).To(BeNil())
Expect(nodes.Items).To(BeEmpty())
})
})
Expand Down Expand Up @@ -436,18 +435,17 @@ var _ = Describe("PerformanceProfile", func() {
errors := profile.validateAllNodesAreSameCpuArchitecture(nodes)
Expect(errors).ToNot(BeEmpty())
})
It("should fail when no nodes are detected", func() {
It("should pass when no nodes are detected", func() {
// Get client with zero nodes
nodeSpecs := []NodeSpecifications{}
validatorClient = GetFakeValidatorClient(nodeSpecs)

nodes, err := profile.getNodesList()
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("no nodes found with selector"))
Expect(err).To(BeNil())
Expect(nodes.Items).To(BeEmpty())

errors := profile.validateAllNodesAreSameCpuArchitecture(nodes)
Expect(errors).ToNot(BeEmpty())
Expect(errors[0].Error()).To(ContainSubstring(("Failed to detect any nodes")))
Expect(errors).To(BeNil())
})
})

Expand Down Expand Up @@ -478,18 +476,17 @@ var _ = Describe("PerformanceProfile", func() {
errors := profile.validateAllNodesAreSameCpuCapacity(nodes)
Expect(errors).ToNot(BeEmpty())
})
It("should fail when no nodes are detected", func() {
It("should pass when no nodes are detected", func() {
// Get client with zero nodes
nodeSpecs := []NodeSpecifications{}
validatorClient = GetFakeValidatorClient(nodeSpecs)

nodes, err := profile.getNodesList()
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("no nodes found with selector"))
Expect(err).To(BeNil())
Expect(nodes.Items).To(BeEmpty())

errors := profile.validateAllNodesAreSameCpuCapacity(nodes)
Expect(errors).ToNot(BeEmpty())
Expect(errors[0].Error()).To(ContainSubstring(("Failed to detect any nodes")))
Expect(errors).To(BeNil())
})
})

Expand Down Expand Up @@ -565,20 +562,43 @@ var _ = Describe("PerformanceProfile", func() {
Expect(errors[0].Error()).To(ContainSubstring(fmt.Sprintf("the page size should be equal to one of %v", aarch64ValidHugepagesSizes)))
})

It("should fail when no nodes are detected", func() {
It("should pass when no nodes are detected with a valid hugepage size", func() {
// Get client with zero nodes
nodeSpecs := []NodeSpecifications{}
validatorClient = GetFakeValidatorClient(nodeSpecs)

nodes, err := profile.getNodesList()
Expect(err).To(BeNil())
Expect(nodes.Items).To(BeEmpty())

errors := profile.validateHugePages(nodes)
Expect(errors).To(BeNil())
})

It("should fail when no nodes are detected with a invalid hugepage size", func() {
// Get client with zero nodes
nodeSpecs := []NodeSpecifications{}
validatorClient = GetFakeValidatorClient(nodeSpecs)

defaultSize := HugePageSize(hugepagesSize2M)
profile.Spec.HugePages.DefaultHugePagesSize = &defaultSize

profile.Spec.HugePages.Pages = append(profile.Spec.HugePages.Pages, HugePage{
Count: 128,
Node: pointer.Int32(0),
Size: "14M",
})

nodes, err := profile.getNodesList()
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("no nodes found with selector"))
Expect(err).To(BeNil())
Expect(nodes.Items).To(BeEmpty())

errors := profile.validateHugePages(nodes)
Expect(errors).ToNot(BeEmpty())
Expect(errors[0].Error()).To(ContainSubstring(("Failed to detect any nodes")))
Expect(errors[0].Error()).To(ContainSubstring(("the page size should be equal to one of")))
})


When("pages have duplication", func() {
Context("with specified NUMA node", func() {
It("should raise the validation error (x86)", func() {
Expand Down

0 comments on commit 3677c6f

Please sign in to comment.