Skip to content

Commit

Permalink
fix: addressed PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
carlkyrillos committed Feb 26, 2025
1 parent 647384c commit 6f882ad
Show file tree
Hide file tree
Showing 5 changed files with 289 additions and 73 deletions.
48 changes: 36 additions & 12 deletions pkg/cluster/cluster_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,28 +289,52 @@ func IsDefaultAuthMethod(ctx context.Context, cli client.Client) (bool, error) {
return authenticationobj.Spec.Type == configv1.AuthenticationTypeIntegratedOAuth || authenticationobj.Spec.Type == "", nil
}

func GetNodeArchitectures(ctx context.Context, client client.Client) (map[string]struct{}, error) {
func GetReadyWorkerNodes(ctx context.Context, k8sclient client.Client) ([]corev1.Node, error) {
nodeList := &corev1.NodeList{}
if err := client.List(ctx, nodeList); err != nil {
return nil, fmt.Errorf("failed to list nodes: %w", err)

labelSelector := client.MatchingLabels{
labels.WorkerNode: "",
}

// Create a map to track unique architectures
nodeArchitectures := make(map[string]struct{})
if err := k8sclient.List(ctx, nodeList, labelSelector); err != nil {
return nil, fmt.Errorf("failed to list worker nodes: %w", err)
}

var readyWorkerNodes []corev1.Node
for _, node := range nodeList.Items {
if arch, exists := node.Labels[labels.NodeArch]; exists {
if node.Status.Conditions != nil {
// Only count nodes that are Ready
for _, condition := range node.Status.Conditions {
if condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue {
nodeArchitectures[arch] = struct{}{}
}
if node.Status.Conditions != nil {
// Only count nodes that are Ready
for _, condition := range node.Status.Conditions {
if condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue {
readyWorkerNodes = append(readyWorkerNodes, node)
}
}
}
}

if len(readyWorkerNodes) < 1 {
return nil, errors.New("no ready worker nodes found")
}

return readyWorkerNodes, nil
}

func GetNodeArchitectures(nodes []corev1.Node) ([]string, error) {
// Create a map to track unique architectures
nodeArchitecturesMap := make(map[string]struct{})

for _, node := range nodes {
if arch, exists := node.Labels[labels.NodeArch]; exists {
nodeArchitecturesMap[arch] = struct{}{}
}
}

// Convert map to a slice now that any duplicate architectures have been removed
nodeArchitectures := make([]string, 0)
for arch := range nodeArchitecturesMap {
nodeArchitectures = append(nodeArchitectures, arch)
}

if len(nodeArchitectures) < 1 {
return nil, errors.New("no valid architectures found")
}
Expand Down
143 changes: 110 additions & 33 deletions pkg/cluster/cluster_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,93 +16,78 @@ import (
)

func TestGetNodeArchitectures(t *testing.T) {
ctx := context.Background()

nodeTypeMeta := metav1.TypeMeta{
APIVersion: gvk.Node.GroupVersion().String(),
Kind: gvk.Node.GroupVersion().String(),
}
nodeStatusReady := corev1.NodeStatus{
Conditions: []corev1.NodeCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionTrue,
},
},
}

amdNode := &corev1.Node{
amdNode := corev1.Node{
TypeMeta: nodeTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Name: "amd-node",
Labels: map[string]string{
labels.NodeArch: "amd64",
},
},
Status: nodeStatusReady,
}
powerNode := &corev1.Node{
powerNode := corev1.Node{
TypeMeta: nodeTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Name: "power-node",
Labels: map[string]string{
labels.NodeArch: "ppc64le",
},
},
Status: nodeStatusReady,
}
notReadyNode := &corev1.Node{
unlabeledNode := corev1.Node{
TypeMeta: nodeTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Name: "not-ready-node",
Labels: map[string]string{
labels.NodeArch: "amd64",
},
Name: "power-node",
},
}

type args struct {
client client.Client
nodes []corev1.Node
}
tests := []struct {
name string
args args
want map[string]struct{}
want []string
wantErr bool
}{
{
name: "Single-node cluster",
name: "Single-arch nodes",
args: args{
client: fake.NewFakeClient(amdNode),
nodes: []corev1.Node{amdNode},
},
want: map[string]struct{}{
"amd64": {},
want: []string{
"amd64",
},
wantErr: false,
},
{
name: "Multi-node cluster",
name: "Multi-arch nodes",
args: args{
client: fake.NewFakeClient(amdNode, powerNode),
nodes: []corev1.Node{amdNode, powerNode},
},
want: map[string]struct{}{
"amd64": {},
"ppc64le": {},
want: []string{
"amd64",
"ppc64le",
},
wantErr: false,
},
{
name: "No ready nodes cluster",
name: "Unlabeled nodes",
args: args{
client: fake.NewFakeClient(notReadyNode),
nodes: []corev1.Node{unlabeledNode},
},
want: nil,
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := cluster.GetNodeArchitectures(ctx, tt.args.client)
got, err := cluster.GetNodeArchitectures(tt.args.nodes)
if (err != nil) != tt.wantErr {
t.Errorf("GetNodeArchitectures() error = %v, wantErr %v", err, tt.wantErr)
return
Expand All @@ -113,3 +98,95 @@ func TestGetNodeArchitectures(t *testing.T) {
})
}
}

func TestGetReadyWorkerNodes(t *testing.T) {
ctx := context.Background()

nodeTypeMeta := metav1.TypeMeta{
APIVersion: gvk.Node.GroupVersion().String(),
Kind: gvk.Node.GroupVersion().String(),
}

readyWorkerNode := corev1.Node{
TypeMeta: nodeTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Name: "ready-worker-node",
Labels: map[string]string{
labels.WorkerNode: "",
},
},
Status: corev1.NodeStatus{
Conditions: []corev1.NodeCondition{
{
Type: corev1.NodeReady,
Status: corev1.ConditionTrue,
},
},
},
}
notReadyWorkerNode := corev1.Node{
TypeMeta: nodeTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Name: "not-ready-worker-node",
Labels: map[string]string{
labels.WorkerNode: "",
},
},
}
masterNode := corev1.Node{
TypeMeta: nodeTypeMeta,
ObjectMeta: metav1.ObjectMeta{
Name: "master-node",
Labels: map[string]string{
"node-role.kubernetes.io/master": "",
},
},
}

type args struct {
k8sclient client.Client
}
tests := []struct {
name string
args args
want []corev1.Node
wantErr bool
}{
{
name: "Ready worker nodes",
args: args{
k8sclient: fake.NewFakeClient(&readyWorkerNode, &masterNode, &notReadyWorkerNode),
},
want: []corev1.Node{readyWorkerNode},
wantErr: false,
},
{
name: "No worker nodes",
args: args{
k8sclient: fake.NewFakeClient(&masterNode),
},
want: nil,
wantErr: true,
},
{
name: "No ready worker nodes",
args: args{
k8sclient: fake.NewFakeClient(&notReadyWorkerNode),
},
want: nil,
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := cluster.GetReadyWorkerNodes(ctx, tt.args.k8sclient)
if (err != nil) != tt.wantErr {
t.Errorf("GetReadyWorkerNodes() error = %v, wantErr %v", err, tt.wantErr)
return
}
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("GetReadyWorkerNodes() got = %v, want %v", got, tt.want)
}
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ import (
odhtypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types"
)

const (
DefaultArchitecture = "amd64"
)

// VerifySupportedArchitectures determines whether a component can be enabled based on the architecture of each node.
//
// This is accomplished by doing the following:
Expand All @@ -27,25 +31,29 @@ func VerifySupportedArchitectures(ctx context.Context, rr *odhtypes.Reconciliati
}

// Fetch the architecture(s) that the component supports
supportedArchitectures := make(map[string]struct{})
var supportedArchitectures []string
componentReleases := obj.GetReleaseStatus()
if componentReleases == nil || len(*componentReleases) < 1 {
return fmt.Errorf("instance %v has no releases", rr.Instance)
}
for _, release := range *componentReleases {
for _, arch := range release.SupportedArchitectures {
supportedArchitectures[arch] = struct{}{}
}
supportedArchitectures = append(supportedArchitectures, release.SupportedArchitectures...)
}

// TODO: Refactor after all components explicitly list supportedArchitectures in their component_metadata.yaml file
// If supportedArchitectures is empty, assume the component only works on amd64
if len(supportedArchitectures) == 0 {
supportedArchitectures["amd64"] = struct{}{}
supportedArchitectures = append(supportedArchitectures, DefaultArchitecture)
}

// Fetch the ready worker nodes
readyWorkerNodes, err := cluster.GetReadyWorkerNodes(ctx, rr.Client)
if err != nil {
return err
}

// Fetch the architecture(s) that the nodes are running on
nodeArchitectures, err := cluster.GetNodeArchitectures(ctx, rr.Client)
nodeArchitectures, err := cluster.GetNodeArchitectures(readyWorkerNodes)
if err != nil {
return err
}
Expand All @@ -71,10 +79,12 @@ func VerifySupportedArchitectures(ctx context.Context, rr *odhtypes.Reconciliati

// HasCompatibleArchitecture Returns true if there's at least one architecture that's in both supportedArches and nodeArches.
// Otherwise, it returns false.
func HasCompatibleArchitecture(supportedArches map[string]struct{}, nodeArches map[string]struct{}) bool {
for arch := range nodeArches {
if _, exists := supportedArches[arch]; exists {
return true
func HasCompatibleArchitecture(supportedArches []string, nodeArches []string) bool {
for _, nodeArch := range nodeArches {
for _, supportedArch := range supportedArches {
if nodeArch == supportedArch {
return true
}
}
}

Expand Down
Loading

0 comments on commit 6f882ad

Please sign in to comment.