MGMT-17080: enable infrastructure operator #6037

rccrdpccl · 2024-02-28T15:43:06Z

When MCE and a storage operator (LVM or ODF) are selected, generate manifest for AgentServiceConfig

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2024-02-28T15:43:10Z

rccrdpccl · 2024-02-28T15:43:15Z

/hold
WIP

openshift-ci · 2024-02-28T15:44:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

rccrdpccl · 2024-02-28T18:37:50Z

internal/operators/mce/manifest.go

+	if err != nil {
+		return []byte{}, err
+	}
+	agentServiceConfig := &aiv1beta1.AgentServiceConfig{


Will add more default values here

internal/operators/mce/manifest.go

rccrdpccl · 2024-03-08T17:39:33Z

/test edge-subsystem-aws

internal/operators/lvm/config.go

internal/operators/manager.go

internal/bminventory/inventory_test.go

rccrdpccl · 2024-03-15T08:45:12Z

/retest

rccrdpccl · 2024-03-15T10:56:56Z

/unhold

rccrdpccl · 2024-03-15T15:49:00Z

/retest

docs/user-guide/additional-operator-notes.md

CrystalChun · 2024-03-15T16:47:05Z

internal/operators/common/common.go

@@ -0,0 +1,32 @@
+package common


Maybe a little too nit: but not sure if the name common is ok since the internal/common pkg exists already 😅 @avishayt @carbonin wdyt?

I don't think it's a technical problem, but it's generally good to avoid packages like "common" and "util" as they tend to get stuffed with unrelated things that defeat the purpose of packages.

So if it can be avoided then we should, but sometimes it's a necessary evil.

An alternative is for this to stay in the operators package in a util.go or something, but that's not much better than this for the same reasons I mentioned about util packages.

I totally agree that common is a crappy name, I've maintained it for consistency as it is used throughout the code.
Adding this to operators package would've caused circular dependency as operators package is requiring operators/odf and operators/odf would require this function(s).

As I discussed in another comment, I think the best approach to this would be using <package>utils pattern (see for example std lib ioutils). This way we can segregate in a package all related utilities and minimize circular dependency issues.
For example, all functions (not methods) related cluster object could go in internal/clusterutils. This would reduce dependencies to a minimum and can be used from the whole codebase without any issue.

internal/operators/lvm/lvm_operator.go

carbonin

Looking good, mostly minor comments

internal/common/common_test.go

carbonin · 2024-03-19T12:55:40Z

internal/operators/common/common.go

@@ -0,0 +1,32 @@
+package common


I don't think it's a technical problem, but it's generally good to avoid packages like "common" and "util" as they tend to get stuffed with unrelated things that defeat the purpose of packages.

So if it can be avoided then we should, but sometimes it's a necessary evil.

An alternative is for this to stay in the operators package in a util.go or something, but that's not much better than this for the same reasons I mentioned about util packages.

carbonin · 2024-03-19T12:59:38Z

internal/operators/common/common.go

+)
+
+// Return eligible disks count (disks that are both available and fulfill minSize requirements) and available disks (disks that are available but do not fulfill the size requirement)
+func GetValidDiskCount(disks []*models.Disk, installationDiskID string, minSize int64) (int64, int64) {


Valid for what? I can see the function is ensuring that it's not the install disk, it's of a particular type and atleast the passed size, but what does that make the disk valid for?

Based on the PR I'd assume it's use for a storage operator, but the function could use a better name/comment.

internal/operators/common/common_test.go

carbonin · 2024-03-19T14:26:56Z

internal/operators/lvm/lvm_operator.go

@@ -21,6 +22,8 @@ type operator struct {
 	extracter oc.Extracter
 }

+const defaultStorageClassName = "lvms-vg1"


Is this not something we can set when deploying the operators?

I don't know much about them but it seems like that should be a feature 🤷

internal/operators/manager_test.go

internal/operators/mce/manifest.go

internal/operators/mce/mce_operator_test.go

internal/operators/odf/validations.go

rccrdpccl · 2024-03-19T18:05:43Z

/retest

rccrdpccl · 2024-03-19T21:19:57Z

/retest

internal/operators/lvm/lvm_operator.go

internal/operators/manager.go

internal/operators/mce/config.go

internal/operators/mce/manifest.go

…tors are selected Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>

rccrdpccl · 2024-03-21T09:40:11Z

/retest

rccrdpccl · 2024-03-21T14:02:29Z

/retest

openshift-ci · 2024-03-21T14:09:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, rccrdpccl

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~~ [carbonin,rccrdpccl]

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

openshift-ci · 2024-03-21T16:30:13Z

@rccrdpccl: 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/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2024-03-21T19:29:49Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-agent-installer-api-server-container-v4.16.0-202403211443.p0.g41ad865.assembly.stream.el8 for distgit ose-agent-installer-api-server.
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 28, 2024

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 28, 2024

openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 28, 2024

rccrdpccl force-pushed the enable-infrastructure-operator-when-mce-and-storage branch from 23a1313 to 72ef83a Compare February 28, 2024 18:36

openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 28, 2024

rccrdpccl commented Feb 28, 2024

View reviewed changes

rccrdpccl commented Feb 29, 2024

View reviewed changes

internal/operators/mce/manifest.go Outdated Show resolved Hide resolved

rccrdpccl commented Feb 29, 2024

View reviewed changes

internal/operators/mce/manifest.go Outdated Show resolved Hide resolved

rccrdpccl commented Feb 29, 2024

View reviewed changes

internal/operators/mce/manifest.go Outdated Show resolved Hide resolved

rccrdpccl force-pushed the enable-infrastructure-operator-when-mce-and-storage branch from 72ef83a to 3b9adf5 Compare March 4, 2024 15:09

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 4, 2024

rccrdpccl force-pushed the enable-infrastructure-operator-when-mce-and-storage branch from 3b9adf5 to 126badd Compare March 8, 2024 17:37

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 8, 2024

rccrdpccl commented Mar 11, 2024

View reviewed changes

internal/operators/lvm/config.go Show resolved Hide resolved

rccrdpccl commented Mar 11, 2024

View reviewed changes

internal/operators/lvm/config.go Show resolved Hide resolved

rccrdpccl commented Mar 11, 2024

View reviewed changes

internal/operators/manager.go Show resolved Hide resolved

rccrdpccl force-pushed the enable-infrastructure-operator-when-mce-and-storage branch from 126badd to e400b25 Compare March 11, 2024 12:29

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 11, 2024

rccrdpccl marked this pull request as ready for review March 11, 2024 12:39

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2024

rccrdpccl force-pushed the enable-infrastructure-operator-when-mce-and-storage branch from 9f84d85 to 290e4f1 Compare March 14, 2024 17:45

CrystalChun reviewed Mar 14, 2024

View reviewed changes

internal/bminventory/inventory_test.go Show resolved Hide resolved

rccrdpccl force-pushed the enable-infrastructure-operator-when-mce-and-storage branch from 290e4f1 to 06fc7f8 Compare March 15, 2024 10:56

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 15, 2024

CrystalChun reviewed Mar 15, 2024

View reviewed changes

docs/user-guide/additional-operator-notes.md Show resolved Hide resolved

CrystalChun reviewed Mar 15, 2024

View reviewed changes

docs/user-guide/additional-operator-notes.md Show resolved Hide resolved

CrystalChun reviewed Mar 15, 2024

View reviewed changes

internal/operators/lvm/lvm_operator.go Outdated Show resolved Hide resolved

carbonin reviewed Mar 19, 2024

View reviewed changes

rccrdpccl force-pushed the enable-infrastructure-operator-when-mce-and-storage branch from 06fc7f8 to 127cc9c Compare March 19, 2024 17:13

carbonin reviewed Mar 20, 2024

View reviewed changes

internal/operators/lvm/lvm_operator.go Outdated Show resolved Hide resolved

carbonin reviewed Mar 20, 2024

View reviewed changes

internal/operators/manager.go Outdated Show resolved Hide resolved

rccrdpccl commented Mar 20, 2024

View reviewed changes

internal/operators/mce/config.go Outdated Show resolved Hide resolved

carbonin reviewed Mar 20, 2024

View reviewed changes

internal/operators/mce/manifest.go Outdated Show resolved Hide resolved

MGMT-17080: enable infrastructure operator when MCE and storage opera…

6f638f8

…tors are selected Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>

rccrdpccl force-pushed the enable-infrastructure-operator-when-mce-and-storage branch from 127cc9c to 6f638f8 Compare March 20, 2024 16:03

carbonin approved these changes Mar 21, 2024

View reviewed changes

openshift-ci bot assigned carbonin Mar 21, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2024

openshift-merge-bot bot merged commit 41ad865 into openshift:master Mar 21, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-17080: enable infrastructure operator #6037

MGMT-17080: enable infrastructure operator #6037

rccrdpccl commented Feb 28, 2024

openshift-ci-robot commented Feb 28, 2024 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

rccrdpccl commented Feb 28, 2024

openshift-ci bot commented Feb 28, 2024

rccrdpccl Feb 28, 2024

rccrdpccl commented Mar 8, 2024

rccrdpccl commented Mar 15, 2024

rccrdpccl commented Mar 15, 2024

rccrdpccl commented Mar 15, 2024

CrystalChun Mar 15, 2024

carbonin Mar 19, 2024

rccrdpccl Mar 19, 2024

carbonin left a comment

carbonin Mar 19, 2024

carbonin Mar 19, 2024

carbonin Mar 19, 2024

rccrdpccl commented Mar 19, 2024

rccrdpccl commented Mar 19, 2024

rccrdpccl commented Mar 21, 2024

rccrdpccl commented Mar 21, 2024

openshift-ci bot commented Mar 21, 2024

openshift-ci bot commented Mar 21, 2024

openshift-bot commented Mar 21, 2024

MGMT-17080: enable infrastructure operator #6037

MGMT-17080: enable infrastructure operator #6037

Conversation

rccrdpccl commented Feb 28, 2024

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Feb 28, 2024 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

rccrdpccl commented Feb 28, 2024

openshift-ci bot commented Feb 28, 2024

rccrdpccl Feb 28, 2024

Choose a reason for hiding this comment

rccrdpccl commented Mar 8, 2024

rccrdpccl commented Mar 15, 2024

rccrdpccl commented Mar 15, 2024

rccrdpccl commented Mar 15, 2024

CrystalChun Mar 15, 2024

Choose a reason for hiding this comment

carbonin Mar 19, 2024

Choose a reason for hiding this comment

rccrdpccl Mar 19, 2024

Choose a reason for hiding this comment

carbonin left a comment

Choose a reason for hiding this comment

carbonin Mar 19, 2024

Choose a reason for hiding this comment

carbonin Mar 19, 2024

Choose a reason for hiding this comment

carbonin Mar 19, 2024

Choose a reason for hiding this comment

rccrdpccl commented Mar 19, 2024

rccrdpccl commented Mar 19, 2024

rccrdpccl commented Mar 21, 2024

rccrdpccl commented Mar 21, 2024

openshift-ci bot commented Mar 21, 2024

openshift-ci bot commented Mar 21, 2024

openshift-bot commented Mar 21, 2024

openshift-ci-robot commented Feb 28, 2024 •

edited by openshift-ci bot