-
Notifications
You must be signed in to change notification settings - Fork 112
feat: Add BM upgrader for self-managed bare metal clusters #699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,6 +93,7 @@ spec: | |
| enum: | ||
| - OSD | ||
| - ARO | ||
| - BM | ||
| type: string | ||
| upgradeAt: | ||
| description: Specify the upgrade start time | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,7 @@ spec: | |
| enum: | ||
| - OSD | ||
| - ARO | ||
| - BM | ||
| type: string | ||
| upgradeAt: | ||
| description: Specify the upgrade start time | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,7 @@ spec: | |
| enum: | ||
| - OSD | ||
| - ARO | ||
| - BM | ||
| type: string | ||
| upgradeAt: | ||
| description: Specify the upgrade start time | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |||||
| - [How to use it](#how-to-use-it) | ||||||
| - [For OSD](#for-osd) | ||||||
| - [For ARO](#for-aro) | ||||||
| - [For BM](#for-bm) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Fix the BM table-of-contents link. The heading at Line 35 generates Proposed fix- - [For BM](`#for-bm`)
+ - [For BM](`#for-bm-cluster`)📝 Committable suggestion
Suggested change
🧰 Tools🪛 markdownlint-cli2 (0.23.2)[warning] 9-9: Link fragments should be valid (MD051, link-fragments) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||||||
| - [Configurable knobs](#configurable-knobs) | ||||||
| - [configManager](#configmanager) | ||||||
| - [maintenance](#maintenance) | ||||||
|
|
@@ -31,6 +32,10 @@ Maintained in the [managed-cluster-config](https://github.com/openshift/managed- | |||||
| Maintained in the [ARO-RP](https://github.com/Azure/ARO-RP) repository: | ||||||
| - https://github.com/Azure/ARO-RP/blob/master/pkg/operator/controllers/muo/staticresources/config.yaml | ||||||
|
|
||||||
| ### For BM cluster | ||||||
|
|
||||||
| Maintained in your own individual cluster-config repository, but the BM upgrades are driven from a local `UpgradeConfig` on the cluster. Set `upgradeType: BM` and `configManager.source: LOCAL`. Extra worker scaling is not part of this upgrader, set `capacityReservation: false` on the `UpgradeConfig`. | ||||||
|
|
||||||
| ## Configurable knobs | ||||||
|
|
||||||
| #### upgradeType | ||||||
|
|
@@ -40,6 +45,7 @@ This defines which upgrader MUO should use to upgrade the cluster. | |||||
| Valid options are: | ||||||
| - [ARO](https://github.com/openshift/managed-upgrade-operator/blob/master/pkg/upgraders/aroupgrader.go) | ||||||
| - [OSD](https://github.com/openshift/managed-upgrade-operator/blob/master/pkg/upgraders/osdupgrader.go) | ||||||
| - [BM](https://github.com/openshift/managed-upgrade-operator/blob/master/pkg/upgraders/bmupgrader.go) | ||||||
|
|
||||||
| If this field is not present or is an empty value, the ARO upgrader is used by default. | ||||||
|
|
||||||
|
|
@@ -109,7 +115,7 @@ Example: | |||||
|
|
||||||
| #### upgradeWindow | ||||||
|
|
||||||
| The `upgradeWindow` section is used to control the `managed-upgrade-operator`'s behaviour in relation to the upgrade window within which an upgrade should take place. | ||||||
| The `upgradeWindow` section is used to control the `managed-upgrade-operator`'s behaviour in relation to the upgrade window within which an upgrade should take place. | ||||||
|
|
||||||
| | Key | Description | | ||||||
| | --- |-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -54,7 +54,7 @@ func (cfg *SpecProviderConfig) IsValid() error { | |||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| switch upgradev1alpha1.UpgradeType(cfg.UpgradeType) { | ||||||||||||||||
| case upgradev1alpha1.ARO, upgradev1alpha1.OSD, "": | ||||||||||||||||
| case upgradev1alpha1.ARO, upgradev1alpha1.OSD, upgradev1alpha1.BM, "": | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Require
Proposed fix switch upgradev1alpha1.UpgradeType(cfg.UpgradeType) {
-case upgradev1alpha1.ARO, upgradev1alpha1.OSD, upgradev1alpha1.BM, "":
+case upgradev1alpha1.BM:
+ if strings.ToUpper(cfg.ConfigManager.Source) != string(LOCAL) {
+ return ErrInvalidSpecProvider
+ }
+case upgradev1alpha1.ARO, upgradev1alpha1.OSD, "":📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| // An empty upgrade type is fine | ||||||||||||||||
| break | ||||||||||||||||
| default: | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| package upgraders | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
|
|
||
| upgradev1alpha1 "github.com/openshift/managed-upgrade-operator/api/v1alpha1" | ||
| ac "github.com/openshift/managed-upgrade-operator/pkg/availabilitychecks" | ||
| cv "github.com/openshift/managed-upgrade-operator/pkg/clusterversion" | ||
| "github.com/openshift/managed-upgrade-operator/pkg/configmanager" | ||
| "github.com/openshift/managed-upgrade-operator/pkg/drain" | ||
| "github.com/openshift/managed-upgrade-operator/pkg/eventmanager" | ||
| "github.com/openshift/managed-upgrade-operator/pkg/machinery" | ||
| "github.com/openshift/managed-upgrade-operator/pkg/maintenance" | ||
| "github.com/openshift/managed-upgrade-operator/pkg/metrics" | ||
| "github.com/openshift/managed-upgrade-operator/pkg/scaler" | ||
| "github.com/openshift/managed-upgrade-operator/pkg/upgradesteps" | ||
| ) | ||
|
|
||
| // bmUpgrader is a cluster upgrader suitable for self-managed bare metal clusters. | ||
| // It inherits from the base clusterUpgrader. | ||
| type bmUpgrader struct { | ||
| *clusterUpgrader | ||
| } | ||
|
|
||
| // NewBMUpgrader creates a new instance of a bmUpgrader | ||
| func NewBMUpgrader(c client.Client, cfm configmanager.ConfigManager, mc metrics.Metrics, notifier eventmanager.EventManager) (*bmUpgrader, error) { | ||
| cfg := &upgraderConfig{} | ||
| err := cfm.Into(cfg) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| m, err := maintenance.NewBuilder().NewClient(c) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| acs, err := ac.GetAvailabilityCheckers(&cfg.ExtDependencyAvailabilityCheck) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| bu := bmUpgrader{ | ||
| clusterUpgrader: &clusterUpgrader{ | ||
| client: c, | ||
| metrics: mc, | ||
| cvClient: cv.NewCVClient(c), | ||
| notifier: notifier, | ||
| config: cfg, | ||
| scaler: scaler.NewScaler(), | ||
| drainstrategyBuilder: drain.NewBuilder(), | ||
| maintenance: m, | ||
| machinery: machinery.NewMachinery(), | ||
| availabilityCheckers: acs, | ||
| }, | ||
| } | ||
|
|
||
| bu.steps = bmUpgradeSteps(&bu) | ||
|
|
||
| return &bu, nil | ||
| } | ||
|
|
||
| // bmUpgradeSteps returns the ordered upgrade steps for BM clusters. | ||
| // Capacity reservation (extra worker scaling) is intentionally omitted. | ||
| func bmUpgradeSteps(u *bmUpgrader) []upgradesteps.UpgradeStep { | ||
| return []upgradesteps.UpgradeStep{ | ||
| upgradesteps.Action(string(upgradev1alpha1.SendStartedNotification), u.SendStartedNotification), | ||
| upgradesteps.Action(string(upgradev1alpha1.IsClusterUpgradable), u.IsUpgradeable), | ||
| upgradesteps.Action(string(upgradev1alpha1.UpgradePreHealthCheck), u.PreUpgradeHealthCheck), | ||
| upgradesteps.Action(string(upgradev1alpha1.ExtDepAvailabilityCheck), u.ExternalDependencyAvailabilityCheck), | ||
| upgradesteps.Action(string(upgradev1alpha1.ControlPlaneMaintWindow), u.CreateControlPlaneMaintWindow), | ||
| upgradesteps.Action(string(upgradev1alpha1.CommenceUpgrade), u.CommenceUpgrade), | ||
| upgradesteps.Action(string(upgradev1alpha1.ControlPlaneUpgraded), u.ControlPlaneUpgraded), | ||
| upgradesteps.Action(string(upgradev1alpha1.RemoveControlPlaneMaintWindow), u.RemoveControlPlaneMaintWindow), | ||
| upgradesteps.Action(string(upgradev1alpha1.WorkersMaintWindow), u.CreateWorkerMaintWindow), | ||
| upgradesteps.Action(string(upgradev1alpha1.AllWorkerNodesUpgraded), u.AllWorkersUpgraded), | ||
| upgradesteps.Action(string(upgradev1alpha1.RemoveMaintWindow), u.RemoveMaintWindow), | ||
| upgradesteps.Action(string(upgradev1alpha1.PostClusterHealthCheck), u.PostUpgradeHealthCheck), | ||
| upgradesteps.Action(string(upgradev1alpha1.SendCompletedNotification), u.SendCompletedNotification), | ||
| } | ||
| } | ||
|
|
||
| // UpgradeCluster performs the upgrade of the cluster and returns an indication of the | ||
| // last-executed upgrade phase and any error associated with the phase execution. | ||
| func (u *bmUpgrader) UpgradeCluster(ctx context.Context, upgradeConfig *upgradev1alpha1.UpgradeConfig, logger logr.Logger) (upgradev1alpha1.UpgradePhase, error) { | ||
| u.upgradeConfig = upgradeConfig | ||
| return u.runSteps(ctx, logger, u.steps) | ||
| } | ||
|
|
||
| // HealthCheck performs a pre-upgrade healthcheck when an upgrade is scheduled in advance mainly | ||
| // to highlight and notify of issues which could get fixed before the upgrade begins. | ||
| func (u *bmUpgrader) HealthCheck(ctx context.Context, upgradeConfig *upgradev1alpha1.UpgradeConfig, logger logr.Logger) (bool, error) { | ||
| u.upgradeConfig = upgradeConfig | ||
| ok, err := u.PreUpgradeHealthCheck(ctx, logger) | ||
| return ok, err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject capacity reservation for
BM.Line 48 permits
type: BMwithcapacityReservation: true.pkg/upgraders/bmupgrader.goomits capacity-reservation scaling, so the operator accepts a request for extra capacity and then does not provide it. Reject this combination before execution, and regenerate the CRDs after adding the validation.As per coding guidelines, “Regenerate deepcopy, OpenAPI, and mocks after modifying API types or interfaces using
boilerplate/_lib/container-make generate.”🤖 Prompt for AI Agents
Source: Coding guidelines