MGMT-19421: phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount #1027

andrej1991 · 2025-02-06T15:16:29Z

phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount
in the first stage it still accepst --high-availability-mode as a command line argument, but it is being converted to ControlPlaneCount in the following maner:

ControlPlaneCount is being set to 3 if --high-availability-mode is set to full
ControlPlaneCount is being set ot 1 if --high-availability-mode is set to none
After the phase 1 of assisted-installer-agent is being merged --high-availability-mode will be fully removed from the valid parameters.

openshift-ci-robot · 2025-02-06T15:16:34Z

@andrej1991: This pull request references MGMT-19421 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.19.0" version, but no target version was set.

In response to this:

MGMT-19421: phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount
in the first stage it still accepst --high-availability-mode as a command line argument, but it is being converted to ControlPlaneCount in the following maner:

ControlPlaneCount is being set to 3 if --high-availability-mode is set to full

ControlPlaneCount is being set ot 1 if --high-availability-mode is set to none

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2025-02-06T15:17:05Z

@andrej1991: This pull request references MGMT-19421 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.19.0" version, but no target version was set.

In response to this:

MGMT-19421: phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount
in the first stage it still accepst --high-availability-mode as a command line argument, but it is being converted to ControlPlaneCount in the following maner:

ControlPlaneCount is being set to 3 if --high-availability-mode is set to full

ControlPlaneCount is being set ot 1 if --high-availability-mode is set to none

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

andrej1991 · 2025-02-06T15:19:34Z

@danmanor

codecov · 2025-02-06T15:38:13Z

Codecov Report

Attention: Patch coverage is 63.15789% with 7 lines in your changes missing coverage. Please review.

Project coverage is 55.63%. Comparing base (ab89730) to head (46b40a8).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/config/config.go	55.55%	4 Missing ⚠️
src/ops/ops.go	0.00%	2 Missing ⚠️
...ed-installer-controller/assisted_installer_main.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
- Coverage   55.67%   55.63%   -0.04%     
==========================================
  Files          15       15              
  Lines        3393     3399       +6     
==========================================
+ Hits         1889     1891       +2     
- Misses       1311     1315       +4     
  Partials      193      193

Files with missing lines	Coverage Δ
...taller_controller/assisted_installer_controller.go	`75.88% <100.00%> (ø)`
src/installer/installer.go	`68.58% <100.00%> (ø)`
...ed-installer-controller/assisted_installer_main.go	`27.54% <0.00%> (ø)`
src/ops/ops.go	`45.25% <0.00%> (ø)`
src/config/config.go	`67.56% <55.55%> (-3.03%)`	⬇️

openshift-ci-robot · 2025-02-06T16:10:53Z

@andrej1991: This pull request references MGMT-19421 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.19.0" version, but no target version was set.

In response to this:

phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount
in the first stage it still accepst --high-availability-mode as a command line argument, but it is being converted to ControlPlaneCount in the following maner:

ControlPlaneCount is being set to 3 if --high-availability-mode is set to full

ControlPlaneCount is being set ot 1 if --high-availability-mode is set to none

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

andrej1991 · 2025-02-06T19:17:00Z

/cc: @danmanor @AlonaKaplan

src/assisted_installer_controller/assisted_installer_controller.go

src/assisted_installer_controller/assisted_installer_controller_test.go

src/config/config.go

src/ops/ops.go

…ntrolPlaneCountit has to be further checked if changing the templates will not harm to the installation

andrej1991 · 2025-02-12T14:27:35Z

/cc: @pastequo @adriengentil

pastequo · 2025-02-12T14:36:15Z

/lgtm

andrej1991 · 2025-02-13T07:43:55Z

/retest

danmanor · 2025-02-13T14:13:19Z

deploy/assisted-installer-controller/assisted-installer-controller-cm.yaml.template

@@ -11,5 +11,5 @@ data:
  skip-cert-verification: '{{.SkipCertVerification}}'
  ca-cert-path: '{{.CACertPath}}'
  check-cluster-version: '{{.CheckCVO}}'
-  high-availability-mode: {{.HaMode}}
+  control-plane-count: '{{.ControllerCount}}'


Lets name this ControlPlaneCount like everywhere else

danmanor · 2025-02-13T14:16:00Z

src/config/config.go

@@ -73,7 +73,7 @@ func (c *Config) ProcessArgs(args []string) {
 	flagSet.StringVar(&c.HTTPSProxy, "https-proxy", "", "A proxy URL to use for creating HTTPS connections outside the cluster")
 	flagSet.StringVar(&c.NoProxy, "no-proxy", "", "A comma-separated list of destination domain names, domains, IP addresses, or other network CIDRs to exclude proxying")
 	flagSet.StringVar(&c.ServiceIPs, "service-ips", "", "All IPs of assisted service node")
-	flagSet.StringVar(&c.HighAvailabilityMode, "high-availability-mode", "", "high-availability expectations, \"Full\" which represents the behavior in a \"normal\" cluster. Use 'None' for single-node deployment. Leave this value as \"\" for workers as we do not care about HA mode for workers.")
+	flagSet.IntVar(&c.ControlPlaneCount, "control-plane-count", 0, "The number of controller nodes in the cluster")


Lets change to The number of control plane nodes in the cluster

danmanor · 2025-02-13T14:19:05Z

src/config/config.go

@@ -85,6 +85,7 @@ func (c *Config) ProcessArgs(args []string) {
 	var installerArgs string
 	flagSet.StringVar(&installerArgs, "installer-args", "", "JSON array of additional coreos-installer arguments")
 	h := flagSet.Bool("help", false, "Help message")
+	highAvailability := flagSet.String("high-availability-mode", "", "WARNING: this parameter will be shortly deprecated! valid values: full/none")


Lets keep it as is for now

flagSet.StringVar(&c.HighAvailabilityMode, "high-availability-mode", "", "high-availability expectations, \"Full\" which represents the behavior in a \"normal\" cluster. Use 'None' for single-node deployment. Leave this value as \"\" for workers as we do not care about HA mode for workers.")

danmanor · 2025-02-13T14:20:03Z

src/config/config.go

@@ -123,6 +124,15 @@ func (c *Config) ProcessArgs(args []string) {
 		utils.SetNoProxyEnv(c.NoProxy)
 	}

+	if highAvailability != nil && *highAvailability != "" {


I guess once you will use flagSet.StringVar you won't need the nil check ? also you will have "" default so yo will not need the if at all

danmanor · 2025-02-13T16:53:48Z

deploy/assisted-installer-controller/assisted-installer-controller-cm.yaml.template

@@ -11,5 +11,5 @@ data:
  skip-cert-verification: '{{.SkipCertVerification}}'
  ca-cert-path: '{{.CACertPath}}'
  check-cluster-version: '{{.CheckCVO}}'
-  high-availability-mode: {{.HaMode}}
+  control-plane-count: '{{.ControlPlaneCount}}'


What should ControlPlaneCount match ?

I guess it should match https://github.com/andrej1991/assisted-installer/blob/MGMT-19421-phase-1/src/ops/ops.go#L462

…with ControlPlaneCountin the first stage it still accepst --high-availability-mode as a command line argument, but it is being converted to ControlPlaneCount in the following maner:ControlPlaneCount is being set to 3 if --high-availability-mode is set to fullControlPlaneCount is being set ot 1 if --high-availability-mode is set to none

danmanor

Looks good !

danmanor · 2025-02-16T07:24:36Z

src/config/config.go

 	flagSet.StringVar(&installerArgs, "installer-args", "", "JSON array of additional coreos-installer arguments")
 	h := flagSet.Bool("help", false, "Help message")
+	flagSet.StringVar(&highAvailability, "high-availability-mode", "", "high-availability expectations, \"Full\" which represents the behavior in a \"normal\" cluster. Use 'None' for single-node deployment. Leave this value as \"\" for workers as we do not care about HA mode for workers.")


NIT - Probably best just keep

flagSet.StringVar(&c.HighAvailabilityMode, "high-availability-mode", "", "high-availability expectations, \"Full\" which represents the behavior in a \"normal\" cluster. Use 'None' for single-node deployment. Leave this value as \"\" for workers as we do not care about HA mode for workers.")

as it is, in effort to avoid creating unnecessary diff that can cause confusion along the way

danmanor · 2025-02-16T07:25:03Z

src/config/config.go

@@ -82,9 +82,10 @@ func (c *Config) ProcessArgs(args []string) {
 	flagSet.BoolVar(&c.NotifyNumReboots, "notify-num-reboots", false, "indicate number of reboots should be notified as event")
 	flagSet.StringVar(&c.CoreosImage, "coreos-image", "", "CoreOS image to install to the existing root")

-	var installerArgs string
+	var highAvailability, installerArgs string


See comment below, then this is redundant

danmanor · 2025-02-16T07:25:24Z

src/config/config.go

@@ -73,7 +73,7 @@ func (c *Config) ProcessArgs(args []string) {
 	flagSet.StringVar(&c.HTTPSProxy, "https-proxy", "", "A proxy URL to use for creating HTTPS connections outside the cluster")
 	flagSet.StringVar(&c.NoProxy, "no-proxy", "", "A comma-separated list of destination domain names, domains, IP addresses, or other network CIDRs to exclude proxying")
 	flagSet.StringVar(&c.ServiceIPs, "service-ips", "", "All IPs of assisted service node")
-	flagSet.StringVar(&c.HighAvailabilityMode, "high-availability-mode", "", "high-availability expectations, \"Full\" which represents the behavior in a \"normal\" cluster. Use 'None' for single-node deployment. Leave this value as \"\" for workers as we do not care about HA mode for workers.")


See comment below, we want to avoid this removal / addition diff

openshift-ci-robot · 2025-02-17T07:43:33Z

@andrej1991: This pull request references MGMT-19421 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.19.0" version, but no target version was set.

In response to this:

phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount
in the first stage it still accepst --high-availability-mode as a command line argument, but it is being converted to ControlPlaneCount in the following maner:

ControlPlaneCount is being set to 3 if --high-availability-mode is set to full

ControlPlaneCount is being set ot 1 if --high-availability-mode is set to none
After the phase 1 of assisted-installer-agent is being merged --high-availability-mode will be fully removed from the valid parameters.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-02-17T08:54:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrej1991, danmanor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [danmanor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2025-02-17T12:07:29Z

@andrej1991: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-bot · 2025-02-17T15:18:25Z

[ART PR BUILD NOTIFIER]

Distgit: ose-agent-installer-orchestrator
This PR has been included in build ose-agent-installer-orchestrator-container-v4.19.0-202502171407.p0.gd50758c.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-02-17T15:21:24Z

[ART PR BUILD NOTIFIER]

Distgit: ose-agent-installer-csr-approver
This PR has been included in build ose-agent-installer-csr-approver-container-v4.19.0-202502171407.p0.gd50758c.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 6, 2025

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 6, 2025

openshift-ci bot requested review from carbonin and romfreiman February 6, 2025 15:24

pastequo reviewed Feb 7, 2025

View reviewed changes

src/assisted_installer_controller/assisted_installer_controller.go Outdated Show resolved Hide resolved

pastequo reviewed Feb 7, 2025

View reviewed changes

src/assisted_installer_controller/assisted_installer_controller_test.go Outdated Show resolved Hide resolved

pastequo reviewed Feb 7, 2025

View reviewed changes

src/config/config.go Outdated Show resolved Hide resolved

pastequo reviewed Feb 7, 2025

View reviewed changes

src/ops/ops.go Show resolved Hide resolved

andrej1991 force-pushed the MGMT-19421-phase-1 branch from 43e807e to 500082a Compare February 12, 2025 14:09

andrej1991 added 2 commits February 12, 2025 15:24

test

9bf6d00

remove the reference of HighAvailabilityMode and replacing it with Co…

0d0e257

…ntrolPlaneCountit has to be further checked if changing the templates will not harm to the installation

andrej1991 force-pushed the MGMT-19421-phase-1 branch from 500082a to fe6ba50 Compare February 12, 2025 14:25

openshift-ci bot assigned pastequo Feb 12, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2025

danmanor requested changes Feb 13, 2025

View reviewed changes

openshift-ci bot assigned danmanor Feb 13, 2025

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 13, 2025

andrej1991 force-pushed the MGMT-19421-phase-1 branch from fe6ba50 to 3dc961f Compare February 13, 2025 15:54

danmanor reviewed Feb 13, 2025

View reviewed changes

andrej1991 force-pushed the MGMT-19421-phase-1 branch from 3dc961f to 46b40a8 Compare February 14, 2025 20:00

danmanor reviewed Feb 16, 2025

View reviewed changes

danmanor approved these changes Feb 17, 2025

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2025

andrej1991 mentioned this pull request Feb 17, 2025

MGMT-19420: Replace HighAvailabilityMode with ControlPlaneCount openshift/assisted-installer-agent#911

Open

openshift-merge-bot bot merged commit d50758c into openshift:master Feb 17, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-19421: phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount #1027

MGMT-19421: phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount #1027

andrej1991 commented Feb 6, 2025 •

edited

Loading

openshift-ci-robot commented Feb 6, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 6, 2025 •

edited by openshift-ci bot

Loading

andrej1991 commented Feb 6, 2025

codecov bot commented Feb 6, 2025 •

edited

Loading

openshift-ci-robot commented Feb 6, 2025 •

edited by openshift-ci bot

Loading

andrej1991 commented Feb 6, 2025

andrej1991 commented Feb 12, 2025

pastequo commented Feb 12, 2025

andrej1991 commented Feb 13, 2025

danmanor Feb 13, 2025

danmanor Feb 13, 2025

danmanor Feb 13, 2025

danmanor Feb 13, 2025

danmanor Feb 13, 2025

andrej1991 Feb 14, 2025

danmanor left a comment

danmanor Feb 16, 2025

danmanor Feb 16, 2025

danmanor Feb 16, 2025

openshift-ci-robot commented Feb 17, 2025 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Feb 17, 2025

openshift-ci bot commented Feb 17, 2025

openshift-bot commented Feb 17, 2025

openshift-bot commented Feb 17, 2025

MGMT-19421: phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount #1027

MGMT-19421: phase 1 of replacing the refference HighAvailabilityMode with ControlPlaneCount #1027

Conversation

andrej1991 commented Feb 6, 2025 • edited Loading

openshift-ci-robot commented Feb 6, 2025 • edited by openshift-ci bot Loading

openshift-ci-robot commented Feb 6, 2025 • edited by openshift-ci bot Loading

andrej1991 commented Feb 6, 2025

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

openshift-ci-robot commented Feb 6, 2025 • edited by openshift-ci bot Loading

andrej1991 commented Feb 6, 2025

andrej1991 commented Feb 12, 2025

pastequo commented Feb 12, 2025

andrej1991 commented Feb 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmanor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 17, 2025 • edited by openshift-ci bot Loading

openshift-ci bot commented Feb 17, 2025

openshift-ci bot commented Feb 17, 2025

openshift-bot commented Feb 17, 2025

openshift-bot commented Feb 17, 2025

andrej1991 commented Feb 6, 2025 •

edited

Loading

openshift-ci-robot commented Feb 6, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 6, 2025 •

edited by openshift-ci bot

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

openshift-ci-robot commented Feb 6, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 17, 2025 •

edited by openshift-ci bot

Loading