Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion cmd/openshift-install/agent/waitfor.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
agentasset "github.com/openshift/installer/pkg/asset/agent"
"github.com/openshift/installer/pkg/asset/agent/workflow"
assetstore "github.com/openshift/installer/pkg/asset/store"
"github.com/openshift/installer/pkg/types"
)

// NewWaitForCmd create the commands for waiting the completion of the agent based cluster installation.
Expand Down Expand Up @@ -124,15 +125,20 @@ func newWaitForInstallCompleteCmd() *cobra.Command {

// Load install-config to check if FIPS verification is needed
var fipsEnabled bool

var expectedMasters, expectedWorkers int
if installConfigAsset, err := assetStore.Load(&agentasset.OptionalInstallConfig{}); err == nil && installConfigAsset != nil {
ic := installConfigAsset.(*agentasset.OptionalInstallConfig)
if ic.Config != nil {
fipsEnabled = ic.Config.FIPS
expectedMasters, expectedWorkers = extractExpectedNodeCounts(ic.Config)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not silently disable node verification on install-config load failures.

Line 130 ignores every Load error, leaving both counts at zero; verifyExpectedNodes then skips validation. Return or terminate on non-optional load errors instead of treating them as an absent config. As per path instructions, “Never ignore error returns.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/openshift-install/agent/waitfor.go` around lines 130 - 136, Handle errors
from assetStore.Load in the install-config loading flow before processing
installConfigAsset: propagate or terminate on non-optional load failures rather
than silently leaving expectedMasters and expectedWorkers at zero. Preserve the
existing optional-config behavior when no asset is available, and keep the FIPS
and extractExpectedNodeCounts handling for successfully loaded configs.

Source: Path instructions


options := command.WaitOptions{
VerifyFIPS: fipsEnabled,
VerifyFIPS: fipsEnabled,
ExpectedMasterNodes: expectedMasters,
ExpectedWorkerNodes: expectedWorkers,
}

if err = command.WaitForInstallComplete(ctx, cluster.API.Kube.Config, options); err != nil {
Expand All @@ -148,3 +154,18 @@ func newWaitForInstallCompleteCmd() *cobra.Command {
},
}
}

func extractExpectedNodeCounts(ic *types.InstallConfig) (masters int, workers int) {
if ic == nil {
return 0, 0
}
if ic.ControlPlane != nil && ic.ControlPlane.Replicas != nil {
masters = int(*ic.ControlPlane.Replicas)
}
for _, pool := range ic.Compute {
if pool.Replicas != nil {
workers += int(*pool.Replicas)
}
}
return masters, workers
}
145 changes: 145 additions & 0 deletions cmd/openshift-install/agent/waitfor_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
package agent

import (
"testing"

"github.com/openshift/installer/pkg/types"
)

func int64Ptr(i int64) *int64 {
return &i
}

func TestExtractExpectedNodeCounts(t *testing.T) {
tests := []struct {
name string
config *types.InstallConfig
expectedMasters int
expectedWorkers int
}{
{
name: "nil config",
config: nil,
expectedMasters: 0,
expectedWorkers: 0,
},
{
name: "3 masters and 2 workers",
config: &types.InstallConfig{
ControlPlane: &types.MachinePool{
Name: "master",
Replicas: int64Ptr(3),
},
Compute: []types.MachinePool{
{
Name: "worker",
Replicas: int64Ptr(2),
},
},
},
expectedMasters: 3,
expectedWorkers: 2,
},
{
name: "compact cluster with 3 masters and 0 workers",
config: &types.InstallConfig{
ControlPlane: &types.MachinePool{
Name: "master",
Replicas: int64Ptr(3),
},
Compute: []types.MachinePool{
{
Name: "worker",
Replicas: int64Ptr(0),
},
},
},
expectedMasters: 3,
expectedWorkers: 0,
},
{
name: "single node openshift",
config: &types.InstallConfig{
ControlPlane: &types.MachinePool{
Name: "master",
Replicas: int64Ptr(1),
},
Compute: []types.MachinePool{
{
Name: "worker",
Replicas: int64Ptr(0),
},
},
},
expectedMasters: 1,
expectedWorkers: 0,
},
{
name: "multiple compute pools",
config: &types.InstallConfig{
ControlPlane: &types.MachinePool{
Name: "master",
Replicas: int64Ptr(3),
},
Compute: []types.MachinePool{
{
Name: "worker",
Replicas: int64Ptr(2),
},
{
Name: "worker",
Replicas: int64Ptr(3),
},
},
},
expectedMasters: 3,
expectedWorkers: 5,
},
{
name: "nil replicas fields",
config: &types.InstallConfig{
ControlPlane: &types.MachinePool{
Name: "master",
},
Compute: []types.MachinePool{
{
Name: "worker",
},
},
},
expectedMasters: 0,
expectedWorkers: 0,
},
{
name: "nil control plane with workers",
config: &types.InstallConfig{
Compute: []types.MachinePool{
{
Name: "worker",
Replicas: int64Ptr(2),
},
},
},
expectedMasters: 0,
expectedWorkers: 2,
},
{
name: "empty config",
config: &types.InstallConfig{},
expectedMasters: 0,
expectedWorkers: 0,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
masters, workers := extractExpectedNodeCounts(tt.config)
if masters != tt.expectedMasters {
t.Errorf("expected %d masters but got %d", tt.expectedMasters, masters)
}
if workers != tt.expectedWorkers {
t.Errorf("expected %d workers but got %d", tt.expectedWorkers, workers)
}
})
}
}
77 changes: 77 additions & 0 deletions cmd/openshift-install/command/waitfor.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/pkg/errors"
"github.com/sirupsen/logrus"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
Expand Down Expand Up @@ -67,6 +68,10 @@ type WaitOptions struct {
UserProvisionedDNSEnabled bool
// VerifyFIPS verifies that FIPS mode is enabled on the cluster before completing.
VerifyFIPS bool
// ExpectedMasterNodes is the number of control plane nodes expected from install-config.
ExpectedMasterNodes int
// ExpectedWorkerNodes is the number of compute nodes expected from install-config.
ExpectedWorkerNodes int
}

// verifyFIPSEnabled checks that the cluster has FIPS enabled by querying
Expand Down Expand Up @@ -113,6 +118,74 @@ func verifyFIPSEnabled(ctx context.Context, config *rest.Config) error {
return nil
}

// verifyExpectedNodes checks that the expected number of master and worker nodes
// are present and in Ready state. Returns an error if verification fails.
func verifyExpectedNodes(ctx context.Context, config *rest.Config, expectedMasters, expectedWorkers int) error {
if expectedMasters == 0 && expectedWorkers == 0 {
return nil
}

client, err := kubernetes.NewForConfig(config)
if err != nil {
return errors.Wrap(err, "failed to create Kubernetes client for node verification")
}

return verifyExpectedNodesWithClient(ctx, client, expectedMasters, expectedWorkers)
}

func verifyExpectedNodesWithClient(ctx context.Context, client kubernetes.Interface, expectedMasters, expectedWorkers int) error {
if expectedMasters > 0 {
masterNodes, err := client.CoreV1().Nodes().List(ctx, metav1.ListOptions{
LabelSelector: "node-role.kubernetes.io/master",
})
if err != nil {
return errors.Wrap(err, "failed to list master nodes")
}

readyMasters := 0
for i := range masterNodes.Items {
for _, condition := range masterNodes.Items[i].Status.Conditions {
if condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue {
readyMasters++
break
}
}
}

if readyMasters < expectedMasters {
return fmt.Errorf("expected %d master node(s) to be Ready but only %d found", expectedMasters, readyMasters)
}
logrus.Debugf("Verified %d/%d master node(s) are Ready", readyMasters, expectedMasters)
}

if expectedWorkers > 0 {
workerNodes, err := client.CoreV1().Nodes().List(ctx, metav1.ListOptions{
LabelSelector: "node-role.kubernetes.io/worker",
})
if err != nil {
return errors.Wrap(err, "failed to list worker nodes")
}

readyWorkers := 0
for i := range workerNodes.Items {
for _, condition := range workerNodes.Items[i].Status.Conditions {
if condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue {
readyWorkers++
break
}
}
}

if readyWorkers < expectedWorkers {
return fmt.Errorf("expected %d worker node(s) to be Ready but only %d found", expectedWorkers, readyWorkers)
}
logrus.Debugf("Verified %d/%d worker node(s) are Ready", readyWorkers, expectedWorkers)
}

logrus.Info("Verified expected nodes are present and Ready")
return nil
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// WaitForInstallComplete waits for cluster to complete installation, checks for operator stability
// and logs cluster information when successful.
func WaitForInstallComplete(ctx context.Context, config *rest.Config, options WaitOptions) error {
Expand All @@ -134,6 +207,10 @@ func WaitForInstallComplete(ctx context.Context, config *rest.Config, options Wa
}
}

if err := verifyExpectedNodes(ctx, config, options.ExpectedMasterNodes, options.ExpectedWorkerNodes); err != nil {
return err
}

consoleURL, err := getConsole(ctx, config)
if err != nil {
logrus.Warnf("Cluster does not have a console available: %v", err)
Expand Down
Loading