Skip to content

Commit

Permalink
[cherry-pick]: from ODH to RHOAI with refactor and bug fixes (#232)
Browse files Browse the repository at this point in the history
* chore: move testing fixtures to fixtures directory (#906)

This change aims to enhance the readability of our test suite.

It brings `/crds` and `/templates` into `/fixtures` and moves more helper functions out into dedicated files.

* feat(build): adds gci to go fmt target (#925)

This update incorporates the `gci` tool into the `make fmt` command, aiming to improve the formatting of Go files and ensure that imports adhere to our established conventions.

Unfortunately, due to how `.golangci.yml` is defined, we are not able to just use `--disable-all --enable=gci` flags. This yields an error as we use `enable-all` and `disable` in the config file for linter.

To address this, the `golangci-lint` configuration is dynamically adjusted using the `yq` tool, which is now included in our toolchain.

This process creates a temporary configuration file specifically for `golangci-lint` execution, ensuring `gci`'s integration without altering the primary linting setup.

Furthermore, the generated temporary file is added to the `CLEANFILES` list, ensuring its removal when calling `make clean`, maintaining a clean build environment.

To prevent this temporary file from being tracked by Git, a `*.mktmp.*` pattern has been added to our `.gitignore`, safeguarding against accidental inclusion of temporary files in the repository.: 

### Tidies

- removes duplicate `blank` import section in `gci` config, otherwise it can add `_` imports twice

* chore: splits feature tests to separate files (#924)

As the number of test cases (grouped as `Describe`) started to grow, we can split a single file into several groups per functionality.

Follow-up for #906.

* fix(dashboard): do not set owner on CR (#923)

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* (fix): Description patch during make bundle (#926)

* feat(DW): prepare CF,ray,kueue for GA (#929)

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* fix(dashboard): wrong path for consolelink in downstream (#933)

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* docs: remove modelregistry and update default application NS

Signed-off-by: Zhou, Wen <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Signed-off-by: Zhou, Wen <wenzhou@redhat.com>
Co-authored-by: Cameron Garrison <cgarriso@redhat.com>
Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Co-authored-by: Ajay Jaganathan <36824134+AjayJagan@users.noreply.github.com>
  • Loading branch information
4 people authored Mar 25, 2024
1 parent c4b93d5 commit dd2b28f
Show file tree
Hide file tree
Showing 28 changed files with 2,756 additions and 744 deletions.
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,6 @@ cover.out
# Ignore any local.mk files that would be consumed by the Makefile
local.mk

# Ignore temporary files created by the Makefile
*.mktmp.*

1 change: 0 additions & 1 deletion .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ linters-settings:
sections:
- standard
- default
- blank
- prefix(github.com/opendatahub-io/opendatahub-operator)
- blank
- dot
Expand Down
13 changes: 12 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,13 @@ ENVTEST ?= $(LOCALBIN)/setup-envtest
OPERATOR_SDK ?= $(LOCALBIN)/operator-sdk
GOLANGCI_LINT ?= $(LOCALBIN)/golangci-lint
CRD_REF_DOCS ?= $(LOCALBIN)/crd-ref-docs
YQ ?= $(LOCALBIN)/yq
## Tool Versions
KUSTOMIZE_VERSION ?= v3.8.7
CONTROLLER_GEN_VERSION ?= v0.9.2
OPERATOR_SDK_VERSION ?= v1.24.1
GOLANGCI_LINT_VERSION ?= v1.54.0
YQ_VERSION ?= v4.12.2
# ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
ENVTEST_K8S_VERSION = 1.24.2
ENVTEST_PACKAGE_VERSION = v0.0.0-20240320141353-395cfc7486e6
Expand Down Expand Up @@ -150,9 +152,13 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust
generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
$(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths="./..."

GOLANGCI_TMP_FILE = .golangci.mktmp.yml
.PHONY: fmt
fmt: ## Run go fmt against code.
fmt: golangci-lint yq ## Formats code and imports.
go fmt ./...
$(YQ) e '.linters = {"disable-all": true, "enable": ["gci"]}' .golangci.yml > $(GOLANGCI_TMP_FILE)
$(GOLANGCI_LINT) run --config=$(GOLANGCI_TMP_FILE) --fix
CLEANFILES += $(GOLANGCI_TMP_FILE)

.PHONY: vet
vet: ## Run go vet against code.
Expand Down Expand Up @@ -242,6 +248,11 @@ controller-gen: $(CONTROLLER_GEN) ## Download controller-gen locally if necessar
$(CONTROLLER_GEN): $(LOCALBIN)
test -s $(CONTROLLER_GEN) || GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-tools/cmd/controller-gen@$(CONTROLLER_GEN_VERSION)

.PHONY: yq
yq: $(YQ) ## Download yq locally if necessary.
$(YQ): $(LOCALBIN)
test -s $(YQ) || GOBIN=$(LOCALBIN) go install github.com/mikefarah/yq/v4@$(YQ_VERSION)

OPERATOR_SDK_DL_URL ?= https://github.com/operator-framework/operator-sdk/releases/download/$(OPERATOR_SDK_VERSION)
.PHONY: operator-sdk
operator-sdk: $(OPERATOR_SDK) ## Download and install operator-sdk
Expand Down
6 changes: 3 additions & 3 deletions bundle/manifests/rhods-operator.clusterserviceversion.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ metadata:
"spec": {
"components": {
"codeflare": {
"managementState": "Removed"
"managementState": "Managed"
},
"dashboard": {
"managementState": "Managed"
Expand All @@ -41,13 +41,13 @@ metadata:
}
},
"kueue": {
"managementState": "Removed"
"managementState": "Managed"
},
"modelmeshserving": {
"managementState": "Managed"
},
"ray": {
"managementState": "Removed"
"managementState": "Managed"
},
"workbenches": {
"managementState": "Managed"
Expand Down
4 changes: 2 additions & 2 deletions components/dashboard/dashboard.go
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,9 @@ func (d *Dashboard) deployConsoleLink(cli client.Client, owner metav1.Object, pl
}

enabled := d.ManagementState == operatorv1.Managed
err = deploy.DeployManifestsFromPath(cli, owner, PathConsoleLink, namespace, componentName, enabled)
err = deploy.DeployManifestsFromPath(cli, owner, manifestsPath, namespace, componentName, enabled)
if err != nil {
return fmt.Errorf("failed to set dashboard consolelink from %s: %w", PathConsoleLink, err)
return fmt.Errorf("failed to set dashboard consolelink from %s: %w", manifestsPath, err)
}

return nil
Expand Down
1 change: 0 additions & 1 deletion config/manifests/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ patches:
target:
group: operators.coreos.com
kind: ClusterServiceVersion
name: rhods-operator.v2.0.0

# [WEBHOOK] To enable webhooks, uncomment all the sections with [WEBHOOK] prefix.
# Do NOT uncomment sections with prefix [CERTMANAGER], as OLM does not support cert-manager.
Expand Down
6 changes: 3 additions & 3 deletions config/samples/datasciencecluster_v1_datasciencecluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ metadata:
spec:
components:
codeflare:
managementState: "Removed"
managementState: "Managed"
dashboard:
managementState: "Managed"
datasciencepipelines:
Expand All @@ -31,8 +31,8 @@ spec:
modelmeshserving:
managementState: "Managed"
kueue:
managementState: "Removed"
managementState: "Managed"
ray:
managementState: "Removed"
managementState: "Managed"
workbenches:
managementState: "Managed"
32 changes: 5 additions & 27 deletions docs/api-overview.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ _Appears in:_
- [Kserve](#kserve)
- [Kueue](#kueue)
- [ModelMeshServing](#modelmeshserving)
- [ModelRegistry](#modelregistry)
- [Ray](#ray)
- [TrustyAI](#trustyai)
- [Workbenches](#workbenches)
Expand Down Expand Up @@ -230,29 +229,6 @@ _Appears in:_



## datasciencecluster.opendatahub.io/modelregistry

Package modelregistry provides utility functions to config ModelRegistry, an ML Model metadata repository service



#### ModelRegistry



ModelRegistry struct holds the configuration for the ModelRegistry component.



_Appears in:_
- [Components](#components)

| Field | Description | Default | Validation |
| --- | --- | --- | --- |
| `Component` _[Component](#component)_ | | | |



## datasciencecluster.opendatahub.io/ray

Package ray provides utility functions to config Ray as part of the stack
Expand Down Expand Up @@ -288,6 +264,9 @@ Package trustyai provides utility functions to config TrustyAI, a bias/fairness


TrustyAI struct holds the configuration for the TrustyAI component.
## DEPRECATED ## : Installation of TrustyAI operator is deprecated in RHOAI.
If TrustyAI operator is installed, it will be removed
Changes in managemenstState are not supported.



Expand Down Expand Up @@ -378,7 +357,6 @@ _Appears in:_
| `codeflare` _[CodeFlare](#codeflare)_ | CodeFlare component configuration.<br />If CodeFlare Operator has been installed in the cluster, it should be uninstalled first before enabled component. | | |
| `ray` _[Ray](#ray)_ | Ray component configuration. | | |
| `trustyai` _[TrustyAI](#trustyai)_ | TrustyAI component configuration. | | |
| `modelregistry` _[ModelRegistry](#modelregistry)_ | ModelRegistry component configuration. | | |


#### ControlPlaneSpec
Expand Down Expand Up @@ -577,7 +555,7 @@ _Appears in:_

| Field | Description | Default | Validation |
| --- | --- | --- | --- |
| `applicationsNamespace` _string_ | Namespace for applications to be installed, non-configurable, default to "opendatahub" | opendatahub | |
| `applicationsNamespace` _string_ | Namespace for applications to be installed, non-configurable, default to "redhat-ods-applications" | redhat-ods-applications | |
| `monitoring` _[Monitoring](#monitoring)_ | Enable monitoring on specified namespace | | |
| `serviceMesh` _[ServiceMeshSpec](#servicemeshspec)_ | Configures Service Mesh as networking layer for Data Science Clusters components.<br />The Service Mesh is a mandatory prerequisite for single model serving (KServe) and<br />you should review this configuration if you are planning to use KServe.<br />For other components, it enhances user experience; e.g. it provides unified<br />authentication giving a Single Sign On experience. | | |
| `trustedCABundle` _[TrustedCABundleSpec](#trustedcabundlespec)_ | When set to `Managed`, adds odh-trusted-ca-bundle Configmap to all namespaces that includes<br />cluster-wide Trusted CA Bundle in .data["ca-bundle.crt"].<br />Additionally, this fields allows admins to add custom CA bundles to the configmap using the .CustomCABundle field. | | |
Expand Down Expand Up @@ -634,7 +612,7 @@ _Appears in:_
| Field | Description | Default | Validation |
| --- | --- | --- | --- |
| `managementState` _[ManagementState](#managementstate)_ | Set to one of the following values:<br />- "Managed" : the operator is actively managing the component and trying to keep it active.<br /> It will only upgrade the component if it is safe to do so.<br />- "Removed" : the operator is actively managing the component and will not install it,<br /> or if it is installed, the operator will try to remove it. | | Enum: [Managed Removed] <br /> |
| `namespace` _string_ | Namespace for monitoring if it is enabled | opendatahub | |
| `namespace` _string_ | Namespace for monitoring if it is enabled | redhat-ods-monitoring | |


#### TrustedCABundleSpec
Expand Down
7 changes: 4 additions & 3 deletions pkg/upgrade/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,13 @@ func CreateDefaultDSC(ctx context.Context, cli client.Client) error {
Component: components.Component{ManagementState: operatorv1.Managed},
},
CodeFlare: codeflare.CodeFlare{
Component: components.Component{ManagementState: operatorv1.Removed},
Component: components.Component{ManagementState: operatorv1.Managed},
},
Ray: ray.Ray{
Component: components.Component{ManagementState: operatorv1.Removed},
Component: components.Component{ManagementState: operatorv1.Managed},
},
Kueue: kueue.Kueue{
Component: components.Component{ManagementState: operatorv1.Removed},
Component: components.Component{ManagementState: operatorv1.Managed},
},
},
},
Expand Down Expand Up @@ -344,6 +344,7 @@ func UpdateFromLegacyVersion(cli client.Client, platform deploy.Platform, appNS
if err := deleteResource(cli, montNamespace, "statefulset"); err != nil {
return err
}
// only for downstream since ODH has a different way to create this CR by dashboard
if err := unsetOwnerReference(cli, "odh-dashboard-config", appNS); err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func setupDSCInstance() *dsc.DataScienceCluster {
},
CodeFlare: codeflare.CodeFlare{
Component: components.Component{
ManagementState: operatorv1.Removed,
ManagementState: operatorv1.Managed,
},
},
Ray: ray.Ray{
Expand Down
111 changes: 111 additions & 0 deletions tests/integration/features/cleanup_int_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package features_test

import (
"context"

"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"
featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature"
"github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil"
"github.com/opendatahub-io/opendatahub-operator/v2/tests/integration/features/fixtures"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
. "github.com/onsi/gomega/gstruct"
)

var _ = Describe("feature cleanup", func() {

Context("using FeatureTracker and ownership as cleanup strategy", Ordered, func() {

const (
featureName = "create-secret"
secretName = "test-secret"
)

var (
dsci *dsciv1.DSCInitialization
namespace string
featuresHandler *feature.FeaturesHandler
)

BeforeAll(func() {
namespace = envtestutil.AppendRandomNameTo("test-secret-ownership")
dsci = fixtures.NewDSCInitialization(namespace)
featuresHandler = feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error {
secretCreationErr := feature.CreateFeature(featureName).
For(handler).
UsingConfig(envTest.Config).
PreConditions(
feature.CreateNamespaceIfNotExists(namespace),
).
WithResources(fixtures.CreateSecret(secretName, namespace)).
Load()

Expect(secretCreationErr).ToNot(HaveOccurred())

return nil
})

})

It("should successfully create resource and associated feature tracker", func() {
// when
Expect(featuresHandler.Apply()).Should(Succeed())

// then
Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, secretName)).
WithTimeout(fixtures.Timeout).
WithPolling(fixtures.Interval).
Should(Succeed())
})

It("should remove feature tracker on clean-up", func() {
// when
Expect(featuresHandler.Delete()).To(Succeed())

// then
Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, secretName)).
WithTimeout(fixtures.Timeout).
WithPolling(fixtures.Interval).
Should(WithTransform(errors.IsNotFound, BeTrue()))
})

})

})

func createdSecretHasOwnerReferenceToOwningFeature(namespace, secretName string) func() error {
return func() error {
secret, err := envTestClientset.CoreV1().
Secrets(namespace).
Get(context.TODO(), secretName, metav1.GetOptions{})

if err != nil {
return err
}

Expect(secret.OwnerReferences).Should(
ContainElement(
MatchFields(IgnoreExtras, Fields{"Kind": Equal("FeatureTracker")}),
),
)

trackerName := ""
for _, ownerRef := range secret.OwnerReferences {
if ownerRef.Kind == "FeatureTracker" {
trackerName = ownerRef.Name
break
}
}

tracker := &featurev1.FeatureTracker{}
return envTestClient.Get(context.Background(), client.ObjectKey{
Name: trackerName,
}, tracker)
}
}
Loading

0 comments on commit dd2b28f

Please sign in to comment.