Skip to content

Fix node addition failure when the control plan has a custom name - #34

Merged
alicefr merged 2 commits into
mainfrom
fix-32
May 28, 2026
Merged

Fix node addition failure when the control plan has a custom name#34
alicefr merged 2 commits into
mainfrom
fix-32

Conversation

@alicefr

@alicefr alicefr commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Auto-detect the control-plane node name from container labels when --control-plane is not explicitly set

Fixes: #32

Summary by Sourcery

Auto-detect the control-plane node name when adding nodes so clusters with custom control-plane names work without explicitly specifying --control-plane.

Bug Fixes:

  • Fix failure to add worker nodes to clusters that use a custom control-plane node name when --control-plane is not provided.

Tests:

  • Add integration coverage for adding a worker node to a cluster that uses a custom control-plane node name.
  • Add a helper to create clusters with a custom control-plane node name in integration tests.

alicefr added 2 commits May 28, 2026 11:47
When a cluster is created with a custom --node-name, adding a new node
fails because --control-plane defaults to "node1". Discover the actual
control-plane node from container labels before creating the cluster
manager.

Fixes: #32

Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add integration test that creates a cluster with --node-name and adds
a worker node without specifying --control-plane, covering the scenario
from issue #32.

Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcery-ai

sourcery-ai Bot commented May 28, 2026

Copy link
Copy Markdown

Reviewer's Guide

Auto-detects the control-plane node name from existing containers during node addition and adds an integration test flow (with helpers) to verify adding a worker node when the control-plane uses a custom node name.

Sequence diagram for node addition with control-plane auto-detection

sequenceDiagram
    actor User
    participant CLIAddNode
    participant PodmanClient
    participant ClusterMgr

    User->>CLIAddNode: runAdd(nodeName, controlPlane, nodeImage, role)
    alt [controlPlane not set]
        CLIAddNode->>PodmanClient: findControlPlaneNode(ctx, podmanClient, clusterName, "")
        PodmanClient-->>CLIAddNode: discoveredControlPlaneName
        CLIAddNode->>CLIAddNode: controlPlane = discoveredControlPlaneName
    end

    CLIAddNode->>ClusterMgr: New(cluster.Config{ Name, ControlPlane, HostNetworkPopulator, Logger })
    CLIAddNode->>ClusterMgr: EnsureImagesVolume(ctx, nodeImage)
    ClusterMgr-->>CLIAddNode: clusterImagesVolume

    CLIAddNode-->>User: node added with images volume ensured
Loading

File-Level Changes

Change Details Files
Auto-detect control-plane node name before ensuring images volume in node-add flow.
  • Move images volume initialization to after control-plane discovery in the node add workflow
  • Call findControlPlaneNode with an empty control-plane name to discover the control-plane container from labels when --control-plane is not provided
  • Use the discovered control-plane name to initialize the cluster manager before ensuring the images volume
internal/cli/node/add.go
Add integration coverage for adding worker nodes to clusters with custom control-plane node names.
  • Add a new integration test that creates a cluster with a custom control-plane node name and then adds a worker node without specifying --control-plane
  • Verify the control-plane container is running with the custom name and both nodes become Ready in Kubernetes
  • Introduce a helper function to create a cluster with a custom control-plane node name using the CLI
test/integration/multinode_test.go
test/integration/helpers/cluster.go

Assessment against linked issues

Issue Objective Addressed Explanation
https://github.com/alicefr/bink/issues/32 Ensure that bink node add can successfully add a node to a cluster whose control-plane node was created with a custom node name, without requiring the --control-plane flag.
https://github.com/alicefr/bink/issues/32 Add automated test coverage to prevent regressions when adding nodes to clusters that use a custom control-plane node name.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • The new auto-detection logic runs unconditionally and will overwrite a user-specified --control-plane value; consider only calling findControlPlaneNode (or only assigning its result) when the controlPlane argument is empty to respect explicit CLI input.
  • If findControlPlaneNode returns an error, the code silently ignores it; adding at least a debug log would make it easier to diagnose cases where control-plane discovery fails and EnsureImagesVolume subsequently misbehaves.
  • The log message Step 0: Ensuring cluster images volume... now appears after Step 1: Creating ... node; consider renumbering or reordering these steps so the log output reflects the actual execution order.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new auto-detection logic runs unconditionally and will overwrite a user-specified `--control-plane` value; consider only calling `findControlPlaneNode` (or only assigning its result) when the `controlPlane` argument is empty to respect explicit CLI input.
- If `findControlPlaneNode` returns an error, the code silently ignores it; adding at least a debug log would make it easier to diagnose cases where control-plane discovery fails and `EnsureImagesVolume` subsequently misbehaves.
- The log message `Step 0: Ensuring cluster images volume...` now appears after `Step 1: Creating ... node`; consider renumbering or reordering these steps so the log output reflects the actual execution order.

## Individual Comments

### Comment 1
<location path="internal/cli/node/add.go" line_range="128-132" />
<code_context>
 		}
 	}

+	// Auto-detect the control-plane node name from container labels
+	discovered, err := findControlPlaneNode(ctx, podmanClient, clusterName, "")
+	if err == nil {
+		controlPlane = discovered
+	}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid silently overriding an explicitly provided controlPlane value.

This unconditionally overwrites any user-specified `controlPlane` when `findControlPlaneNode` returns a value, which can break explicit targeting. Only apply the auto-detected value when `controlPlane` is empty/unset, e.g. `if controlPlane == "" && err == nil { ... }`.

```suggestion
	// Auto-detect the control-plane node name from container labels
	if controlPlane == "" {
		discovered, err := findControlPlaneNode(ctx, podmanClient, clusterName, "")
		if err == nil {
			controlPlane = discovered
		}
	}
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread internal/cli/node/add.go
@alicefr
alicefr merged commit 830b982 into main May 28, 2026
6 checks passed
@alicefr
alicefr deleted the fix-32 branch June 18, 2026 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Adding a second node fails

1 participant