Osc operator #6990

xiangchunfu · 2024-11-13T08:38:27Z

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?

eifrach · 2024-11-13T09:00:36Z

internal/operators/osc/manifest.go

+  sourceNamespace: openshift-marketplace
+  name: {{.OPERATOR_SOURCE_NAME}}
+  installPlanApproval: Automatic
+  startingCSV: startingCSV: sandboxed-containers-operator.v1.7.0


there is a typo here - but I perfer if we don't have static versions - this can break it later releases
so we can remove the statingCSV

Suggested change

startingCSV: startingCSV: sandboxed-containers-operator.v1.7.0

startingCSV: sandboxed-containers-operator.v1.7.0

eifrach · 2024-11-13T09:04:20Z

internal/operators/osc/operator.go

+}
+
+func (o *operator) GetDependencies(cluster *common.Cluster) ([]string, error) {
+	return []string{cnv.Operator.Name}, nil


this will enforce CNV installation when selecting osc

codecov · 2024-11-13T09:37:57Z

Codecov Report

Attention: Patch coverage is 74.69880% with 42 lines in your changes missing coverage. Please review.

Project coverage is 68.24%. Comparing base (d8cdc34) to head (fa68f19).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/operators/osc/operator.go	69.89%	22 Missing and 6 partials ⚠️
internal/operators/osc/manifest.go	74.35%	5 Missing and 5 partials ⚠️
internal/featuresupport/features_olm_operators.go	85.71%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6990      +/-   ##
==========================================
+ Coverage   68.21%   68.24%   +0.02%     
==========================================
  Files         273      275       +2     
  Lines       39079    39244     +165     
==========================================
+ Hits        26659    26781     +122     
- Misses      10008    10038      +30     
- Partials     2412     2425      +13

Files with missing lines	Coverage Δ
internal/cluster/statemachine.go	`99.65% <100.00%> (+<0.01%)`	⬆️
internal/cluster/validation_id.go	`92.30% <ø> (ø)`
internal/featuresupport/feature_support_level.go	`96.49% <ø> (ø)`
internal/featuresupport/features_platforms.go	`92.81% <100.00%> (+0.63%)`	⬆️
internal/host/statemachine.go	`100.00% <100.00%> (ø)`
internal/host/validation_id.go	`90.90% <100.00%> (ø)`
internal/operators/builder.go	`100.00% <100.00%> (ø)`
internal/featuresupport/features_olm_operators.go	`81.30% <85.71%> (+0.42%)`	⬆️
internal/operators/osc/manifest.go	`74.35% <74.35%> (ø)`
internal/operators/osc/operator.go	`69.89% <69.89%> (ø)`

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

ariel-adam · 2024-11-20T10:22:25Z

internal/operators/osc/operator.go

+}
+
+func (o *operator) GetDependencies(cluster *common.Cluster) ([]string, error) {
+	return []string{Operator.Name}, nil


Should be

return make([]string, 0), nil

eifrach · 2024-11-13T10:03:36Z

internal/featuresupport/features_olm_operators.go

+		return models.SupportLevelUnavailable
+	}
+
+	return models.SupportLevelSupported


should be tech preview

Hello @eifrach , we plan to support OSC operator in this PR. The OSC operator has been GA from OCP4.10. So the "Supported" is expected here. Thanks.

eifrach · 2024-11-20T11:10:24Z

internal/featuresupport/features_olm_operators.go

+func (feature *OSCFeature) getId() models.FeatureSupportLevelID {
+	return models.FeatureSupportLevelIDOSC
+}


this function is not in interface
https://github.com/openshift/assisted-service/blob/master/internal/operators/api/api.go#L31

eifrach · 2024-11-20T11:11:08Z

internal/featuresupport/features_olm_operators.go

+func (feature *OSCFeature) getId() models.FeatureSupportLevelID {
+	return models.FeatureSupportLevelIDOSC
+}
+


I'm missing two functions

func (o *operator) GetClusterValidationID() string func (o *operator) GetHostValidationID() string

I'm missing more than just two
can you go over it and check that we have implement all the methods in the interface?

Hello @eifrach , the interface type is SupportLevelFeature. Following https://github.com/openshift/assisted-service/blob/master/internal/featuresupport/support_level_feature.go, all the methods have been implemented here. Thanks.

eifrach · 2024-11-20T11:16:57Z

internal/operators/osc/config.go

+	MasterMemory int64 = 1
+	MasterCPU    int64 = 1
+	// Memory value provided in GIB
+	WorkerMemory int64 = 1
+	WorkerCPU    int64 = 1


those are not in use

eifrach · 2024-11-20T11:17:56Z

internal/operators/osc/operator.go

+		return nil, err
+	}
+
+	return preflightRequirements.Requirements.Master.Quantitative, nil


is the requirement the same for Master and workers?
OSC workloads runs on all nodes?

@eifrach The requirements should be different for master and worker nodes. The OSC workloads run on worker nodes. And when starting a OSC pod, the default VM size is: 2G memory and 1 CPU. I will discuss with Xiangchun to set a new value here. Thanks.

eifrach · 2024-11-20T14:00:25Z

internal/operators/osc/manifest_test.go

+			Expect(err).To(BeNil())
+
+			Expect(extractData(controllerData, "metadata.namespace")).To(Equal(Namespace))
+		})


missing checkes for getLayeredImageDeployConfigMap

eifrach · 2024-11-20T14:05:50Z

internal/operators/osc/operator_test.go

+			Expect(operator.GetName()).To(Equal(Name))
+			Expect(operator.GetFullName()).To(Equal(FullName))


this can never fail

Yes, I agree. We can remove this function. Thanks.

eifrach · 2024-11-20T14:07:44Z

internal/operators/osc/operator_test.go

+			Expect(requirements).To(BeEquivalentTo(expectedRequirements))
+
+		},
+			Entry("min requirements", models.HostRoleMaster, newRequirements(minCpu, minRamMib)),


need more tests
role.master, not enough memory or CPU

eifrach · 2024-11-20T14:08:50Z

internal/operators/osc/operator_test.go

+		It("should return the dependencies", func() {
+			preflightRequirements, err := operator.GetPreflightRequirements(context.TODO(), &cluster)
+			Expect(err).To(BeNil())
+			Expect(preflightRequirements.Dependencies).To(Equal([]string{cnv.Operator.Name}))


eifrach · 2024-11-20T14:10:25Z

internal/operators/osc/operator_test.go

+		})
+
+		It("host should be valid", func() {
+			host := models.Host{Role: models.HostRoleMaster, Inventory: getInventory(int64(1024))}


testing only role master?

openshift-ci · 2024-11-24T13:14:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xiangchunfu
Once this PR has been reviewed and has the lgtm label, please ask for approval from eifrach. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Signed-off-by: xiangchun Fu <xfu@redhat.com>

openshift-ci · 2024-11-25T17:24:55Z

@xiangchunfu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/edge-e2e-metal-assisted-cnv-4-17	`fa68f19`	link	true	`/test edge-e2e-metal-assisted-cnv-4-17`
ci/prow/edge-e2e-metal-assisted-odf-4-17	`fa68f19`	link	true	`/test edge-e2e-metal-assisted-odf-4-17`
ci/prow/edge-e2e-metal-assisted-lvm	`fa68f19`	link	true	`/test edge-e2e-metal-assisted-lvm`
ci/prow/edge-e2e-ai-operator-disconnected-capi	`fa68f19`	link	false	`/test edge-e2e-ai-operator-disconnected-capi`
ci/prow/edge-e2e-ai-operator-ztp-capi	`fa68f19`	link	false	`/test edge-e2e-ai-operator-ztp-capi`

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.

pezhang · 2024-11-26T10:43:25Z

internal/operators/osc/operator.go

+				},
+			},
+		},
+	}, nil


@xiangchunfu Here we should consider the SNO scenario. Let's discuss offline about the details.

openshift-ci bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 13, 2024

openshift-ci bot requested review from javipolo and romfreiman November 13, 2024 08:39

openshift-ci bot added the api-review Categorizes an issue or PR as actively needing an API review. label Nov 13, 2024

$eifrach$

eifrach reviewed Nov 13, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2024

xiangchunfu force-pushed the osc-operator branch from 13f4ae5 to fb67951 Compare November 14, 2024 07:50

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2024

xiangchunfu force-pushed the osc-operator branch 5 times, most recently from 5c98524 to 66bfe9c Compare November 19, 2024 06:59

ariel-adam reviewed Nov 20, 2024

View reviewed changes

$eifrach$

eifrach suggested changes Nov 20, 2024

View reviewed changes

openshift-ci bot assigned eifrach Nov 20, 2024

xiangchunfu force-pushed the osc-operator branch from 66bfe9c to 089a5ae Compare November 24, 2024 13:13

xiangchunfu added 2 commits November 24, 2024 08:21

Add osc operator

b5eafcd

Signed-off-by: xiangchun Fu <xfu@redhat.com>

Add osc operator unittest

9081769

Signed-off-by: xiangchun Fu <xfu@redhat.com>

xiangchunfu force-pushed the osc-operator branch from 089a5ae to ffa89b9 Compare November 24, 2024 13:25

xiangchunfu added 2 commits November 25, 2024 08:44

Fix go file format

9c842f2

Signed-off-by: xiangchun Fu <xfu@redhat.com>

Fixed go files format and resolved comments

fa68f19

Signed-off-by: xiangchun Fu <xfu@redhat.com>

xiangchunfu force-pushed the osc-operator branch from ffa89b9 to fa68f19 Compare November 25, 2024 14:02

pezhang reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Osc operator #6990

Osc operator #6990

xiangchunfu commented Nov 13, 2024

$@eifrach$ eifrach Nov 13, 2024

$@eifrach$ eifrach Nov 13, 2024

codecov bot commented Nov 13, 2024 •

edited

Loading

ariel-adam Nov 20, 2024

$@eifrach$ eifrach Nov 13, 2024

pezhang Nov 25, 2024

$@eifrach$ eifrach Nov 20, 2024

$@eifrach$ eifrach Nov 20, 2024

$@eifrach$ eifrach Nov 20, 2024

pezhang Nov 25, 2024

$@eifrach$ eifrach Nov 20, 2024

$@eifrach$ eifrach Nov 20, 2024

pezhang Nov 26, 2024

$@eifrach$ eifrach Nov 20, 2024

$@eifrach$ eifrach Nov 20, 2024

pezhang Nov 26, 2024

$@eifrach$ eifrach Nov 20, 2024

$@eifrach$ eifrach Nov 20, 2024

$@eifrach$ eifrach Nov 20, 2024

openshift-ci bot commented Nov 24, 2024

openshift-ci bot commented Nov 25, 2024

pezhang Nov 26, 2024

	startingCSV: startingCSV: sandboxed-containers-operator.v1.7.0
	startingCSV: sandboxed-containers-operator.v1.7.0

		Expect(operator.GetName()).To(Equal(Name))
		Expect(operator.GetFullName()).To(Equal(FullName))

Osc operator #6990

Are you sure you want to change the base?

Osc operator #6990

Conversation

xiangchunfu commented Nov 13, 2024

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

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

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

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 bot commented Nov 24, 2024

openshift-ci bot commented Nov 25, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2024 •

edited

Loading