fix: no run configure OS in exist node#3068
fix: no run configure OS in exist node#3068yongchuanzhou merged 1 commit intokubesphere:release-v3.1.0-rc.0from
Conversation
Signed-off-by: redscholar <blacktiledhouse@gmail.com>
kubesphere-prow
Bot
added
release-note
kind/bug
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: redscholar The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubesphere-prow
Bot
added
the
size/S
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to skip OS configuration tasks for nodes already present in the cluster by implementing a NodeInCluster check. To support this, the StatusModule was moved before the ConfigureOSModule in several pipelines, and the pipeline cache is now initialized with a default Kubernetes status. However, review feedback points out that this reordering is missing in several critical paths, including the main cluster creation branch and the K3s/K8e node addition pipelines, which will cause the skip logic to fail in those scenarios.
| &kubernetes.StatusModule{}, | ||
| &os.ConfigureOSModule{Skip: runtime.Cluster.System.SkipConfigureOS}, |
There was a problem hiding this comment.
The StatusModule is correctly moved before ConfigureOSModule here to ensure the cluster status is available for the NodeInCluster check. However, this change is missing in the main cluster creation branch (the else block starting at line 83), where ConfigureOSModule still precedes StatusModule. This inconsistency will cause the OS configuration to run on all nodes in standard installations, defeating the purpose of the PR.
| &artifact.UnArchiveModule{Skip: noArtifact}, | ||
| &os.RepositoryModule{Skip: noArtifact || !runtime.Arg.InstallPackages}, | ||
| &binaries.NodeBinariesModule{}, | ||
| &kubernetes.StatusModule{}, |
There was a problem hiding this comment.
The StatusModule is added here to support the NodeInCluster check. Note that NewK3sAddNodesPipeline and NewK8eAddNodesPipeline in this file (lines 93 and 130) also use ConfigureOSModule but have not been updated to place their respective status modules before it. This will prevent the skip logic from working for K3s and K8e clusters.
yongchuanzhou
merged commit 2e75970
into
kubesphere:release-v3.1.0-rc.0
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for reviewers:
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: